Skip to content

Fix module specification hashtable in ModuleSpecification.Tests.ps1 #7663

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
adityapatwardhan merged 2 commits into from
Sep 4, 2018
Merged

Fix module specification hashtable in ModuleSpecification.Tests.ps1 #7663

adityapatwardhan merged 2 commits into from
Sep 4, 2018

Conversation

@sethvs
Copy link
Contributor

@sethvs sethvs commented Aug 30, 2018

PR Summary

Change Name to ModuleName in module specification hashtable.

PR Checklist

@iSazonov
Copy link
Collaborator

iSazonov commented Aug 30, 2018

@sethvs The broken names was created specially for test "Cannot create from invalid module hashtables".

@sethvs
Copy link
Contributor Author

sethvs commented Aug 30, 2018

@iSazonov
I believe this is not the case.
Each testcase tests for some specific error in hashtable.

@{TestName = "Version+RequiredVersion"
ModuleSpecification = @{ ModuleName = "BadVersionModule"; ModuleVersion = "3.1"; RequiredVersion = "3.1" }},
@{TestName = "NoName"
ModuleSpecification = @{ ModuleVersion = "0.2" }},
@{TestName = "BadField"
ModuleSpecification = @{ ModuleName = "StrangeFieldModule"; RequiredVersion = "7.4"; Duck = "1.2" }},
@{TestName = "BadType"
ModuleSpecification = @{ ModuleName = "BadTypeModule"; RequiredVersion = "Hello!" }

Now, with Name instead of ModuleName, the code that checks the hashtable throws errors because of the wrong field - Name, instead of the actual error, that should be tested.

For example, the first testcase:

$ModuleSpecification = @{ Name = "BadVersionModule"; ModuleVersion = "3.1"; RequiredVersion = "3.1" }
[Microsoft.PowerShell.Commands.ModuleSpecification]::new($ModuleSpecification)
Exception calling ".ctor" with "1" argument(s): "The hashtable describing a module contains one or more members that are not valid.  The valid members are (ModuleName, ModuleVersion, RequiredVersion, GUID). Remove the members that are not valid ('Name'), then try again."
At line:1 char:1
+ [Microsoft.PowerShell.Commands.ModuleSpecification]::new($ModuleSpeci ...
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo          : NotSpecified: (:) [], MethodInvocationException
+ FullyQualifiedErrorId : ArgumentException

should actually be:

$ModuleSpecification = @{ ModuleName = "BadVersionModule"; ModuleVersion = "3.1"; RequiredVersion = "3.1" }
[Microsoft.PowerShell.Commands.ModuleSpecification]::new($ModuleSpecification)
Exception calling ".ctor" with "1" argument(s): "The parameters ModuleVersion and RequiredVersion cannot be used together. Please specify only one parameter."
At line:1 char:1
+ [Microsoft.PowerShell.Commands.ModuleSpecification]::new($ModuleSpeci ...
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo          : NotSpecified: (:) [], MethodInvocationException
+ FullyQualifiedErrorId : ArgumentException

adityapatwardhan
adityapatwardhan requested changes Aug 30, 2018
View reviewed changes
test/powershell/engine/Module/ModuleSpecification.Tests.ps1
@{
TestName = "BadType"
ModuleSpecification = @{ Name = "BadTypeModule"; RequiredVersion = "Hello!" }
ModuleSpecification = @{ ModuleName = "BadTypeModule"; RequiredVersion = "Hello!" }
Copy link
Member

@adityapatwardhan adityapatwardhan Aug 30, 2018

Choose a reason for hiding this comment

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

To make the tests more effective we modify L255 to be something like below for the specific expected failure. This will ensure such failures are not hidden due to faulty test code.

Should -Throw -ErrorId ArgumentException

Copy link
Contributor Author

@sethvs sethvs Aug 31, 2018

Choose a reason for hiding this comment

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

Regarding first and third tests:

@{TestName = "Version+RequiredVersion"
ModuleSpecification = @{ ModuleName = "BadVersionModule"; ModuleVersion = "3.1"; RequiredVersion = "3.1" }}

@{TestName = "BadField"
ModuleSpecification = @{ ModuleName = "StrangeFieldModule"; RequiredVersion = "7.4"; Duck = "1.2" }}

They return ArgumentException, just like when we use Name instead of ModuleName in the hastable, but I agree, that specifying expected ErrorId is a good addition.

Copy link
Contributor Author

@sethvs sethvs Aug 31, 2018

Choose a reason for hiding this comment

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

Done.

@adityapatwardhan
Copy link
Member

adityapatwardhan commented Aug 30, 2018

@sethvs Thanks for your contribution. I have left a comment to make the test even more effective.

rjmholt
rjmholt approved these changes Aug 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.

Looks good. Thanks for fixing this. @adityapatwardhan's suggestion is quite right -- a more specific test around the error would have prevented this problem, so it would be good to include here

@adityapatwardhan adityapatwardhan merged commit ccd2e53 into PowerShell:master Sep 4, 2018
TravisEz13 pushed a commit that referenced this pull request Sep 5, 2018
@sethvs @TravisEz13
Fix module specification hashtable in ModuleSpecification.Tests.ps1 (#…
3e06138 
…7663)
@TravisEz13 TravisEz13 added this to the v6.1.0 milestone Sep 7, 2018
@sethvs sethvs deleted the moduleSpecification.Tests branch September 27, 2018 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adityapatwardhan adityapatwardhan adityapatwardhan approved these changes

@iSazonov iSazonov iSazonov approved these changes

+2 more reviewers

@rjmholt rjmholt rjmholt approved these changes

@anmenaga anmenaga anmenaga approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@adityapatwardhan adityapatwardhan

Labels

None yet

Projects

None yet

Milestone

v6.1.0

Development

Successfully merging this pull request may close these issues.

6 participants

@sethvs @iSazonov @adityapatwardhan @rjmholt @anmenaga @TravisEz13