Skip to content

Conversation

@iSazonov
Copy link
Collaborator

Check that we can register and change (Set-) remote endpoint
configurations with given PSVersion.

$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
Copy link
Contributor

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.

Copy link
Collaborator Author

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
Copy link
Contributor

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" {
Copy link
Member

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?

Copy link
Member

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)"
Copy link
Member

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.

Copy link
Collaborator Author

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)"
Copy link
Member

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.

Copy link
Collaborator Author

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
}
Copy link
Member

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.

Copy link
Collaborator Author

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" {
Copy link
Member

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.

@daxian-dbw
Copy link
Member

@iSazonov Please add the [Feature] tag to the last commit message to verify whether the changes fix the test failure.

@daxian-dbw
Copy link
Member

I will add the [Feature] tag to the last commit to trigger the full test run.

@daxian-dbw daxian-dbw force-pushed the fix-pssessionconfiguration-tests branch from 2bab0cd to 1ef532a Compare November 28, 2017 17:57

It "Validate Set-PSSessionConfiguration -PSVersion" {

Set-PSSessionConfiguration -Name $TestSessionConfigName -PSVerion 5.1
Copy link
Contributor

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)

@adityapatwardhan
Copy link
Member

Restarted job on Travis-CI as one test failed due to intermittent failure.

@TravisEz13
Copy link
Member

Filed #5567 for travis-ci test failure

@adityapatwardhan adityapatwardhan merged commit 910c5a4 into PowerShell:master Nov 28, 2017
@TravisEz13 TravisEz13 added this to the 6.1.0-MQ milestone Nov 29, 2017
@TravisEz13
Copy link
Member

Not taking for GA because this was caused by a change that is only in master branch

@iSazonov iSazonov deleted the fix-pssessionconfiguration-tests branch November 29, 2017 04:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants