Change UserAgent App WindowsPowerShell -> PowerShell #4914

markekraus · 2017-09-25T00:18:59Z

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

SteveL-MSFT

Thanks!

adityapatwardhan · 2017-09-25T17:20:48Z

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

@@ -720,7 +720,7 @@ Describe "Invoke-WebRequest tests" -Tags "Feature" {
        $result = ExecuteRequestWithOutFile -cmdletName "Invoke-WebRequest" -uri $uri
        $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"


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.

adityapatwardhan · 2017-09-25T17:20:56Z

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

@@ -1322,7 +1322,7 @@ Describe "Invoke-RestMethod tests" -Tags "Feature" {
        $result = ExecuteWebCommand -command $command

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


Same as above.

adityapatwardhan · 2017-09-25T17:21:05Z

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

@@ -1334,7 +1334,7 @@ Describe "Invoke-RestMethod tests" -Tags "Feature" {

        # 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"


Same as above

adityapatwardhan · 2017-09-25T17:21:09Z

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

@@ -1347,7 +1347,7 @@ Describe "Invoke-RestMethod tests" -Tags "Feature" {

        # 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"


Same as above

adityapatwardhan · 2017-09-25T17:21:16Z

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

@@ -1360,7 +1360,7 @@ Describe "Invoke-RestMethod tests" -Tags "Feature" {

        # 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"


Same as above

adityapatwardhan · 2017-09-25T17:21:21Z

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

@@ -1435,7 +1435,7 @@ Describe "Invoke-RestMethod tests" -Tags "Feature" {
        $result = ExecuteWebCommand -command $command

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


Same as above

adityapatwardhan · 2017-09-25T17:21:26Z

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

@@ -1497,7 +1497,7 @@ Describe "Invoke-RestMethod tests" -Tags "Feature" {

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


Same as above

adityapatwardhan · 2017-09-25T17:21:31Z

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

@@ -1525,7 +1525,7 @@ Describe "Invoke-RestMethod tests" -Tags "Feature" {

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


Same as above

adityapatwardhan · 2017-09-25T17:21:36Z

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

@@ -1560,7 +1560,7 @@ Describe "Invoke-RestMethod tests" -Tags "Feature" {
        $result = ExecuteRequestWithOutFile -cmdletName "Invoke-RestMethod" -uri $uri
        $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"


Same as above

markekraus · 2017-09-26T01:35:06Z

Test fails due to #4919

iSazonov

Leave a comment

iSazonov · 2017-09-26T04:58:56Z

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

@@ -421,6 +421,9 @@ $PendingCertificateTest = $false
 if ( $IsMacOS ) { $PendingCertificateTest = $true }
 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)


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.

iSazonov · 2017-09-26T09:12:37Z

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

+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+.*'


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.

JamesWTruher · 2017-09-26T17:52:27Z

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

@@ -463,7 +463,7 @@ Describe "Invoke-WebRequest tests" -Tags "Feature" {

        # 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+.*'


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

adityapatwardhan

LGTM

Change UserAgent App WindowsPowerShell -> PowerShell

ea9f5d8

markekraus requested review from dantraMSFT, JamesWTruher and PaulHigin as code owners September 25, 2017 00:18

msftclas added the cla-already-signed label Sep 25, 2017

[Feature] Run Feature tests

9313cdb

SteveL-MSFT approved these changes Sep 25, 2017

View reviewed changes

daxian-dbw assigned adityapatwardhan Sep 25, 2017

adityapatwardhan requested changes Sep 25, 2017

View reviewed changes

[feature] Address PR Feedback with more precise assertion

6ecbdcc

iSazonov reviewed Sep 26, 2017

View reviewed changes

msftgits removed the cla-already-signed label Sep 26, 2017

[feature] Address PR Feedback and add PSUserAgent Test

59e02b4

iSazonov reviewed Sep 26, 2017

View reviewed changes

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.

iSazonov approved these changes Sep 26, 2017

View reviewed changes

JamesWTruher approved these changes Sep 26, 2017

View reviewed changes

[feature] Match -> MatchExactly

5caf544

adityapatwardhan approved these changes Sep 26, 2017

View reviewed changes

PowerShell deleted a comment from msftclas Sep 27, 2017

adityapatwardhan merged commit a8e8b1f into PowerShell:master Sep 27, 2017

markekraus mentioned this pull request Oct 5, 2017

beta.8 changelog draft #5006

Merged

markekraus deleted the FixDefaultUserAgent branch January 19, 2018 18:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change UserAgent App WindowsPowerShell -> PowerShell #4914

Change UserAgent App WindowsPowerShell -> PowerShell #4914

markekraus commented Sep 25, 2017

SteveL-MSFT left a comment

adityapatwardhan Sep 25, 2017

iSazonov Sep 25, 2017 •

edited

adityapatwardhan Sep 25, 2017

markekraus Sep 25, 2017 •

edited

markekraus Sep 26, 2017

adityapatwardhan Sep 25, 2017

adityapatwardhan Sep 25, 2017

adityapatwardhan Sep 25, 2017

adityapatwardhan Sep 25, 2017

adityapatwardhan Sep 25, 2017

adityapatwardhan Sep 25, 2017

adityapatwardhan Sep 25, 2017

adityapatwardhan Sep 25, 2017

markekraus commented Sep 26, 2017

iSazonov left a comment

iSazonov Sep 26, 2017

markekraus Sep 26, 2017

iSazonov Sep 26, 2017

markekraus Sep 26, 2017

iSazonov Sep 26, 2017

markekraus Sep 26, 2017

JamesWTruher Sep 26, 2017

markekraus Sep 26, 2017

adityapatwardhan left a comment

Change UserAgent App WindowsPowerShell -> PowerShell #4914

Change UserAgent App WindowsPowerShell -> PowerShell #4914

Conversation

markekraus commented Sep 25, 2017

SteveL-MSFT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iSazonov Sep 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markekraus Sep 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markekraus commented Sep 26, 2017

iSazonov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adityapatwardhan left a comment

Choose a reason for hiding this comment

iSazonov Sep 25, 2017 •

edited

markekraus Sep 25, 2017 •

edited