Change UserAgent App WindowsPowerShell -> PowerShell by markekraus · Pull Request #4914 · PowerShell/PowerShell · GitHub

renames the App portion of the user agent to PowerShell
Adjusts tests.

Thanks!

This match would work with WindowsPowerShell as well. This should be ^PowerShell.

In addition to that, it should be MatchExactly

User-Agent can be PowerShell/6.0.0-Beta.8 or PowerShell/6.0.0- so if we use MatchExactly the pattern should be "^PowerShell/\d+\.\d+\.\d+.*"
We use the pattern many times - it make sense to use a variable.

Makes sense.

It was doing an imprecise partial match before.

It's actually more like Mozilla/5.0 (Windows NT; Microsoft Windows 10.0.15063 ; en-US) WindowsPowerShell/6.0.0 and it will be more dynamic "soon"

we could pull the exact one through reflection in BeforeAll. Honestly, I'm not even sure this is a good test. Should Be $uri.Authority should be sufficient, IMO.

fixed.

Same as above.

Same as above

Test fails due to #4919

Leave a comment

It is dummy test: if C# code will be changed the test will be automatically passed. I believe we should use explicit desired pattern.
Also please move to BeforeAll.

That's not the purpose of the tests. They are just testing the response from the server to ensure that expected data sent is also returned. if we want to test the patterns in the UserAgent, that would be a separate set of tests for the PSUserAgent class.

