Powershell: Refactor ImportModule commandlet

[pougetat]

Refactor Import-Module part of the codebase

After speaking with @rjmholt I've learned that the Import-Module part of the codebase has gotten rather complex and needs to be refactored. However, due to numerous subtleties in how Import-Module works we need to achieve much better testing coverage of Import-Module before work can be done to refactor.

For some context the following PR is what is leading me to create this issue as I noticed some odd behavior which is described in the comments : https://github.com/PowerShell/PowerShell/pull/8758

I'm opening this meta issue to track ongoing work relating to this refactoring and hopefully it can lead to useful discussions. I will also use it to add my own insights, ideas and questions as I dive into it.

[rjmholt]

A few PRs in this vein:

• https://github.com/PowerShell/PowerShell/pull/7499
• https://github.com/PowerShell/PowerShell/pull/7140
• https://github.com/PowerShell/PowerShell/pull/7126
• https://github.com/PowerShell/PowerShell/pull/7125

Note an example of a regression caused by this refactoring, fixed in https://github.com/PowerShell/PowerShell/pull/8218.

[pougetat]

Hey @rjmholt, @iSazonov 😄 ,

A potential refactoring I've been thinking about has to to with LoadModuleManifest method within ModuleCmdletBase.cs
This method does essentially two things :

• Checks the manifest file properties.
• Loads the related modules included in the file.

Since we already have a TestModuleManifest.cs which uses LoadModuleManifest + flag disabling all the imports it might be interesting to move the checks to TestModuleManifest.cs (something along the lines of CheckManifestProperties) and have LoadModuleManifest call that method before carrying on with the imports.

This has the following benefits :

• Delegate module manifest checks to TestModuleManifest.cs so that the property checking can be "owned" by the file implementing the associated cmdlet.
• Reduce ModuleCmdletBase.cs size by 25% making it more readable and relieving it of some of its responsibilities improving overall readability.

This has the following potential issues :

• Module related cmdlets can have some subtle behavior and the current test base does not cover the cmdlets sufficiently to easily refactor without running a big risk of introducing unforeseen potential breaking changes.
• LoadModuleManifest intertwines simple property checks with imports of related modules and therefore taking into account the previous point we could very well be introducting breaking changes by following through with that refactoring idea.
  Currently the method does : TestSomeProperties => ImportModules => TestSomeMoreProperties
  And I am thinking of : TestAllProperties => ImportModules

Here are the steps to get to where we'd like to go if this strategy gets a green light :

• More tests for Remove-Module (very few tests exist for this one right now)
• More tests for Import-Module
• More tests for Test-ModuleManifest (This should be the one with the smallest risk of unexpected behavior)
• More tests for Get-Module
• Implement refactoring idea

One question I've had regarding the testing base in general is :

• How do we know we have sufficient coverage regarding a cmdlet (especially since for example import-module can have subtle behavior so it's tough to test for that behavior without knowing about it) ?

[iSazonov]

How do we know …

1. We should create tests for all practical scenarios
2. Analyze code and cover all paths
3. Use CodeCov (It seems still out of order /cc @adityapatwardhan)
4. Review opened issues for the code

[adityapatwardhan]

/cc @JamesWTruher is working on fixing CodeCov.