★ wanayoo — archive 1999 https://github.com/PowerShell/PowerShell/pull/7663Nouvelle recherche | Portail wanayoo
Skip to content
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

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

Merged

Conversation

Projects
None yet
6 participants
@sethvs
Copy link
Contributor

commented Aug 30, 2018

PR Summary

Change Name to ModuleName in module specification hashtable.

PR Checklist

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Aug 30, 2018

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

@sethvs

This comment has been minimized.

Copy link
Contributor Author

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

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

},
@{
TestName = "BadType"
ModuleSpecification = @{ Name = "BadTypeModule"; RequiredVersion = "Hello!" }
ModuleSpecification = @{ ModuleName = "BadTypeModule"; RequiredVersion = "Hello!" }

This comment has been minimized.

Copy link
@adityapatwardhan

adityapatwardhan Aug 30, 2018

Member

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

This comment has been minimized.

Copy link
@sethvs

sethvs Aug 31, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@sethvs

sethvs Aug 31, 2018

Author Contributor

Done.

@adityapatwardhan

This comment has been minimized.

Copy link
Member

commented Aug 30, 2018

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

@rjmholt
Copy link
Member

left a comment

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

8 checks passed

CodeFactor No issues found.
Details
PowerShell-CI-linux #PR-7663-20180831.01 succeeded with issues
Details
PowerShell-CI-macos #PR-7663-20180831.01 succeeded
Details
PowerShell-CI-windows #PR-7663-20180831.01 succeeded
Details
WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.

TravisEz13 added a commit that referenced this pull request Sep 5, 2018

@TravisEz13 TravisEz13 added this to the v6.1.0 milestone Sep 7, 2018

@sethvs sethvs deleted the sethvs:moduleSpecification.Tests branch Sep 27, 2018

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.