I will add a separate describe block for PSUserAgent tests and a test for the App member (there will be other tests possibly for #4193). I will create a separate issue to add and track adding other tests for PSUserAgent.

Here I don't see the point of checking internal property - we are free to change (add/remove) internals in any time. We should test public API. Here "public API" is User-Agent which we send to a server and get it back. We should test the User-Agent format if we want to control the User-Agent. So it is good to use regex pattern. I think that check User-Agent 14 times it's redundant - we need to have one check for each web cmdlet.

Again, the point of most of those tests is not to test the User-Agent that is sent. Most of those assertions are being used to ensure meaningful data is returned in a response after a set of parameters has been used to make web requests. The only reason I updated them at all was because the change from WindowsPowerShell to PowerShell would have caused all these tests to false fail.

I will change the two tests that are explicitly about the User-Agent.

As you mentioned $uri.Authority is sufficient. So I'd remove $DefaultUserAgent at all. I unlike it as you see 😄

I'm ok with that. This entire file needs to be more deeply analyzed and cleaned up. Maybe after I'm done making radical changes to it every day 😆.

Remove all assertions for User-Agent that are not specifically about the User-Agent header.

I think you might use MatchExactly which does a case sensitive match (-cmatch). Especially since you're explicitly not matching WindowsPowerShell

fixed

LGTM

* Change UserAgent App WindowsPowerShell -> PowerShell * [Feature] Run Feature tests * [feature] Address PR Feedback with more precise assertion * [feature] Address PR Feedback and add PSUserAgent Test * [feature] add tag to describe block * [feature] Remove describe block Add pattern to User-Agent specific tests * [feature] Address PF Feedback Remove all assertions for User-Agent that are not specifically about the User-Agent header. * [feature] Match -> MatchExactly

Change UserAgent App WindowsPowerShell -> PowerShell

ea9f5d8

Mark Kraus (markekraus) requested review from James Truher (JamesWTruher), Paul Higinbotham (PaulHigin) and Dan Travison (dantraMSFT) as code owners September 25, 2017 00:18

Microsoft Contribution License Agreements (msftclas) added the cla-already-signed label Sep 25, 2017

[Feature] Run Feature tests

9313cdb

Steve Lee (SteveL-MSFT) approved these changes Sep 25, 2017

View reviewed changes

Dongbo Wang (daxian-dbw) assigned Aditya Patwardhan (adityapatwardhan) Sep 25, 2017

Aditya Patwardhan (adityapatwardhan) requested changes Sep 25, 2017

View reviewed changes

[feature] Address PR Feedback with more precise assertion

6ecbdcc

Ilya (iSazonov) reviewed Sep 26, 2017

View reviewed changes

Microsoft GitHub User (msftgits) removed the cla-already-signed label Sep 26, 2017

[feature] Address PR Feedback and add PSUserAgent Test

59e02b4

Ilya (iSazonov) reviewed Sep 26, 2017

View reviewed changes

Mark Kraus (markekraus) added 3 commits September 26, 2017 04:13

[feature] add tag to describe block

7198bec

[feature] Remove describe block Add pattern to User-Agent specific tests

671fc63

[feature] Address PF Feedback

ba056d9

Remove all assertions for User-Agent that are not specifically about the User-Agent header.

Ilya (iSazonov) approved these changes Sep 26, 2017

View reviewed changes

James Truher (JamesWTruher) approved these changes Sep 26, 2017

View reviewed changes

[feature] Match -> MatchExactly

5caf544

Aditya Patwardhan (adityapatwardhan) approved these changes Sep 26, 2017

View reviewed changes

PowerShell Team (PowerShell) deleted a comment from Microsoft Contribution License Agreements (msftclas) Sep 27, 2017

Aditya Patwardhan (adityapatwardhan) merged commit a8e8b1f into PowerShell:master Sep 27, 2017

Mark Kraus (markekraus) mentioned this pull request Oct 5, 2017

beta.8 changelog draft #5006

Merged

Mark Kraus (markekraus) deleted the FixDefaultUserAgent branch January 19, 2018 18:57

                       $jsonContent = $result.Output | ConvertFrom-Json
                       $jsonContent.headers.Host | Should Be $uri.Authority
-                      $jsonContent.headers.'User-Agent' | Should Match "WindowsPowerShell"
+                      $jsonContent.headers.'User-Agent' | Should Match "PowerShell"

                       # Validate response
                       $result.Output.headers.Host | Should Be $Uri.Authority
-                      $result.Output.headers.'User-Agent' | Should Match "WindowsPowerShell"
+                      $result.Output.headers.'User-Agent' | Should Match "PowerShell"

                       # Validate response
                       $result.Output.headers.Host | Should Be $uri.Authority
-                      $result.Output.headers.'User-Agent' | Should Match "WindowsPowerShell"
+                      $result.Output.headers.'User-Agent' | Should Match "PowerShell"

                       # Validate response
                       $result.Output.headers.Host | Should Match $uri.Authority
-                      $result.Output.headers.'User-Agent' | Should Match "WindowsPowerShell"
+                      $result.Output.headers.'User-Agent' | Should Match "PowerShell"

                               # Validate response
                               $result.Output.url | Should Match $uri
-                              $result.Output.headers.'User-Agent' | Should Match "WindowsPowerShell"
+                              $result.Output.headers.'User-Agent' | Should Match "PowerShell"

               if ( test-path /etc/centos-release ) { $PendingCertificateTest = $true }
+              # Get the default UserAgent through reflection
+              $DefaultUserAgent = [Microsoft.PowerShell.Commands.PSUserAgent].GetProperty('UserAgent',@('Static','NonPublic')).GetValue($null,$null)

+              Describe "PSUserAgent Tests" {
+                  It "App Should Match ^PowerShell/\d+\.\d+\.\d+.*" {
+                      $app = [Microsoft.PowerShell.Commands.PSUserAgent].GetProperty('App',@('Static','NonPublic')).GetValue($null,$null)
+                      $app | Should Match '^PowerShell/\d+\.\d+\.\d+.*'

                       # Validate response content
                       $jsonContent = $result.Output.Content | ConvertFrom-Json
-                      $jsonContent.headers.'User-Agent' | Should Match "WindowsPowerShell"
+                      $jsonContent.headers.'User-Agent' | Should Match '(?<!Windows)PowerShell\/\d+\.\d+\.\d+.*'

Change UserAgent App WindowsPowerShell -> PowerShell by markekraus · Pull Request #4914 · PowerShell/PowerShell · GitHub

Uh oh!

Change UserAgent App WindowsPowerShell -> PowerShell - #4914

Change UserAgent App WindowsPowerShell -> PowerShell#4914Aditya Patwardhan (adityapatwardhan) merged 8 commits intoPowerShell:masterPowerShell/PowerShell:masterfrom markekraus:FixDefaultUserAgentmarkekraus/PowerShell:FixDefaultUserAgentCopy head branch name to clipboard

Conversation

Mark Kraus (markekraus) commented Sep 25, 2017

Uh oh!

Steve Lee (SteveL-MSFT) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mark Kraus (markekraus) Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mark Kraus (markekraus) commented Sep 26, 2017

Uh oh!

Ilya (iSazonov) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Aditya Patwardhan (adityapatwardhan) left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Change UserAgent App WindowsPowerShell -> PowerShell#4914
Aditya Patwardhan (adityapatwardhan) merged 8 commits into
PowerShell:masterfrom
markekraus:FixDefaultUserAgent

Ilya (iSazonov) Sep 25, 2017 •
edited
Loading

Mark Kraus (markekraus) Sep 25, 2017 •
edited
Loading