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

Fix issue #4533

Summary of the issue:

Test-ModuleManifest is loading unnecessary modules

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

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

Please fix the change in TestModuleManifestCommand.cs

Dongbo Wang (@daxian-dbw) code file format fixed

New commit was pushed.

Chunqing Chen (@chunqingchen) Thanks! Will continue the review.

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.

Setup should go in BeforeAll block.

resolved

Cleanup should go in AfterAll block.

resolved

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.

resolved

Setup should go in BeforeAll

resolved

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

resolved

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

resolved

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.

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

resolved

Same here, please remove all comments.

resolved

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

deleted

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.

Please fix indentation

resolved

Do not need \Modules, Modules should be sufficient

Add end of line.

Join-path "$pshome\Modules"

Does this call to Join-Path even work?

resolved

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.

resolved

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.

ditto

resolved

Dongbo Wang (@daxian-dbw) Aditya Patwardhan (@adityapatwardhan) your comments are resolved

typo frefresh -> refresh

resolved, thank you

resolved

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?

This should be FunctionsToExport = @()

resolved, thanks

resolved

This should be CmdletsToExport = @()

resolved

This should be AliasesToExport = @()

resolved

Same comments as the other module.

resolved

Should be -ErrorAction SilentlyContinue

resolved, thanks

Should be -ErrorAction SilentlyContinue

resolved

missing new line at end of file.

Comments addressed.

)

Chunqing Chen (chunqingchen) requested a review from Dongbo Wang (daxian-dbw) August 10, 2017 01:18

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

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

Dongbo Wang (daxian-dbw) reviewed Aug 10, 2017

View reviewed changes

Dongbo Wang (daxian-dbw) previously requested changes Aug 10, 2017

View reviewed changes

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

Dongbo Wang (daxian-dbw) requested a review from Aditya Patwardhan (adityapatwardhan) August 11, 2017 14:55

Dongbo Wang (daxian-dbw) requested changes Aug 12, 2017

View reviewed changes

Aditya Patwardhan (adityapatwardhan) requested changes Aug 15, 2017

View reviewed changes

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

Dongbo Wang (daxian-dbw) reviewed Aug 15, 2017

View reviewed changes

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

Chunqing Chen (chunqingchen) requested review from Bruce Payette (BrucePay), James Truher (JamesWTruher), Travis Plunk (TravisEz13), Jason Shirk (lzybkr) and sergei (vors) as code owners August 15, 2017 21:00

Chunqing Chen (chunqingchen) removed request for Bruce Payette (BrucePay), James Truher (JamesWTruher), Travis Plunk (TravisEz13), Jason Shirk (lzybkr) and sergei (vors) August 15, 2017 21:01

Aditya Patwardhan (adityapatwardhan) previously requested changes Aug 15, 2017

View reviewed changes

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

Test-ModuleManifest should not load unnessary modules

69a630b

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

Travis Plunk (TravisEz13) reviewed Aug 25, 2017

View reviewed changes

Dongbo Wang (daxian-dbw) added 2 commits August 28, 2017 14:24

Address some remaining comments

0eda079

Add a new line at end

b523ce8

Dongbo Wang (daxian-dbw) approved these changes Aug 28, 2017

View reviewed changes

Dongbo Wang (daxian-dbw) self-assigned this Aug 28, 2017

Dongbo Wang (daxian-dbw) merged commit fe51fb7 into PowerShell:master Aug 28, 2017

Dongbo Wang (daxian-dbw) changed the title Test-ModuleManifest should not load unnessary modules Test-ModuleManifest should not load unnecessary modules Aug 28, 2017

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj/PowerShell that referenced this pull request Aug 6, 2026

Make 'Test-ModuleManifest' not load unnecessary modules (PowerShell#4541

3d8e691

)

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

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

Uh oh!

Test-ModuleManifest should not load unnecessary modules - #4541

Test-ModuleManifest should not load unnecessary modules#4541Dongbo Wang (daxian-dbw) merged 3 commits intoPowerShell:masterPowerShell/PowerShell:masterfrom chunqingchen:bugfix2chunqingchen/PowerShell:bugfix2Copy head branch name to clipboard

Conversation

Chunqing Chen (chunqingchen) commented Aug 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dongbo Wang (daxian-dbw) left a comment

Choose a reason for hiding this comment

Uh oh!

Chunqing Chen (chunqingchen) commented Aug 11, 2017

Uh oh!

Dongbo Wang (daxian-dbw) commented Aug 11, 2017

Uh oh!

Dongbo Wang (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!

Chunqing Chen (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!

Chunqing Chen (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

Test-ModuleManifest should not load unnecessary modules#4541
Dongbo Wang (daxian-dbw) merged 3 commits into
PowerShell:masterfrom
chunqingchen:bugfix2

Chunqing Chen (chunqingchen) Aug 15, 2017 •
edited
Loading

Chunqing Chen (chunqingchen) Aug 15, 2017 •
edited
Loading