★ wanayoo — archive 1999 https://github.com/PowerShell/PowerShell/pull/8137Nouvelle recherche | Portail wanayoo
Skip to content

/PowerShell

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

add test coverage for additional Get-Module parameters #8137

Merged
merged 4 commits into from Nov 7, 2018

Conversation

Reviewers

@iSazonov iSazonov

@rjmholt rjmholt

@SteveL-MSFT SteveL-MSFT

@daxian-dbw daxian-dbw

@adityapatwardhan adityapatwardhan

Assignees

@iSazonov iSazonov

Labels
CL-Test
Projects
None yet
Milestone
No milestone
4 participants
@KevinMarquette @iSazonov @rjmholt @SteveL-MSFT
@KevinMarquette
Copy link
Contributor

commented Oct 28, 2018 •
edited

PR Summary

add test coverage for Get-Module parameters FullyQualifiedName ,PSEdition and Refresh

PR Checklist

@KevinMarquette KevinMarquette changed the title add test coverage for additional Get-Module parameters [WIP] add test coverage for additional Get-Module parameters Oct 28, 2018

@KevinMarquette
add test coverage for Get-Module
Loading status checks…
529264c

@KevinMarquette KevinMarquette force-pushed the loanDepot:kevinmarquette/get-module-test-coverage branch to 529264c Oct 28, 2018

@KevinMarquette KevinMarquette changed the title [WIP] add test coverage for additional Get-Module parameters add test coverage for additional Get-Module parameters Oct 28, 2018

@iSazonov iSazonov requested review from adityapatwardhan and rjmholt Oct 30, 2018

@rjmholt

rjmholt approved these changes Oct 30, 2018
View changes

Copy link
Member

left a comment

This looks like important coverage to me!

One thing that strikes me is the name of the file (which I know is pre-existing). The tests here all cover Get-Module -ListAvailable scenarios, where Get-Module scenarios are mostly covered in ModuleConstraint.Tests.ps1 and the CompatiblePSEditions checks are covered in CompatiblePSEditions.Tests.ps1 (different to the ones here, they don't filter by PSEdition explicitly but do the implicity manifest check for modules on the Windows PowerShell module path).

@rjmholt rjmholt requested a review from daxian-dbw Oct 30, 2018

@iSazonov

This comment has been minimized.

Sign in to view
Copy link
Collaborator

commented Nov 1, 2018

@KevinMarquette Please address @rjmholt feedback about file names.
@rjmholt

This comment has been minimized.

Sign in to view
Copy link
Member

commented Nov 1, 2018

Oh sorry, my feedback isn't terribly clear.

I would support changing the file name to something like Get-Module-ListAvailable.Tests.ps1 (theoretically, maybe it should be ListAvailable.Get-Module.Tests.ps1...?). Point is that Get-Module is tested in a whole bunch of places, and discoverability is a problem for our tests.
@iSazonov

This comment has been minimized.

Sign in to view
Copy link
Collaborator

commented Nov 2, 2018

@adityapatwardhan We need your help.
@SteveL-MSFT

SteveL-MSFT requested changes Nov 2, 2018
View changes

Show resolved Hide resolved test/powershell/Modules/Microsoft.PowerShell.Core/Get-Module.Tests.ps1 Outdated
Show resolved Hide resolved test/powershell/Modules/Microsoft.PowerShell.Core/Get-Module.Tests.ps1 Outdated
@SteveL-MSFT

This comment has been minimized.

Sign in to view
Copy link
Member

commented Nov 2, 2018

For the organization of test cases to make them discoverable, I think it's out of scope for this PR and should be a separate PR.

@KevinMarquette KevinMarquette force-pushed the loanDepot:kevinmarquette/get-module-test-coverage branch to 27fdb7b Nov 3, 2018

@KevinMarquette
whitespace
Loading status checks…
9e2ad33
@SteveL-MSFT

SteveL-MSFT approved these changes Nov 3, 2018
View changes

Copy link
Member

left a comment

LGTM
@iSazonov

iSazonov reviewed Nov 4, 2018
View changes

Show resolved Hide resolved test/powershell/Modules/Microsoft.PowerShell.Core/Get-Module.Tests.ps1 Outdated
Show resolved Hide resolved test/powershell/Modules/Microsoft.PowerShell.Core/Get-Module.Tests.ps1
Show resolved Hide resolved test/powershell/Modules/Microsoft.PowerShell.Core/Get-Module.Tests.ps1
Show resolved Hide resolved test/powershell/Modules/Microsoft.PowerShell.Core/Get-Module.Tests.ps1 Outdated
Kevin Marquette
test style corrections
Loading status checks…
eadd2a4

@iSazonov iSazonov self-assigned this Nov 7, 2018

@iSazonov iSazonov merged commit 75fa6af into PowerShell:master Nov 7, 2018
7 checks passed

7 checks passed

CodeFactor No issues found.
Details
PowerShell-CI-linux #PR-8137-20181107.01 succeeded
Details
PowerShell-CI-macos #PR-8137-20181107.01 succeeded
Details
PowerShell-CI-spelling #PR-8137-20181107.01 succeeded
Details
PowerShell-CI-windows #PR-8137-20181107.01 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla All CLA requirements met.
Details
@iSazonov

This comment has been minimized.

Sign in to view
Copy link
Collaborator

commented Nov 7, 2018

@KevinMarquette Thanks for your contribution!

@KevinMarquette KevinMarquette deleted the loanDepot:kevinmarquette/get-module-test-coverage branch Nov 8, 2018

@iSazonov iSazonov added the CL-Test label Jan 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.