-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Fix PSVersion in PSSessionConfiguration tests #5554
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix PSVersion in PSSessionConfiguration tests #5554
Conversation
89e8a2d to
594e17a
Compare
| $Result.Session.Name | Should be $TestSessionConfigName | ||
| $Result.Session.SessionType | Should be "Default" | ||
| $Result.Session.PSVersion | Should be 6.0 | ||
| $Result.Session.PSVersion | Should BeExactly $expectedPSVersion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still not convinced changing this from a string literal to the variable is the right move. Please provide answers to the question in the other PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests should work on all PowerShell Core versions without changes.
We don't pass-through the version constant, we evaluate it. So we should have a contract for this in the tests.
|
|
||
| $Result.Session.Name | Should be $TestSessionConfigName | ||
| $Result.Session.PSVersion | Should be 6.0 | ||
| $Result.Session.PSVersion | Should BeExactly $expectedPSVersion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
| $Result.Session.UseSharedProcess | Should be $UseSharedProcess | ||
| } | ||
|
|
||
| It "Validate Set-PSSessionConfiguration -PSVersion" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is very similar to the test at L504. Can you use -testcases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine to keep it as is. This Context is for Set-PSSessionConfiguration and the one above is in the Context of Register-PSSessionConfiguration.
|
|
||
| $LocalConfigFilePath = CreateTestConfigFile | ||
|
|
||
| $expectedPSVersion = "$($PSVersionTable.PSVersion.Major)`.$($PSVersionTable.PSVersion.Minor)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`.
The backtick is not necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
| Unregister-PSSessionConfiguration -Name $TestSessionConfigName -Force -NoServiceRestart -ErrorAction SilentlyContinue | ||
| } | ||
|
|
||
| $expectedPSVersion = "$($PSVersionTable.PSVersion.Major)`.$($PSVersionTable.PSVersion.Minor)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be moved to the BeforeAll block of the Describe, otherwise the use of $expectedPSVersion below from a different Context block won't see the variable.
And the backtick is not necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
|
|
||
| $Session.Name | Should be $TestSessionConfigName | ||
| $Session.PSVersion | Should Be 5.1 | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix the indentation of this it block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
| $Result.Session.UseSharedProcess | Should be $UseSharedProcess | ||
| } | ||
|
|
||
| It "Validate Set-PSSessionConfiguration -PSVersion" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine to keep it as is. This Context is for Set-PSSessionConfiguration and the one above is in the Context of Register-PSSessionConfiguration.
|
@iSazonov Please add the |
|
I will add the |
2bab0cd to
1ef532a
Compare
|
|
||
| It "Validate Set-PSSessionConfiguration -PSVersion" { | ||
|
|
||
| Set-PSSessionConfiguration -Name $TestSessionConfigName -PSVerion 5.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be -PSVersion (missing an s)
|
Restarted job on Travis-CI as one test failed due to intermittent failure. |
|
Filed #5567 for travis-ci test failure |
|
Not taking for GA because this was caused by a change that is only in |
Check that we can register and change (Set-) remote endpoint
configurations with given PSVersion.