GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Fix module specification hashtable in ModuleSpecification.Tests.ps1 by sethvs · Pull Request #7663 · PowerShell/PowerShell

Conversation

Sergey Vasin (sethvs) commented Aug 30, 2018

Copy link

Copy Markdown

Contributor

PR Summary

Change Name to ModuleName in module specification hashtable.

PR Checklist

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
Change is not breaking
Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.
User-facing changes
- Not Applicable
- OR
- User-facing Documentation needed
  - Issue filed - Issue link:
Testing - New and feature
- Not Applicable or can only be tested interactively
- OR
- Make sure you've added a new test if existing tests do not effectively test the code changed
  - Add [feature] if the change is significant or affects feature tests

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

iSazonov reacted with thumbs up emoji

All reactions

👍 1 reaction


          Fix module specification hashtable in ModuleSpecification.Tests.ps1.

264fcd4

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".

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Ilya (iSazonov) requested review from Aditya Patwardhan (adityapatwardhan) and Rob Holt (rjmholt)

August 30, 2018 13:11

Ilya (iSazonov) approved these changes

Aug 30, 2018

View reviewed changes

Ilya (iSazonov) assigned Aditya Patwardhan (adityapatwardhan)

Aug 30, 2018

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!" }

Aditya Patwardhan (adityapatwardhan) Aug 30, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Sergey Vasin (sethvs) Aug 31, 2018

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Sergey Vasin (sethvs) Aug 31, 2018

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Done.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Rob Holt (rjmholt) approved these changes

Aug 30, 2018

View reviewed changes

Rob Holt (rjmholt) left a comment

Copy link

Copy Markdown

Collaborator

There was a problem hiding this 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. 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Add ErrorId.

34a5f3a

Andrew (anmenaga) approved these changes

Aug 31, 2018

View reviewed changes

Aditya Patwardhan (adityapatwardhan) approved these changes

Sep 4, 2018

View reviewed changes

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


          Fix module specification hashtable in ModuleSpecification.Tests.ps1 (#…

3e06138

…7663)

Travis Plunk (TravisEz13) added this to the v6.1.0 milestone

Sep 7, 2018

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


          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


          Fix module specification hashtable in ModuleSpecification.Tests.ps1 (P…

3f95807

…owerShell#7663)

Reviewers

Aditya Patwardhan (adityapatwardhan)

adityapatwardhan approved these changes

Ilya (iSazonov)

iSazonov approved these changes

+2 more reviewers

Rob Holt (rjmholt)

rjmholt approved these changes

Andrew (anmenaga)

anmenaga approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

Aditya Patwardhan (adityapatwardhan)

Labels

None yet

Projects

None yet

Milestone

v6.1.0

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.