Replace HttpListener Link Header Tests with WebListener #5806

markekraus · 2018-01-06T18:36:20Z

PR Summary

reference #5464
closes #4639

Add Link controller to WebListener
Replace HttpListener Link tests with WebListener
Update WebListener Documentation
Enable cross-platform multiple Link header tests

PR Checklist

Note: Please mark anything not applicable to this PR NA.

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
NA User facing Documentation needed
- NA Issue filed - Issue link:
Change is not breaking
Make sure you've added a new test if existing tests do not effectively test the code changed
- Add [feature] if the change is significant or affectes feature tests
This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.

markekraus · 2018-01-06T19:05:32Z

Let it be known that on this day of January 6th, 2018 the macOS CI build finished before Linux and Windows...

bergmeister · 2018-01-06T20:31:23Z

@markekraus Maybe because macOS hasn't patched the Spectre bug yet, you can thank Intel for that 😛 (just kidding)

bergmeister · 2018-01-06T20:40:41Z

test/tools/WebListener/Controllers/LinkController.cs

As far as I know, one of the last .Net Core SDK updates included the latest version of Roslyn, therefore it should be possible to inline the out parameter declarations as e.g. out StringValues maxLinksSV, etc.
Very minor but here and below is no space after the if statement (tip: just select everything and Ctrl+K+F auto-formats you all those things)

I'm not really a C# developer... so that inline trick is new to me. Good to know! I've changed all the instances of it.

For some reason.. the keyboard shortcuts for formatting are not working for me *scratches head. I had to right-click and chose format document. weird. Any way, formatting is fixed now.

I know, C# is moving quite fast now and it is hard to keep up, you can have a read here or watch one of the videos from Kasey Uhlenhuth if you are interested but I mainly learn those tricks also from other people when pair programming or reviewing. Here are my personal favourites :

String interpolation

Expression-bodied function members

Tuple naming instead of Item1, Item2

Thanks for pointing me in the right direction. Discards will come in handy too.

TravisEz13 · 2018-01-08T20:19:53Z

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

should there be any links in this case?

iSazonov · 2018-01-13T15:59:20Z

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

-        $result.Output.RelationLink["next"] | Should BeExactly "http://localhost:8080/PowerShell?test=linkheader&maxlinks=5&linknumber=2"
-        $result.Output.RelationLink["last"] | Should BeExactly "http://localhost:8080/PowerShell?test=linkheader&maxlinks=5&linknumber=5"
+
+        $result.Output.RelationLink.Count | Should BeExactly 5


Why we use BeExactly in the whole file for Count?

it was like that before I didn't change them. Does it really matter though?

I'd prefer Should Be because BeExactly is used for strings and my first thought was that here Count is a method in RelationLink which return a string.

@iSazonov This appears to happen in more place. Lets leave it as is here and I will address them all as part of #5669 which I have updated.

iSazonov · 2018-01-13T16:12:19Z

test/tools/WebListener/Controllers/LinkController.cs

+                linkNumber = 1;
+            }
+
+            string baseUri = Regex.Replace(UriHelper.GetDisplayUrl(Request), "\\?.*", String.Empty);


So simple? 👍

Something wrong with it?

No, I expected a url parsing 😄

heh. So far as I can tell, the only way to get the request url is GetDisplayUrl() which returns a string. I could use UriBuilder on that string.. BUT.. on macOS UriBuilder behaves differently with the query section resulting in invalid urls.. rather than mess with that, the regex works just fine :)

iSazonov · 2018-01-13T16:17:40Z

test/tools/WebListener/Controllers/LinkController.cs

+            var getController = new GetController();
+            getController.ControllerContext = this.ControllerContext;
+            var result = getController.Index();
+            var output = result.Value as Hashtable;


Never null?

It will always return a HashTable unless some uncaught exception prevents it from even getting to this point.

Thanks.

Closed.

markekraus · 2018-01-16T13:20:45Z

@daxian-dbw ping

markekraus requested review from TravisEz13, adityapatwardhan and daxian-dbw as code owners January 6, 2018 18:36

bergmeister reviewed Jan 6, 2018

View reviewed changes

daxian-dbw self-assigned this Jan 8, 2018

TravisEz13 reviewed Jan 8, 2018

View reviewed changes

TravisEz13 approved these changes Jan 8, 2018

View reviewed changes

markekraus added 2 commits January 9, 2018 15:58

[Feature] Replace HttpListener Link Header tests with WebListener

ebc5660

[Feature] Address PR Feedback

e31d601

markekraus force-pushed the ReplaceHttpListernerLink branch from 7f470d8 to e31d601 Compare January 9, 2018 21:59

markekraus mentioned this pull request Jan 12, 2018

Replace HttpListener Redirect Tests with WebListener #5872

Merged

6 tasks

SteveL-MSFT mentioned this pull request Jan 12, 2018

macOS is hanging during webcmdlets tests #5464

Closed

5 tasks

markekraus requested a review from iSazonov January 13, 2018 14:04

iSazonov reviewed Jan 13, 2018

View reviewed changes

iSazonov approved these changes Jan 14, 2018

View reviewed changes

daxian-dbw merged commit 3f9564e into PowerShell:master Jan 16, 2018

markekraus deleted the ReplaceHttpListernerLink branch January 19, 2018 18:59

Replace HttpListener Link Header Tests with WebListener #5806

Replace HttpListener Link Header Tests with WebListener #5806

Uh oh!

Conversation

markekraus commented Jan 6, 2018

PR Summary

PR Checklist

Uh oh!

markekraus commented Jan 6, 2018

Uh oh!

bergmeister commented Jan 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bergmeister Jan 6, 2018 • 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!

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 Jan 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bergmeister Jan 6, 2018 •

edited

Loading