Replace httpbin.org/get tests With WebListener#4738
Replace httpbin.org/get tests With WebListener#4738iSazonov merged 8 commits intoPowerShell:masterfrom
Conversation
|
@markekraus, |
|
AppVeyor Failure is unrelated |
There was a problem hiding this comment.
Please remove parentheses.
We use this many times - can we move the line to BeforeAll?
It seems the time has come to add ValidateSet() to Get-WebListenerUrl parameters.
There was a problem hiding this comment.
Unless I'm mistaken, the Parens are required when assign a default value with a statement in a Param block. At your suggestion I tried removing them but I get all kinds of errors.
As for moving this to the BeforeAll block, I don't think that makes sense in this context since this is part of the ExecuteRequestWithOutFile function.
For the more general concern, we could create a variable, but, as we move the other tests over you will see that it will get less possible to do so in the BeforeAll Block without losing meaning in the It blocks (in the case where there will be a query string required as well). I would prefer we keep the URL generation to each It block for consistency.
There was a problem hiding this comment.
Ah, sorry - My thoughts was about many Get-WebListenerUrl -Test 'Get' in the file - I see only 10 times so I think we can keep it as is.
There was a problem hiding this comment.
Please remove parentheses.
There was a problem hiding this comment.
Same parse issue when trying to remove.
There was a problem hiding this comment.
I am not a big fan of unencapsulated strings in evaluated commands. the command would become
Invoke-WebRequest -Uri http://localhost:8083/Get -CustomMethod GET -Body @{'testparam'='testvalue'}Which is improper IMO. Does this have to be done? I feel like this change was actually an improvement and I made it everywhere purposefully.
There was a problem hiding this comment.
Binding is not preprocessor, we don't parse the variable value, so we shouldn't quote variables. Uri get right string.
My thoughts was that we should use the file pattern.
There was a problem hiding this comment.
In this instance the $Command string is later evaluated by Invoke-Expression. $uri is expanded before that evaluation. In instances where the url may contain a $, it would be expanded first during the $command assignment and then when the Invoke-Expression is run on $command the $ in the url would attempt to be expanded again.
# $Field1 is a literal field name, not a variable
$uri = 'http://localhost:8083/Get?$Field1=Value1'
$Command = "Write-Output $uri"
Invoke-Expression $CommandResult:
http://localhost:8083/Get?=Value1
vs:
$uri = 'http://localhost:8083/Get?$Field1=Value1'
$Command = "Write-Output '$uri'"
Invoke-Expression $CommandResult:
http://localhost:8083/Get?$Field1=Value1
There was a problem hiding this comment.
Sorry again - I lost the context.
Closed.
There was a problem hiding this comment.
We use " ," twice - it is good to use a const.
Do we really have space?
There was a problem hiding this comment.
I will create a constant, as for the space, it should be the other way around ", " and yes, the space is there when multiple values are supplied for a given header.
X-Fake-Header: Value1
X-Fake-Header: Value2
and
X-Fake-Header: Value1, Value2
Are equivalent.
5bd6b82 to
c89de63
Compare
There was a problem hiding this comment.
It seems we skip a description for '/' in Readme.md.
There was a problem hiding this comment.
The default page will return a list of available tests.
I could make it an explicit section. but honestly, I only include it in the ValidateSet to accommodate scenarios where looping may always provide the -Test param but may not need any more than the default index. / is less of a test and more of a convenience during development of tests.
There was a problem hiding this comment.
Please clarify. Previously we used Get-WebListenerUrl -Https in tests - do we use by default '/'?
There was a problem hiding this comment.
we do return the / by default.
This is the scenario I included it for:
$Testcases = @(
@{
Name = 'Get Test'
Test = 'Get'
ExpectedResult = 'Foo'
Https = $False
}
@{
Name = 'Cert test'
Test = 'Cert'
ExpectedResult = 'Bar'
Https = $True
}
@{
Name = 'Default Test'
Test = '/'
ExpectedResult = 'Baz'
Https = $False
}
)
It "<Name>" -TestCases $TestCases {
Param($Name, $Test, $ExpectedResult, $Https)
$Url = Get-WebListenerUrl -Test $Test -Https:$Https
Invoke-RestMethod | Should Match $ExpectedResult
}-Test is always passed, but a named test might not always be needed.
There was a problem hiding this comment.
Thanks! I think we need the explicit section for -Test '/', -Test "", -Test $null. Maybe block last two by validate attribute?
There was a problem hiding this comment.
The ValidetSet already blocks them:
Get-WebListenerUrl -Test ""Get-WebListenerUrl : Cannot validate argument on parameter 'Test'. The argument "" does not
belong to the set "Cert,Get,Home,/" specified by the ValidateSet attribute. Supply an argument
that is in the set and then try the command again.
At line:1 char:26
+ Get-WebListenerUrl -Test ""
+ ~~
+ CategoryInfo : InvalidData: (:) [Get-WebListenerUrl], ParameterBindingValidationEx
ception
+ FullyQualifiedErrorId : ParameterArgumentValidationError,Get-WebListenerUrl
Get-WebListenerUrl -Test $NullGet-WebListenerUrl : Cannot validate argument on parameter 'Test'. The argument "" does not
belong to the set "Cert,Get,Home,/" specified by the ValidateSet attribute. Supply an argument
that is in the set and then try the command again.
At line:1 char:26
+ Get-WebListenerUrl -Test $Null
+ ~~~~~
+ CategoryInfo : InvalidData: (:) [Get-WebListenerUrl], ParameterBindingValidationEx
ception
+ FullyQualifiedErrorId : ParameterArgumentValidationError,Get-WebListenerUrl
test/tools/WebListener/README.md
Outdated
There was a problem hiding this comment.
I think we could add that the page can be used as default for some test as we do in HTTPS tests already.
There was a problem hiding this comment.
heh. I had a blurb about that but removed it thinking it was too wordy (I have a tendency to be overly verbose). I'll add it back.
|
@markekraus Sorry for my inconsiderateness today. Thanks for the great work! |
|
@iSazonov No problem! I was making a lot of tiny changes in a huge context. I got lost a few times making these changes. I suspected it might make for a tough review. |
|
It seems WebListener.psm1 is stable now and follow PRs will be small. 😄 |
|
@iSazonov It actually will change again. I plan on adding a |
There was a problem hiding this comment.
shouldn't the be localhost... not just match?
There was a problem hiding this comment.
it actually returns localhost:<portnumber> but, I didn't want to couple the port number, which may change, with the individual tests. If 8083 and 8084 will be the permanent ports, then this can be changed.
There was a problem hiding this comment.
I guess it could be
$result.Output.headers.Host | Should Be $uri.Authority348a5ca to
09efb78
Compare
|
AppVeyor fail is unrelated (#4762) |
|
@markekraus The test is now pending - so please rebase to pass CI. |
09efb78 to
a35adde
Compare
|
@iSazonov rebased and passing. |
|
@markekraus Thanks for very useful work! |
Some tests which use a loop still call httpbin.org. those will be moved when the other functions they rely on have been added. Also, A few of the tests has disabled
should's that can now be enabled.reference #2504