Fixed ScriptsToProcess when -Version param is used in Import-Module#3897
Conversation
| manifestInfo.AddDetectedCmdletExport(detectedCmdlet); | ||
| } | ||
| bool found = false; | ||
| PSModuleInfo module = LoadModule(scriptFile, |
There was a problem hiding this comment.
Can you use named parameters. Example: https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/named-and-optional-arguments
There was a problem hiding this comment.
That is actually existing code; I didn't change it; looks like Github's diff'er has room for improvement :) - see picture below.
Anyway, there are multiple LoadModule calls like this one throughout the file, so if refactoring is to be done - it has to be done to all of them - and that is out of scope of this bugfix.
| } | ||
| foreach (string detectedCmdlet in module.ExportedCmdlets.Keys) | ||
| { | ||
| manifestInfo.AddDetectedCmdletExport(detectedCmdlet); |
There was a problem hiding this comment.
AddDetected* methods from PSModuleInfo in most cases seem to be called multiple times using a for loop. Will it make sense to add an overload which accepts IEnumerable?
There was a problem hiding this comment.
Maybe. But as previous comment - this would be refactoring of unrelated code - out of scope of this bug fix.
| Import-Module .\module2.psd1 -Version 1.0 | ||
| Get-Content out.txt | Should Be '21' | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Add new line at the end.
|
|
||
| AfterAll { | ||
| Pop-Location | ||
| #Remove-Item $moduleRootPath -Recurse -Force -ErrorAction SilentlyContinue |
There was a problem hiding this comment.
Remove commented line.
| Remove-Item out.txt -Force -ErrorAction SilentlyContinue | ||
| } | ||
|
|
||
| It "Verify ScriptsToProcess are executed for top-level module" { |
There was a problem hiding this comment.
You may want to take a look at TestCases parameter of Pester It.
There was a problem hiding this comment.
Thanks for suggestion; I did give it a try. I believe that semantic difference between these tests is larger than what TestCases parameter is aimed for; plus I don't like that TestCases parameter has worse logging (same test name repeated several times vs individual, descriptive test names in case of individual 'It's).
There was a problem hiding this comment.
You can have custom test case names for individual test cases. Have look at: https://github.com/PowerShell/PowerShell/blob/02b5f357a20e6dee9f8e60e3adb9025be3c94490/test/powershell/engine/ParameterBinding/NullableBooleanDCR.Tests.ps1
Here the parameters will be parameter splat for Import-Module, and expected result, test name.
There was a problem hiding this comment.
That is actually cool; thank you!
I've updated tests.
| BeforeAll { | ||
| $moduleRootPath = Join-Path $TestDrive 'TestModules' | ||
| New-Item $moduleRootPath -ItemType Directory -Force | Out-Null | ||
| Push-Location $moduleRootPath |
There was a problem hiding this comment.
Why do you need to push location? Can you use full paths for all cmdlets like New-Item, New-ModuleManifest etc?
There was a problem hiding this comment.
I chose push location specifically because I didn't want to deal with full paths. :)
| Remove-Item out.txt -Force -ErrorAction SilentlyContinue | ||
| } | ||
|
|
||
| It "Verify ScriptsToProcess are executed for top-level module" { |
There was a problem hiding this comment.
You can have custom test case names for individual test cases. Have look at: https://github.com/PowerShell/PowerShell/blob/02b5f357a20e6dee9f8e60e3adb9025be3c94490/test/powershell/engine/ParameterBinding/NullableBooleanDCR.Tests.ps1
Here the parameters will be parameter splat for Import-Module, and expected result, test name.
This is fix for #3738 "ScriptsToProcess not honored when Import-Module is passed -Version param".
The bug was cased by existing check in LoadModule functions:
When loading usual module files, this check is valid;
However, same LoadModule function is used to load ScriptsToProcess - and this when this check was
triggered and ScriptsToProcess were not executed.
The fix is to work around that condition for ScriptsToProcess in the same way it is currently done for NestedModules - temporary reset the version info for the duration of processing ScriptsToProcess / NestedModules.
Test results before the fix:
Test results after the fix:
