Skip to content

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

Merged
Aditya Patwardhan (adityapatwardhan) merged 2 commits into
PowerShell:masterfrom
sethvs:moduleSpecification.Tests
Sep 4, 2018
Merged

Fix module specification hashtable in ModuleSpecification.Tests.ps1#7663
Aditya Patwardhan (adityapatwardhan) merged 2 commits into
PowerShell:masterfrom
sethvs:moduleSpecification.Tests

Conversation

@sethvs

@sethvs Sergey Vasin (sethvs) commented Aug 30, 2018

Copy link
Copy Markdown
Contributor

PR Summary

Change Name to ModuleName in module specification hashtable.

PR Checklist

@iSazonov

Ilya (iSazonov) commented Aug 30, 2018

Copy link
Copy Markdown
Collaborator

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

@sethvs

Sergey Vasin (sethvs) commented Aug 30, 2018

Copy link
Copy Markdown
Contributor Author

Ilya (@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
Aditya Patwardhan (adityapatwardhan) requested changes Aug 30, 2018
View reviewed changes
Comment thread test/powershell/engine/Module/ModuleSpecification.Tests.ps1
@{
TestName = "BadType"
ModuleSpecification = @{ Name = "BadTypeModule"; RequiredVersion = "Hello!" }
ModuleSpecification = @{ ModuleName = "BadTypeModule"; RequiredVersion = "Hello!" }

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Aug 30, 2018

Copy link
Copy Markdown
Member

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

@sethvs Sergey Vasin (sethvs) Aug 31, 2018

Copy link
Copy Markdown
Contributor Author

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.

@sethvs Sergey Vasin (sethvs) Aug 31, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

@adityapatwardhan

Aditya Patwardhan (adityapatwardhan) commented Aug 30, 2018

Copy link
Copy Markdown
Member

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

rjmholt
Rob Holt (rjmholt) approved these changes Aug 30, 2018
View reviewed changes

@rjmholt Rob Holt (rjmholt) left a comment

Copy link
Copy Markdown
Collaborator

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. Aditya Patwardhan (@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
Aditya Patwardhan (adityapatwardhan) merged commit ccd2e53 into PowerShell:master Sep 4, 2018
Travis Plunk (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 Travis Plunk (TravisEz13) added this to the v6.1.0 milestone Sep 7, 2018
@sethvs
Sergey Vasin (sethvs) deleted the moduleSpecification.Tests branch September 27, 2018 11:25
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@sethvs @adityapatwardhan
Fix module specification hashtable in ModuleSpecification.Tests.ps1 (P…
c05b976 
…owerShell#7663)
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@sethvs @TravisEz13
Fix module specification hashtable in ModuleSpecification.Tests.ps1 (P…
3f95807 
…owerShell#7663)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) adityapatwardhan approved these changes
@iSazonov Ilya (iSazonov) iSazonov approved these changes
+2 more reviewers
@rjmholt Rob Holt (rjmholt) rjmholt approved these changes
@anmenaga Andrew (anmenaga) anmenaga approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@adityapatwardhan Aditya Patwardhan (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