Join GitHub today
GitHub is home to over 36 million developers working together to host and review code, manage projects, and build software together.
Sign upFix module specification hashtable in ModuleSpecification.Tests.ps1 #7663
Conversation
This comment has been minimized.
This comment has been minimized.
|
@sethvs The broken names was created specially for test "Cannot create from invalid module hashtables". |
This comment has been minimized.
This comment has been minimized.
|
@iSazonov @{TestName = " 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)
should actually be: $ModuleSpecification = @{ ModuleName = "BadVersionModule"; ModuleVersion = "3.1"; RequiredVersion = "3.1" }
[Microsoft.PowerShell.Commands.ModuleSpecification]::new($ModuleSpecification)
|
iSazonov
requested review from
rjmholt and
adityapatwardhan
Aug 30, 2018
iSazonov
approved these changes
Aug 30, 2018
iSazonov
assigned
adityapatwardhan
Aug 30, 2018
adityapatwardhan
requested changes
Aug 30, 2018
| }, | ||
| @{ | ||
| TestName = "BadType" | ||
| ModuleSpecification = @{ Name = "BadTypeModule"; RequiredVersion = "Hello!" } | ||
| ModuleSpecification = @{ ModuleName = "BadTypeModule"; RequiredVersion = "Hello!" } |
This comment has been minimized.
This comment has been minimized.
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.
This comment has been minimized.
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@sethvs Thanks for your contribution. I have left a comment to make the test even more effective. |
rjmholt
approved these changes
Aug 30, 2018
|
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 |
sethvs commentedAug 30, 2018
PR Summary
Change Name to ModuleName in module specification hashtable.
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.[feature]if the change is significant or affects feature tests