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

        $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


        # 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

        # 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

        # 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

        # 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


        # 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

                # 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

        # 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

        $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

 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

        # 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 JamesWTruher, PaulHigin and dantraMSFT 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

Change UserAgent App WindowsPowerShell -> PowerShell #4914

Change UserAgent App WindowsPowerShell -> PowerShell #4914

Uh oh!

Conversation

markekraus commented Sep 25, 2017

Uh oh!

SteveL-MSFT left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

markekraus commented Sep 26, 2017

Uh oh!

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!

adityapatwardhan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

iSazonov Sep 25, 2017 •

edited

Loading

markekraus Sep 25, 2017 •

edited

Loading