Test-ModuleManifest should not load unnecessary modules #4541

chunqingchen · 2017-08-10T01:18:34Z

Fix issue #4533

Summary of the issue:

Test-ModuleManifest is loading unnecessary modules

Fix:
Change the recursive loading flag from 'true' to 'false'

daxian-dbw · 2017-08-10T18:39:30Z

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

-/********************************************************************++
-Copyright (c) Microsoft Corporation.  All rights reserved.
--********************************************************************/
-


Something is wrong with your change in this file. The diff is messed up.

daxian-dbw

Please fix the change in TestModuleManifestCommand.cs

chunqingchen · 2017-08-11T03:09:24Z

@daxian-dbw code file format fixed

New commit was pushed.

daxian-dbw · 2017-08-11T14:55:26Z

@chunqingchen Thanks! Will continue the review.

daxian-dbw

The test assert folder testmodulerunspace should be moved to engine\Module\assets folder. Create assets folder if it doesn't exist.
All test assets should follow this pattern, take a look at the test\powershell\engine\Help\assets folder as an example.

daxian-dbw · 2017-08-11T23:46:32Z

test/powershell/engine/Module/TestModuleManifest.Tests.ps1

+
+Describe "Test-ModuleManifest Performance bug followup" -tags "CI" {
+
+$TestModulesFolder= 'testmodulerunspace'


Setup should go in BeforeAll block.

daxian-dbw · 2017-08-11T23:47:56Z

test/powershell/engine/Module/TestModuleManifest.Tests.ps1

+
+#clen up the test module
+Remove-Item $UserModulesPath\ModuleWithDependencies2 -Recurse -Force -ErrorAction Ignore
+Remove-Item $UserModulesPath\NestedRequiredModule1 -Recurse -Force -ErrorAction Ignore


Cleanup should go in AfterAll block.

daxian-dbw · 2017-08-11T23:50:32Z

test/powershell/engine/Module/TestModuleManifest.Tests.ps1

+
+$TestModulesFolder= 'testmodulerunspace'
+$TestModulesPath = Join-path $PSScriptRoot $TestModulesFolder
+$script:UserModulesPath = Join-path [System.AppDomain]::CurrentDomain.BaseDirectory '\Modules'


This is not UserModulePath. If you want to use the Module folder that's next to powershell.exe, then replace ...currentdomain.basedirectory with $pshome.

And also, there is no need to use $script: for this variable.

daxian-dbw · 2017-08-11T23:51:30Z

test/powershell/engine/Module/TestModuleManifest.Tests.ps1

+
+
+# Install the Test Module
+Copy-Item $TestModulesPath\* $UserModulesPath -Recurse -Force


Setup should go in BeforeAll

daxian-dbw · 2017-08-11T23:56:25Z

test/powershell/engine/Module/TestModuleManifest.Tests.ps1

+        catch
+        {
+            throw $_
+        }


this catch doesn't seem necessary -- if it just rethrows the exception, why would you catch it?

daxian-dbw · 2017-08-11T23:57:32Z

test/powershell/engine/Module/TestModuleManifest.Tests.ps1

+            throw $_
+        }
+
+        $verbose = $job.ChildJobs[0].Verbose.ReadAll()


Don't you need to wait for the job to finish to read all verbose message?

daxian-dbw · 2017-08-11T23:59:41Z

test/powershell/engine/Module/TestModuleManifest.Tests.ps1

+        }
+
+        $verbose = $job.ChildJobs[0].Verbose.ReadAll()
+        $verbose.Count -le 50 | should be true


What does the number 50 mean here? Why do you think -le 50 means it doesn't load unnecessary modules?

Prior to the fix test-modulemanifest will load all the modules in the $pshome, creating a far more big verbose list.

50 is a random small number that small enough to make sure the issue doesn't repro.

It can't be fixed as well since the verbose message contains other information that may vary.

I've changed the number to an even smaller number 15.

daxian-dbw · 2017-08-12T00:02:08Z

