GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Fix PSVersion in PSSessionConfiguration tests by iSazonov · Pull Request #5554 · PowerShell/PowerShell

Conversation

@iSazonov

@iSazonov Ilya (iSazonov) commented Nov 27, 2017

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@iSazonov
Fix PSVersion in PSSessionConfiguration tests
594e17a
@iSazonov
Ilya (iSazonov) force-pushed the fix-pssessionconfiguration-tests branch from 89e8a2d to 594e17a Compare November 27, 2017 06:55
@iSazonov Ilya (iSazonov) mentioned this pull request Nov 27, 2017
Increase version to 6.1 for PSSessionConfiguration tests #5542
Closed
@iSazonov Ilya (iSazonov) assigned Aditya Patwardhan (adityapatwardhan) Nov 27, 2017
markekraus
Mark Kraus (markekraus) previously requested changes Nov 27, 2017
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Core/PSSessionConfiguration.Tests.ps1
$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

@markekraus Mark Kraus (markekraus) Nov 27, 2017

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Nov 27, 2017

Copy link
Copy Markdown
Collaborator Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Core/PSSessionConfiguration.Tests.ps1

$Result.Session.Name | Should be $TestSessionConfigName
$Result.Session.PSVersion | Should be 6.0
$Result.Session.PSVersion | Should BeExactly $expectedPSVersion

@markekraus Mark Kraus (markekraus) Nov 27, 2017

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@markekraus
Mark Kraus (markekraus) dismissed their stale review November 27, 2017 13:01

.

adityapatwardhan
Aditya Patwardhan (adityapatwardhan) requested changes Nov 27, 2017
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Core/PSSessionConfiguration.Tests.ps1
$Result.Session.UseSharedProcess | Should be $UseSharedProcess
}

It "Validate Set-PSSessionConfiguration -PSVersion" {

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Nov 27, 2017

Copy link
Copy Markdown
Member

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Nov 28, 2017

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@markekraus Mark Kraus (markekraus) mentioned this pull request Nov 27, 2017
Remove hard coded expected version string #5556
Closed
@adityapatwardhan Aditya Patwardhan (adityapatwardhan) mentioned this pull request Nov 28, 2017
Update test to construct the version from $PSVersionTable.PSVersion #5557
Closed
daxian-dbw
Dongbo Wang (daxian-dbw) requested changes Nov 28, 2017
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Core/PSSessionConfiguration.Tests.ps1 Outdated

$LocalConfigFilePath = CreateTestConfigFile

$expectedPSVersion = "$($PSVersionTable.PSVersion.Major)`.$($PSVersionTable.PSVersion.Minor)"

@daxian-dbw Dongbo Wang (daxian-dbw) Nov 28, 2017

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Nov 28, 2017

Copy link
Copy Markdown
Collaborator Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Core/PSSessionConfiguration.Tests.ps1 Outdated
Unregister-PSSessionConfiguration -Name $TestSessionConfigName -Force -NoServiceRestart -ErrorAction SilentlyContinue
}

$expectedPSVersion = "$($PSVersionTable.PSVersion.Major)`.$($PSVersionTable.PSVersion.Minor)"

@daxian-dbw Dongbo Wang (daxian-dbw) Nov 28, 2017

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Nov 28, 2017

Copy link
Copy Markdown
Collaborator Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Core/PSSessionConfiguration.Tests.ps1 Outdated

$Session.Name | Should be $TestSessionConfigName
$Session.PSVersion | Should Be 5.1
}

@daxian-dbw Dongbo Wang (daxian-dbw) Nov 28, 2017

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Nov 28, 2017

Copy link
Copy Markdown
Collaborator Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Core/PSSessionConfiguration.Tests.ps1
$Result.Session.UseSharedProcess | Should be $UseSharedProcess
}

It "Validate Set-PSSessionConfiguration -PSVersion" {

@daxian-dbw Dongbo Wang (daxian-dbw) Nov 28, 2017

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@markekraus Mark Kraus (markekraus) mentioned this pull request Nov 28, 2017
Replace HttpListener Response Tests with WebListener #5540
Merged
daxian-dbw
Dongbo Wang (daxian-dbw) approved these changes Nov 28, 2017
View reviewed changes
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Nov 28, 2017

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Nov 28, 2017

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov @daxian-dbw
[Feature] Remove backtick. Use BeExactly Fix indentation.
1ef532a
@daxian-dbw
Dongbo Wang (daxian-dbw) force-pushed the fix-pssessionconfiguration-tests branch from 2bab0cd to 1ef532a Compare November 28, 2017 17:57
adityapatwardhan
Aditya Patwardhan (adityapatwardhan) approved these changes Nov 28, 2017
View reviewed changes
markekraus
Mark Kraus (markekraus) suggested changes Nov 28, 2017
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Core/PSSessionConfiguration.Tests.ps1 Outdated

It "Validate Set-PSSessionConfiguration -PSVersion" {

Set-PSSessionConfiguration -Name $TestSessionConfigName -PSVerion 5.1

@markekraus Mark Kraus (markekraus) Nov 28, 2017

Copy link
Copy Markdown
Contributor

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@iSazonov
[Feature] Fix typo.
6b9299c
markekraus
Mark Kraus (markekraus) approved these changes Nov 28, 2017
View reviewed changes
@adityapatwardhan

Aditya Patwardhan (adityapatwardhan) commented Nov 28, 2017

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@TravisEz13

Travis Plunk (TravisEz13) commented Nov 28, 2017

Copy link
Copy Markdown
Member

Filed #5567 for travis-ci test failure
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@adityapatwardhan
Aditya Patwardhan (adityapatwardhan) merged commit 910c5a4 into PowerShell:master Nov 28, 2017
@TravisEz13 Travis Plunk (TravisEz13) added this to the 6.1.0-MQ milestone Nov 29, 2017
@TravisEz13

Travis Plunk (TravisEz13) commented Nov 29, 2017

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov
Ilya (iSazonov) deleted the fix-pssessionconfiguration-tests branch November 29, 2017 04:00
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@iSazonov @adityapatwardhan
Fix PSVersion in PSSessionConfiguration tests (PowerShell#5554)
fafb504 
* Fix PSVersion in PSSessionConfiguration tests by getting the expected value from PSVersionTable instead of hardcoded string.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daxian-dbw Dongbo Wang (daxian-dbw) daxian-dbw approved these changes
@adityapatwardhan Aditya Patwardhan (adityapatwardhan) adityapatwardhan approved these changes
+1 more reviewer
@markekraus Mark Kraus (markekraus) markekraus approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@adityapatwardhan Aditya Patwardhan (adityapatwardhan)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@iSazonov @daxian-dbw @adityapatwardhan @TravisEz13 @markekraus
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.