Test-ModuleManifest should not load unnecessary modules by chunqingchen · Pull Request #4541 · PowerShell/PowerShell

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

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

Setup should go in BeforeAll block.

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

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

Cleanup should go in AfterAll block.

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

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

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

Setup should go in BeforeAll

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

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

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

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

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

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

Same here, please remove all comments.

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

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

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

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

Please fix indentation

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

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

Do not need \Modules, Modules should be sufficient

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

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

Add end of line.

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

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

Add end of line.

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

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

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

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

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

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

typo frefresh -> refresh

resolved, thank you

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

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

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

This should be FunctionsToExport = @()

resolved, thanks

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

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

This should be CmdletsToExport = @()

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

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

This should be AliasesToExport = @()

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

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

Same comments as the other module.

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

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

Should be -ErrorAction SilentlyContinue

resolved, thanks

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

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

Should be -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, TravisEz13, lzybkr and vors as code owners August 15, 2017 21:00

chunqingchen removed request for BrucePay, JamesWTruher, TravisEz13, lzybkr and vors 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

		@@ -0,0 +1 @@
		function Get-NestedRequiredModule1 { Get-Date } No newline at end of file

Conversation

chunqingchen commented Aug 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw left a comment

Choose a reason for hiding this comment

Uh oh!

chunqingchen commented Aug 11, 2017

Uh oh!

daxian-dbw commented Aug 11, 2017

Uh oh!

daxian-dbw 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!

Choose a reason for hiding this comment

Uh oh!

chunqingchen Aug 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!

chunqingchen Aug 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!

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

chunqingchen Aug 15, 2017 •

edited

Loading

chunqingchen Aug 15, 2017 •

edited

Loading