Fixed Test-Modulemanifest to normalize paths correctly before validating #3097

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

lzybkr · 2017-02-07T06:48:22Z

test/powershell/engine/Module/TestModuleManifest.ps1

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

lzybkr · 2017-02-07T06:49:37Z

src/System.Management.Automation/engine/Modules/TestModuleManifestCommand.cs

+                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?

@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

SteveL-MSFT

@lzybkr are you ok with the updated changes?

iSazonov · 2017-02-15T14:27:05Z

test/powershell/engine/Module/TestModuleManifest.ps1

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

iSazonov · 2017-02-15T14:28:45Z

test/powershell/engine/Module/TestModuleManifest.ps1

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

iSazonov · 2017-02-15T14:29:33Z

test/powershell/engine/Module/TestModuleManifest.ps1

+
+        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

iSazonov · 2017-02-15T14:33:28Z

test/powershell/engine/Module/TestModuleManifest.ps1

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


Typo "parametersAndExecptions" <- "parametersAndErrors"

iSazonov · 2017-02-15T15:10:22Z

test/powershell/engine/Module/TestModuleManifest.ps1

+            }
+            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

iSazonov · 2017-02-15T15:12:17Z

test/powershell/engine/Module/TestModuleManifest.ps1

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


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

SteveL-MSFT · 2017-02-16T07:55:36Z

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

iSazonov · 2017-02-22T03:43:34Z

@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

SteveL-MSFT · 2017-02-22T23:01:39Z

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

iSazonov · 2017-02-23T16:22:52Z

test/powershell/engine/Module/TestModuleManifest.ps1

@@ -37,22 +38,17 @@ Describe "Test-ModuleManifest tests" -tags "CI" {
            "ModuleList"="Modules_InvalidModuleListinModuleManifest";
            "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.

iSazonov · 2017-02-23T16:24:32Z

test/powershell/engine/Module/TestModuleManifest.ps1

            "ScriptsToProcess"="Modules_InvalidManifest"
        }
-        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. @JamesWTruher suggestions?

I suppose we should use TestCases

iSazonov · 2017-02-23T16:28:12Z

test/powershell/engine/Module/TestModuleManifest.ps1

-            }
+            [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

iSazonov · 2017-02-24T13:50:07Z

test/powershell/engine/Module/TestModuleManifest.ps1

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

iSazonov · 2017-02-24T13:51:36Z

test/powershell/engine/Module/TestModuleManifest.ps1

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

iSazonov · 2017-02-24T13:52:25Z

test/powershell/engine/Module/TestModuleManifest.ps1

-            { 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?

iSazonov · 2017-02-24T13:53:16Z

test/powershell/engine/Module/TestModuleManifest.ps1

+        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

iSazonov · 2017-02-24T13:53:40Z

test/powershell/engine/Module/TestModuleManifest.ps1

+            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

iSazonov · 2017-02-24T19:29:34Z

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

So the tests LGTM.

lzybkr · 2017-02-27T01:26:16Z

src/System.Management.Automation/engine/Modules/TestModuleManifestCommand.cs

@@ -131,6 +131,22 @@ protected override void ProcessRecord()
                            }
                        }

+                        //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 @HemantMahawar

@lzybkr addressed

added tests updated InvalidModuleManifest error message to be about module manifests

lzybkr · 2017-02-28T22:23:50Z

test/powershell/engine/Module/TestModuleManifest.ps1

+        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

lzybkr · 2017-03-01T03:29:02Z

Closing and re-opening to retry blocked CI.

fixing PowerShell#2610

ea329fc

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

msftclas added the cla-not-required label Feb 4, 2017

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

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

SteveL-MSFT commented Feb 9, 2017

View reviewed changes

iSazonov reviewed Feb 15, 2017

View reviewed changes

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

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

iSazonov reviewed Feb 24, 2017

View reviewed changes

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

c915ff0

…r manifest creation

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

lzybkr reviewed Feb 28, 2017

View reviewed changes

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

lzybkr approved these changes Feb 28, 2017

View reviewed changes

lzybkr closed this Mar 1, 2017

lzybkr reopened this Mar 1, 2017

msftclas added the cla-not-required label Mar 1, 2017

lzybkr merged commit 99d696f into PowerShell:master Mar 1, 2017

SteveL-MSFT deleted the modulemanifest branch March 21, 2017 19:53

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed Test-Modulemanifest to normalize paths correctly before validating #3097

Fixed Test-Modulemanifest to normalize paths correctly before validating #3097


		Test-Path $testModulePath \| Should Be $true

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

Fixed Test-Modulemanifest to normalize paths correctly before validating #3097

Fixed Test-Modulemanifest to normalize paths correctly before validating #3097

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment