Fixed Test-Modulemanifest to normalize paths correctly before validating by SteveL-MSFT · Pull Request #3097 · PowerShell/PowerShell

Steve Lee (SteveL-MSFT) · 2017-02-04T03:22:06Z

changed hard coded Windows directory separator and resolved path so the slashes are correct
added test
addresses #2610

changed hard coded Windows directory separator and resolved path so the slashes are correct added test

Jason Shirk (lzybkr) · 2017-02-07T06:48:22Z

+        "" > testdrive:/module/foo/bar.psm1
+        "" > testdrive:/module/bar/foo.psm1
+        New-ModuleManifest -NestedModules foo\bar.psm1,bar/foo.psm1 -RootModule foo\bar.psm1 -RequiredAssemblies foo/bar.psm1,bar\foo.psm1 -Path testdrive:/module/test.psd1
+        Test-ModuleManifest -Path testdrive:/module/test.psd1 -ErrorAction Stop | Should BeOfType System.Management.Automation.PSModuleInfo


Do you want some additional validation on the paths, maybe a Test-Path on the NestedModules, RootModule, and RequiredAssemblies.

There are other module properties that specify files, maybe you should be testing those too - TypesToProcess, FormatsToProcess, ScriptsToProcess, maybe FileList and ModuleList?

I wonder if it's a bad idea to specify RequiredAssemblies without a valid assembly - if that's not already causing problems, I can see it causing problems in the future.

The semantics of test-modulemanifest seems to only verify existence of files and not if they are valid. Rather than verify the files exist and being validated, I'm adding a negative test where test-modulemanifest should fail if any paths don't exist

Jason Shirk (lzybkr) · 2017-02-07T06:49:37Z

+                CmdletProviderContext cmdContext = new CmdletProviderContext(this);
+                Collection<PathInfo> pathInfos = SessionState.Path.GetResolvedPSPathFromPSPath(path, cmdContext);
+
+                foreach (PathInfo pathInfo in pathInfos)


If you resolve to multiple files, shouldn't that be an error instead of just ignoring it?

Jason Shirk (@lzybkr) Since wildcards are explicitly being checked and not allowed in the module manifest, is a Debug.Assert sufficient?

looking at other code in this file, decided to throw

Possible downside here is I'm adding blocks that can't be hit

added negative test cases that do file path checks

Steve Lee (SteveL-MSFT)

Jason Shirk (@lzybkr) are you ok with the updated changes?

Ilya (iSazonov) · 2017-02-15T14:27:05Z

+        New-Item -ItemType Directory -Path testdrive:/module/foo
+        New-Item -ItemType Directory -Path testdrive:/module/bar
+        "" > testdrive:/module/foo/bar.psm1
+        "" > testdrive:/module/bar/foo.psm1


Maybe better use Set-Content ? This is more clearly.

Agree. Fixed.

Ilya (iSazonov) · 2017-02-15T14:28:45Z

+
+        Test-Path $testModulePath | Should Be $true
+
+        Test-ModuleManifest -Path $testModulePath -ErrorAction Stop | Should BeOfType System.Management.Automation.PSModuleInfo


Why we use -ErrorAction Stop ? It is not a "throw" test. Better -ErrorAction SilentlyContinue.

I used ErrorAction Stop here since Test-ModuleManifest writes to the error stream on manifest errors and I thought this was an easy way to ensure no errors being returned. Otherwise I think I'd have to redirect the error stream and check it and I don't think that's providing additional value. I can add a comment to clarify my intent.

Clear.
Closed.

Ilya (iSazonov) · 2017-02-15T14:29:33Z

+
+        New-Item -ItemType Directory -Path testdrive:/module
+        New-Item -ItemType Directory -Path testdrive:/module/foo
+        "" > testdrive:/module/foo/bar.psm1


The same about Set-Content

Ilya (iSazonov) · 2017-02-15T14:33:28Z

+            "RootModule"="Modules_InvalidManifest";
+            "ScriptsToProcess"="Modules_InvalidManifest"
+        }
+        foreach ($parameter in $parametersAndExecptions.Keys) {


Typo "parametersAndExecptions" <- "parametersAndErrors"

Ilya (iSazonov) · 2017-02-15T15:10:22Z

+            }
+            catch {
+                $_.FullQulaifiedErrorId | Should Match "$errorId"
+            }


Why Should BeOfType System.Management.Automation.PSModuleInfo ? If we expect that Test-ModuleManifest raise an exception in the tests the Should BeOfType will be never run.

I suggest using the excellent function ShouldBeErrorId
(I have already prepared new PR #3161 to move existing tests to this pattern)
The new look of this test:

{ Test-ModuleManifest -Path $testModulePath -ErrorAction Stop } | ShouldBeErrorId $errorId

Only insert:

Import-Module $PSScriptRoot\..\..\Common\Test.Helpers.psm1

Cool! will make this change

Ilya (iSazonov) · 2017-02-15T15:12:17Z

+Describe "Test-ModuleManifest tests" -tags "CI" {
+
+    AfterEach {
+        if (Test-Path testdrive:/module) {


It looks as unneeded check: Remove-Item implicitly do the check.

Steve Lee (SteveL-MSFT) · 2017-02-16T07:55:36Z

Ilya (@iSazonov) thanks for the feedback, I'll take care of these after I get back from vacation on 2/21

Ilya (iSazonov) · 2017-02-22T03:43:34Z

Steve Lee (@SteveL-MSFT) I don't see new commit related with your last comments.

… readable fixed typo in test that exposed issue in cmdlet for validating nestedmodules

Steve Lee (SteveL-MSFT) · 2017-02-22T23:01:39Z

Ilya (@iSazonov) took longer than I expected, after I fixed the typo, I found a issue with the cmdlet

Ilya (iSazonov) · 2017-02-23T16:22:52Z

            "TypesToProcess"="Modules_InvalidManifest";
            "FormatsToProcess"="Modules_InvalidManifest";
-            "RootModule"="Modules_InvalidManifest";
+            #"RootModule"="Modules_InvalidManifest";


Maybe remove it?

Should be uncommented. Looks like the cmdlets doesn't do any validation of RootModule currently, will need to add that code.

Ilya (iSazonov) · 2017-02-23T16:24:32Z

        }
-        foreach ($parameter in $parametersAndExecptions.Keys) {
+        foreach ($parameter in $parametersAndErrors.Keys) {
+            Write-Warning "Testing $parameter"


Not sure what the best practice is here. When doing the test in a loop and something fails, the current error from Pester isn't sufficient to tell you where it failed. James Truher (@JamesWTruher) suggestions?

I suppose we should use TestCases

Ilya (iSazonov) · 2017-02-23T16:28:12Z

+            [string]$errorId = $parametersAndErrors[$parameter] + ",Microsoft.PowerShell.Commands.TestModuleManifestCommand"
+
+            { Test-ModuleManifest -Path $testModulePath -ErrorAction Stop } | ShouldBeErrorId $errorId
            Remove-Item -Recurse -Force $testModulePath


Remove? It seems AfterEach does cleanups.

This is needed as I'm generating a new invalid manifest each iteration of the loop

used TestCases feature of Pester for tests added tests for valid and invalid rootmodules

Ilya (iSazonov) · 2017-02-24T13:50:07Z

-            "ScriptsToProcess"="Modules_InvalidManifest"
+        $args = @{$parameter = "doesnotexist.psm1"}
+        New-ModuleManifest -Path $testModulePath @args
+        Test-Path $testModulePath | Should Be $true


Below ShouldBeErrorId still catches this error. Maybe remove the line?

Yeah, this isn't needed, originally I thought to check that the manifest was actually created, but it would fail later. Will remove.

Ilya (iSazonov) · 2017-02-24T13:51:36Z

+        New-Item -ItemType Directory -Path testdrive:/module
+        $testModulePath = "testdrive:/module/test.psd1"
+
+        if ($rootModuleValue -ne $null -and $rootModuleValue -ne "")


It seems it is better to us to split the tests into two and remove the conditions.

Was perhaps over optimizing on code reuse. I'll split it to make it more readable.

Ilya (iSazonov) · 2017-02-24T13:52:25Z

-            { Test-ModuleManifest -Path $testModulePath -ErrorAction Stop } | ShouldBeErrorId $errorId
-            Remove-Item -Recurse -Force $testModulePath
+        New-ModuleManifest -Path $testModulePath -RootModule $rootModuleValue
+        Test-Path $testModulePath | Should Be $true


Maybe remove too?

Ilya (iSazonov) · 2017-02-24T13:53:16Z

+        New-Item -ItemType Directory -Path testdrive:/module
+        $testModulePath = "testdrive:/module/test.psd1"
+
+        if ($rootModuleValue -ne "doesnotexist.psm1")


It seems it is better to us to split the tests into two and remove the conditions.

I'm ok with splitting

Ilya (iSazonov) · 2017-02-24T13:53:40Z

+            New-Item -ItemType File -Path testdrive:/module/$rootModuleValue
+        }
+        New-ModuleManifest -Path $testModulePath -RootModule $rootModuleValue
+        Test-Path $testModulePath | Should Be $true


Maybe remove too?

…r manifest creation

Ilya (iSazonov) · 2017-02-24T19:29:34Z

I see multiple New-Item but I see no reason optimize them (BeforeAll).

So the tests LGTM.

Jason Shirk (lzybkr) · 2017-02-27T01:26:16Z

                            }
                        }

+                        //RootModule can be null, empty string or point to a valid .psm1, or .dll.  Anything else is invalid.


cdxml and xaml modules are allowed in RootModule just as they are allowed in NestedModules.

Also, when comparing extensions, is ignoring the case correct for non-Windows?

This file contains OrdinalIgnoreCase 4 times. But I see lots of other files to ignore the case when dealing with paths.

Yes, case insensitive comparisons on executable extensions could be a bigger issue than just this file.

I think the extension comparison issue should probably be a separate issue so we can apply same fix everywhere

Created issue #3218

I agree. We don't even have the desired method. Perhaps even better, make a request in .Net.

In testing a fix for cdxml/XAML modules, I see that you get this error message for InvalidModuleManifest which just seems wrong:

PowerShell/src/System.Management.Automation/resources/Modules.resx

Line 157 in 309bf60

<value>The module manifest '{0}' could not be processed because it is not a valid Windows PowerShell restricted language file. Remove the elements that are not permitted by the restricted language:

<value>The module manifest '{0}' could not be processed because it is not a valid Windows PowerShell restricted language file. Remove the elements that are not permitted by the restricted language: {1}</value>

I'd like to change it to something like:

The module manifest '{0}' could not be processed because it is not a valid PowerShell module manifest file. Remove the elements that are not permitted: {1}

CC Hemant Mahawar (@HemantMahawar)

Jason Shirk (@lzybkr) addressed

added tests updated InvalidModuleManifest error message to be about module manifests

Jason Shirk (lzybkr) · 2017-02-28T22:23:50Z

+        Test-ModuleManifest -Path $testModulePath -ErrorAction Stop | Should BeOfType System.Management.Automation.PSModuleInfo
+    }
+
+    It "module manifest containing missing files returns error" -TestCases `


Typical use of -TestCases is to introduce a variable - that is probably easier to read.

If there is a good reason to inline the argument - you should prefer parens instead of a backtick.
Extra indentation on the test cases might also make it more readable, so maybe format like:

It "should work" -TestCases ( @{rootModuleValue = "bar.psm1"}, @{rootModuleValue = "bar.dll"} ) { param($rootModuleValue) }

will change

added tests updated InvalidModuleManifest error message to be about module manifests changed -TestCases usage to parens than backtick for readability

…to modulemanifest

Jason Shirk (lzybkr) · 2017-03-01T03:29:02Z

Closing and re-opening to retry blocked CI.

PowerShell#3097) Changed hard coded Windows directory separator and resolved path so the slashes are correct. Throw if resolving file path returns more than one result Fixes PowerShell#2610

fixing PowerShell#2610

ea329fc

changed hard coded Windows directory separator and resolved path so the slashes are correct added test

Microsoft Contribution License Agreements (msftclas) added the cla-not-required label Feb 4, 2017

Steve Lee (SteveL-MSFT) mentioned this pull request Feb 4, 2017

Test-ModuleManifest should not check everything incl .dll, xml, resouces files #2610

Closed

PowerShellTeam added the Review - Needed The PR is being reviewed label Feb 4, 2017

Jason Shirk (lzybkr) reviewed Feb 7, 2017

View reviewed changes

throw if resolving file path returns more than one result

5657bfd

added negative test cases that do file path checks

Steve Lee (SteveL-MSFT) commented Feb 9, 2017

View reviewed changes

Ilya (iSazonov) reviewed Feb 15, 2017

View reviewed changes

Jason Shirk (lzybkr) self-assigned this Feb 16, 2017

changes to tests based on review feedback to make it simpler and more…

6d3a83c

… readable fixed typo in test that exposed issue in cmdlet for validating nestedmodules

Ilya (iSazonov) reviewed Feb 23, 2017

View reviewed changes

fixed issue with validation of rootmodule

9ba7895

used TestCases feature of Pester for tests added tests for valid and invalid rootmodules

Ilya (iSazonov) reviewed Feb 24, 2017

View reviewed changes

split tests to make them more readable, removed unnecessary checks fo…

c915ff0

…r manifest creation

Jason Shirk (lzybkr) suggested changes Feb 27, 2017

View reviewed changes

added check for cdxml and xaml rootmodules which are allowed

ae5ab4c

added tests updated InvalidModuleManifest error message to be about module manifests

Jason Shirk (lzybkr) reviewed Feb 28, 2017

View reviewed changes

Steve Lee (SteveL-MSFT) added 2 commits February 28, 2017 14:28

added check for cdxml and xaml rootmodules which are allowed

cb5ca17

added tests updated InvalidModuleManifest error message to be about module manifests changed -TestCases usage to parens than backtick for readability

Merge branch 'modulemanifest' of github.com:SteveL-MSFT/PowerShell in…

029b954

…to modulemanifest

Jason Shirk (lzybkr) approved these changes Feb 28, 2017

View reviewed changes

Jason Shirk (lzybkr) closed this Mar 1, 2017

Jason Shirk (lzybkr) reopened this Mar 1, 2017

Microsoft Contribution License Agreements (msftclas) added the cla-not-required label Mar 1, 2017

Jason Shirk (lzybkr) merged commit 99d696f into PowerShell:master Mar 1, 2017

Steve Lee (SteveL-MSFT) deleted the modulemanifest branch March 21, 2017 19:53

Ilya (iSazonov) removed the Review - Needed The PR is being reviewed label Mar 27, 2017


		Test-Path $testModulePath \| Should Be $true

		Test-ModuleManifest -Path $testModulePath -ErrorAction Stop \| Should BeOfType System.Management.Automation.PSModuleInfo

Uh oh!

Conversation

Steve Lee (SteveL-MSFT) commented Feb 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Steve Lee (SteveL-MSFT) Feb 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Steve Lee (SteveL-MSFT) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) Feb 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Steve Lee (SteveL-MSFT) commented Feb 16, 2017

Uh oh!

Ilya (iSazonov) commented Feb 22, 2017

Uh oh!

Steve Lee (SteveL-MSFT) commented Feb 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Steve Lee (SteveL-MSFT) Feb 7, 2017 •

edited

Loading

Ilya (iSazonov) Feb 15, 2017 •

edited

Loading