...ll/engine/Module/testmodulerunspace/ModuleWithDependencies2/2.0/ModuleWithDependencies2.psd1

+# Generated by: manikb
+#
+# Generated on: 10/29/2015
+#


Can you please remove all comments in this file? They make it hard to see what fields are actually used.

daxian-dbw · 2017-08-12T00:02:28Z

...rshell/engine/Module/testmodulerunspace/NestedRequiredModule1/2.5/NestedRequiredModule1.psd1

+#
+# Generated on: 4/14/2015
+#
+


Same here, please remove all comments.

daxian-dbw · 2017-08-12T00:04:41Z

...ll/engine/Module/testmodulerunspace/ModuleWithDependencies2/2.0/ModuleWithDependencies2.psm1

@@ -0,0 +1 @@
+function Get-ModuleWithDependencies2 { Get-Date }


I don't see this file used in ModuleWithDependencies2.psd1, is it really needed?

adityapatwardhan · 2017-08-11T18:29:34Z

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

@@ -216,7 +216,7 @@ protected override void ProcessRecord()
                        {
                            foreach (ModuleSpecification moduleListModule in moduleListModules)
                            {
-                                var modules = GetModule(new[] { moduleListModule.Name }, true, true);
+                                var modules = GetModule(new[] { moduleListModule.Name }, false, true);


Use named parameters.

this is a sync fix of the bug fix under windows bug 7980238.
so I would prefer to keep the code the way the original fix was

I think they should be made named parameters (as well as the other GetModule call in the same file). We don't make any changes to the original fix when re-submitting it for the sake of less churn. Now when making changes to the Github code base, we want the changes to follow best practices.

The comment regarding named parameters is not resolved yet.

adityapatwardhan · 2017-08-11T18:30:16Z

test/powershell/engine/Module/TestModuleManifest.Tests.ps1

@@ -217,3 +217,37 @@ Describe "Tests for circular references in required modules" -tags "CI" {
        { TestImportModule $false $false $true } | ShouldBeErrorId "Modules_InvalidManifest,Microsoft.PowerShell.Commands.ImportModuleCommand"
    }
 }
+
+Describe "Test-ModuleManifest Performance bug followup" -tags "CI" {


Please fix indentation

adityapatwardhan · 2017-08-11T18:32:00Z

test/powershell/engine/Module/TestModuleManifest.Tests.ps1

+
+$TestModulesFolder= 'testmodulerunspace'
+$TestModulesPath = Join-path $PSScriptRoot $TestModulesFolder
+$script:UserModulesPath = Join-path [System.AppDomain]::CurrentDomain.BaseDirectory '\Modules'


Do not need \Modules, Modules should be sufficient

adityapatwardhan · 2017-08-15T17:29:49Z

...rshell/engine/Module/testmodulerunspace/NestedRequiredModule1/2.5/NestedRequiredModule1.psm1

@@ -0,0 +1 @@
+function Get-NestedRequiredModule1 { Get-Date }


Add end of line.

adityapatwardhan · 2017-08-15T17:30:00Z

...ll/engine/Module/testmodulerunspace/ModuleWithDependencies2/2.0/ModuleWithDependencies2.psm1

@@ -0,0 +1 @@
+function Get-ModuleWithDependencies2 { Get-Date }


Add end of line.

daxian-dbw · 2017-08-15T17:59:13Z

test/powershell/engine/Module/TestModuleManifest.Tests.ps1

+    BeforeAll {
+        $TestModulesFolder= 'testmodulerunspace'
+        $TestModulesPath = Join-path "$PSScriptRoot\assets" $TestModulesFolder
+        $UserModulesPath = Join-path "$pshome\Modules"


Join-path "$pshome\Modules"

Does this call to Join-Path even work?

daxian-dbw · 2017-08-15T18:12:27Z

test/powershell/engine/Module/TestModuleManifest.Tests.ps1

+        $job = start-job -name "job1" -ScriptBlock {test-modulemanifest "$using:UserModulesPath\ModuleWithDependencies2\2.0\ModuleWithDependencies2.psd1" -verbose} | Wait-Job
+
+        $verbose = $job.ChildJobs[0].Verbose.ReadAll()
+        $verbose.Count -le 15 | should be true


You should use BeLessThan here. See https://github.com/pester/Pester/wiki/Should#belessthan

And please put your rational about using the number '15' in comments here, so that people look at this code later will understand it.

daxian-dbw · 2017-08-15T18:15:11Z

...ne/Module/assets/testmodulerunspace/ModuleWithDependencies2/2.0/ModuleWithDependencies2.psd1

+# PowerShellHostName = ''
+
+# Minimum version of the Windows PowerShell host required by this module
+# PowerShellHostVersion = ''


There are so many comments in this file that you haven't deleted yet.

i thought you mean the comments added in addition to the auto generated ones. resolved.

daxian-dbw · 2017-08-15T18:15:23Z

...engine/Module/assets/testmodulerunspace/NestedRequiredModule1/2.5/NestedRequiredModule1.psd1

+# PowerShellHostName = ''
+
+# Minimum version of the Windows PowerShell host required by this module
+# PowerShellHostVersion = ''


chunqingchen · 2017-08-15T21:03:26Z

@daxian-dbw @adityapatwardhan your comments are resolved

adityapatwardhan · 2017-08-15T23:23:49Z

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

@@ -183,7 +183,7 @@ protected override void ProcessRecord()
                        {
                            foreach (ModuleSpecification requiredModule in requiredModules)
                            {
-                                var modules = GetModule(new[] { requiredModule.Name }, false, true);
+                                var modules = GetModule(new[] { requiredModule.Name }, all: false, frefresh: true);


typo frefresh -> refresh

resolved, thank you

adityapatwardhan · 2017-08-15T23:28:51Z

test/powershell/engine/Module/TestModuleManifest.Tests.ps1

+Describe "Test-ModuleManifest Performance bug followup" -tags "CI" {
+    BeforeAll {
+        $TestModulesFolder= 'testmodulerunspace'
+        $TestModulesPath = Join-path "$PSScriptRoot\assets" $TestModulesFolder


This can be

$TestModulesPath = Join-path -Path "$PSScriptRoot\assets" -ChildPath $TestModulesFolder -AdditionalChildPath 'testmodulerunspace'

I didn't get it. Any good besides it putting two lines into one? It also throws error once I copied it. Can we just skip this comment?

adityapatwardhan · 2017-08-15T23:32:21Z

...ne/Module/assets/testmodulerunspace/ModuleWithDependencies2/2.0/ModuleWithDependencies2.psd1

+NestedModules = @('NestedRequiredModule1') 
+
+# Functions to export from this module
+FunctionsToExport = '*'


This should be FunctionsToExport = @()

resolved, thanks

adityapatwardhan · 2017-08-15T23:32:34Z

...ne/Module/assets/testmodulerunspace/ModuleWithDependencies2/2.0/ModuleWithDependencies2.psd1

+FunctionsToExport = '*'
+
+# Cmdlets to export from this module
+CmdletsToExport = '*'


This should be CmdletsToExport = @()

adityapatwardhan · 2017-08-15T23:32:46Z

...ne/Module/assets/testmodulerunspace/ModuleWithDependencies2/2.0/ModuleWithDependencies2.psd1

+VariablesToExport = '*'
+
+# Aliases to export from this module
+AliasesToExport = '*'


This should be AliasesToExport = @()

adityapatwardhan · 2017-08-15T23:33:24Z

...engine/Module/assets/testmodulerunspace/NestedRequiredModule1/2.5/NestedRequiredModule1.psd1

+NestedModules = @('NestedRequiredModule1.psm1')
+
+# Functions to export from this module
+FunctionsToExport = '*'


Same comments as the other module.

adityapatwardhan · 2017-08-15T23:37:23Z

test/powershell/engine/Module/TestModuleManifest.Tests.ps1

+
+    AfterAll {
+        #clean up the test module
+        Remove-Item $UserModulesPath\ModuleWithDependencies2 -Recurse -Force -ErrorAction Ignore


Should be -ErrorAction SilentlyContinue

resolved, thanks

adityapatwardhan · 2017-08-15T23:37:28Z

test/powershell/engine/Module/TestModuleManifest.Tests.ps1

+    AfterAll {
+        #clean up the test module
+        Remove-Item $UserModulesPath\ModuleWithDependencies2 -Recurse -Force -ErrorAction Ignore
+        Remove-Item $UserModulesPath\NestedRequiredModule1 -Recurse -Force -ErrorAction Ignore


Should be 10000 -ErrorAction SilentlyContinue

TravisEz13 · 2017-08-25T17:33:09Z

...engine/Module/assets/testmodulerunspace/NestedRequiredModule1/2.5/NestedRequiredModule1.psm1

@@ -0,0 +1 @@
+function Get-NestedRequiredModule1 { Get-Date }


missing new line at end of file.

Comments addressed.

chunqingchen requested a review from daxian-dbw August 10, 2017 01:18

msftclas added the cla-not-required label Aug 10, 2017

chunqingchen force-pushed the bugfix2 branch 3 times, most recently from 69f192f to 5b61238 Compare August 10, 2017 05:30

daxian-dbw reviewed Aug 10, 2017

View reviewed changes

daxian-dbw previously requested changes Aug 10, 2017

View reviewed changes

chunqingchen force-pushed the bugfix2 branch from 5b61238 to 413b755 Compare August 10, 2017 19:16

daxian-dbw requested a review from adityapatwardhan August 11, 2017 14:55

daxian-dbw requested changes Aug 12, 2017

View reviewed changes

adityapatwardhan requested changes Aug 15, 2017

View reviewed changes

chunqingchen force-pushed the bugfix2 branch from 413b755 to 46d8738 Compare August 15, 2017 17:40

daxian-dbw reviewed Aug 15, 2017

View reviewed changes

chunqingchen force-pushed the bugfix2 branch 2 times, most recently from 45d680f to ab26a46 Compare August 15, 2017 21:00

chunqingchen requested review from BrucePay, JamesWTruher, lzybkr, TravisEz13 and vors as code owners August 15, 2017 21:00

chunqingchen removed request for vors, lzybkr, BrucePay, JamesWTruher and TravisEz13 August 15, 2017 21:01

adityapatwardhan previously requested changes Aug 15, 2017

View reviewed changes

chunqingchen force-pushed the bugfix2 branch from ab26a46 to 1126f8d Compare August 25, 2017 09:19

Test-ModuleManifest should not load unnessary modules

69a630b

chunqingchen force-pushed the bugfix2 branch from 1126f8d to 69a630b Compare August 25, 2017 11:39

TravisEz13 reviewed Aug 25, 2017

View reviewed changes

daxian-dbw added 2 commits August 28, 2017 14:24

Address some remaining comments

0eda079

Add a new line at end

b523ce8

daxian-dbw approved these changes Aug 28, 2017

View reviewed changes

daxian-dbw self-assigned this Aug 28, 2017

daxian-dbw merged commit fe51fb7 into PowerShell:master Aug 28, 2017

daxian-dbw changed the title ~~Test-ModuleManifest should not load unnessary modules~~ Test-ModuleManifest should not load unnecessary modules Aug 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test-ModuleManifest should not load unnecessary modules #4541

Test-ModuleManifest should not load unnecessary modules #4541


		Describe "Test-ModuleManifest Performance bug followup" -tags "CI" {

		$TestModulesFolder= 'testmodulerunspace'



		# Install the Test Module
		Copy-Item $TestModulesPath\* $UserModulesPath -Recurse -Force

		@@ -0,0 +1 @@
		function Get-ModuleWithDependencies2 { Get-Date }

		@@ -0,0 +1 @@
		function Get-NestedRequiredModule1 { Get-Date }

Test-ModuleManifest should not load unnecessary modules #4541

Test-ModuleManifest should not load unnecessary modules #4541

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

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