Skip to content

add test coverage for additional Get-Module parameters #8137

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.

Sign up for GitHub

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

Jump to bottom
Merged
iSazonov merged 4 commits into from
Nov 7, 2018
Merged

add test coverage for additional Get-Module parameters #8137

iSazonov merged 4 commits into from
Nov 7, 2018

Conversation

@KevinMarquette
Copy link
Contributor

@KevinMarquette KevinMarquette commented Oct 28, 2018 •
edited
Loading

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 KevinMarquette force-pushed the kevinmarquette/get-module-test-coverage branch from 0712608 to 529264c Compare October 28, 2018 21:05
@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
rjmholt
rjmholt approved these changes Oct 30, 2018
View reviewed changes
Copy link
Collaborator

@rjmholt rjmholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 October 30, 2018 06:10
@iSazonov
Copy link
Collaborator

iSazonov commented Nov 1, 2018

@KevinMarquette Please address @rjmholt feedback about file names.

@rjmholt
Copy link
Collaborator

rjmholt 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
Copy link
Collaborator

iSazonov commented Nov 2, 2018

@adityapatwardhan We need your help.

SteveL-MSFT
SteveL-MSFT requested changes Nov 2, 2018
View reviewed changes
@SteveL-MSFT
Copy link
Member

SteveL-MSFT 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 kevinmarquette/get-module-test-coverage branch from 7f7add8 to 27fdb7b Compare November 3, 2018 18:50
SteveL-MSFT
SteveL-MSFT approved these changes Nov 3, 2018
View reviewed changes
Copy link
Member

@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

iSazonov
iSazonov reviewed Nov 4, 2018
View reviewed changes
@iSazonov iSazonov self-assigned this Nov 7, 2018
@iSazonov iSazonov merged commit 75fa6af into PowerShell:master Nov 7, 2018
@iSazonov
Copy link
Collaborator

iSazonov commented Nov 7, 2018

@KevinMarquette Thanks for your contribution!

@KevinMarquette KevinMarquette deleted the kevinmarquette/get-module-test-coverage branch November 8, 2018 05:13
@iSazonov iSazonov added the CL-Test Indicates that a PR should be marked as a test change in the Change Log label Jan 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov iSazonov iSazonov left review comments

@SteveL-MSFT SteveL-MSFT SteveL-MSFT approved these changes

@adityapatwardhan adityapatwardhan Awaiting requested review from adityapatwardhan

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw

+1 more reviewer

@rjmholt rjmholt rjmholt approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@iSazonov iSazonov

Labels

CL-Test Indicates that a PR should be marked as a test change in the Change Log

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@KevinMarquette @iSazonov @rjmholt @SteveL-MSFT