Version checking tests for Import-Module

[rjmholt]

PR Summary

Adds a number of tests to check the version of the module to import against constraints given to the cmdlet.

PR Checklist

• PR has a meaningful title
  • Use the present tense and imperative mood when describing your changes
• Summarized changes
• Change is not breaking
• Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
• This PR is ready to merge and is not Work in Progress.
  • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.
• User-facing changes
  • Not Applicable
  • OR
  • User-facing Documentation needed
    • Issue filed - Issue link:
• Testing - New and feature
  • Not Applicable or can only be tested interactively
  • OR
  • Make sure you've added a new test if existing tests do not effectively test the code changed
    • Add [feature] if the change is significant or affects feature tests

This adds testing for #7125

[rjmholt]

Have marked as WIP while I add GUID tests

[rjmholt]

Tests marked Pending are due to #7496 - they fail because of what I suspect is incorrect behaviour in PowerShell

[SteveL-MSFT]

I'll wait for some refactoring before reviewing in detail

[SteveL-MSFT]

This assert is already handled by line 189 so this doesn't need to be here

[SteveL-MSFT]

This is already handled by 217

[SteveL-MSFT]

This is handled by 244

[SteveL-MSFT]

Covered by 441

[SteveL-MSFT]

Covered by 469

[SteveL-MSFT]

Covered by 998

[SteveL-MSFT]

Covered by 1025

[SteveL-MSFT]

Covered by 1251

[SteveL-MSFT]

This validation code is very similar throughout this script. It seems you should have a Validate type function instead of the same code repeated

[SteveL-MSFT]

why return instead of else?

[rjmholt]

Given the choice I prefer if-return, because:

• The reader knows that they can stop reading when they see return
• Less indentation and less horizontal programming
• Closer to pattern match/guard style, which I find helps the "exhaust all possibilities" mentality
• The return works as a forcing function to not juggle state

I see many instances in the code base that I imagine initially started as innocent if-elses that could have been if-returns, but whose complexity has since increased due to that choice:

• PowerShell/src/System.Management.Automation/engine/Modules/ImportModuleCommand.cs

  Lines 1698 to 1783 in 01634e4

  	if (this.ParameterSetName.Equals(ParameterSet_ModuleInfo, StringComparison.OrdinalIgnoreCase))
  	{
  	// Process all of the specified PSModuleInfo objects. These would typically be coming in as a result
  	// of doing Get-Module -list
  	foreach (PSModuleInfo module in ModuleInfo)
  	{
  	RemoteDiscoveryHelper.DispatchModuleInfoProcessing(
  	module,
  	localAction: delegate ()
  	{
  	ImportModule_ViaLocalModuleInfo(importModuleOptions, module);
  	SetModuleBaseForEngineModules(module.Name, this.Context);
  	},
  	cimSessionAction: (cimSession, resourceUri, cimNamespace) => ImportModule_RemotelyViaCimSession(
  	importModuleOptions,
  	new string[] { module.Name },
  	cimSession,
  	resourceUri,
  	cimNamespace),
  	psSessionAction: psSession => ImportModule_RemotelyViaPsrpSession(
  	importModuleOptions,
  	new string[] { module.Path },
  	null,
  	psSession));
  	}
  	}
  	else if (this.ParameterSetName.Equals(ParameterSet_Assembly, StringComparison.OrdinalIgnoreCase))
  	{
  	// Now load all of the supplied assemblies...
  	if (Assembly != null)
  	{
  	foreach (Assembly suppliedAssembly in Assembly)
  	{
  	ImportModule_ViaAssembly(importModuleOptions, suppliedAssembly);
  	}
  	}
  	}
  	else if (this.ParameterSetName.Equals(ParameterSet_Name, StringComparison.OrdinalIgnoreCase))
  	{
  	foreach (string name in Name)
  	{
  	PSModuleInfo foundModule = ImportModule_LocallyViaName(importModuleOptions, name);
  	if (foundModule != null)
  	{
  	SetModuleBaseForEngineModules(foundModule.Name, this.Context);
  	#if LEGACYTELEMETRY
  	TelemetryAPI.ReportModuleLoad(foundModule);
  	#endif
  	}
  	}
  	}
  	else if (this.ParameterSetName.Equals(ParameterSet_ViaPsrpSession, StringComparison.OrdinalIgnoreCase))
  	{
  	ImportModule_RemotelyViaPsrpSession(importModuleOptions, this.Name, null, this.PSSession);
  	}
  	else if (this.ParameterSetName.Equals(ParameterSet_ViaCimSession, StringComparison.OrdinalIgnoreCase))
  	{
  	ImportModule_RemotelyViaCimSession(importModuleOptions, this.Name, this.CimSession, this.CimResourceUri, this.CimNamespace);
  	}
  	else if (this.ParameterSetName.Equals(ParameterSet_FQName, StringComparison.OrdinalIgnoreCase))
  	{
  	foreach (var modulespec in FullyQualifiedName)
  	{
  	RequiredVersion = modulespec.RequiredVersion;
  	MinimumVersion = modulespec.Version;
  	MaximumVersion = modulespec.MaximumVersion;
  	BaseGuid = modulespec.Guid;
  	PSModuleInfo foundModule = ImportModule_LocallyViaName(importModuleOptions, modulespec.Name);
  	if (foundModule != null)
  	{
  	SetModuleBaseForEngineModules(foundModule.Name, this.Context);
  	}
  	}
  	}
  	else if (this.ParameterSetName.Equals(ParameterSet_FQName_ViaPsrpSession, StringComparison.OrdinalIgnoreCase))
  	{
  	ImportModule_RemotelyViaPsrpSession(importModuleOptions, null, FullyQualifiedName, this.PSSession);
  	}
  	else
  	{
  	Dbg.Assert(false, "Unrecognized parameter set");
  	}

• PowerShell/src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs

  Lines 6351 to 6506 in 01634e4

  	if (assemblyToLoad != null)
  	{
  	// Figure out what to use for a module path...
  	if (!string.IsNullOrEmpty(fileName))
  	{
  	modulePath = fileName;
  	}
  	else
  	{
  	modulePath = assemblyToLoad.Location;
  	}
  	// And what to use for a module name...
  	if (string.IsNullOrEmpty(moduleName))
  	{
  	moduleName = "dynamic_code_module_" + assemblyToLoad.GetName();
  	}
  	if (importingModule)
  	{
  	// Passing module as a parameter here so that the providers can have the module property populated.
  	// For engine providers, the module should point to top-level module name
  	// For FileSystem, the module is Microsoft.PowerShell.Core and not System.Management.Automation
  	if (parentModule != null && InitialSessionState.IsEngineModule(parentModule.Name))
  	{
  	iss.ImportCmdletsFromAssembly(assemblyToLoad, parentModule);
  	}
  	else
  	{
  	iss.ImportCmdletsFromAssembly(assemblyToLoad, null);
  	}
  	}
  	assemblyVersion = GetAssemblyVersionNumber(assemblyToLoad);
  	assembly = assemblyToLoad;
  	// If this is an in-memory only assembly, add it directly to the assembly cache if
  	// it isn't already there.
  	if (string.IsNullOrEmpty(assembly.Location))
  	{
  	if (! Context.AssemblyCache.ContainsKey(assembly.FullName))
  	{
  	Context.AssemblyCache.Add(assembly.FullName, assembly);
  	}
  	}
  	}
  	else
  	{
  	// Avoid trying to import a PowerShell assembly as Snapin as it results in PSArgumentException
  	if ((moduleName != null) && Utils.IsPowerShellAssembly(moduleName))
  	{
  	trySnapInName = false;
  	}
  	if (trySnapInName && PSSnapInInfo.IsPSSnapinIdValid(moduleName))
  	{
  	PSSnapInInfo snapin = null;
  	#if !CORECLR
  	// Avoid trying to load SnapIns with Import-Module
  	PSSnapInException warning;
  	try
  	{
  	if (importingModule)
  	{
  	snapin = iss.ImportPSSnapIn(moduleName, out warning);
  	}
  	}
  	catch (PSArgumentException)
  	{
  	//BUGBUG - brucepay - probably want to have a verbose message here...
  	;
  	}
  	#endif
  	if (snapin != null)
  	{
  	importSuccessful = true;
  	if (string.IsNullOrEmpty(fileName))
  	modulePath = snapin.AbsoluteModulePath;
  	else
  	modulePath = fileName;
  	assemblyVersion = snapin.Version;
  	// If we're not supposed to load the types files from the snapin
  	// clear the iss member
  	if (!loadTypes)
  	{
  	iss.Types.Reset();
  	}
  	// If we're not supposed to load the format files from the snapin,
  	// clear the iss member
  	if (!loadFormats)
  	{
  	iss.Formats.Reset();
  	}
  	foreach (var a in ClrFacade.GetAssemblies())
  	{
  	if (a.GetName().FullName.Equals(snapin.AssemblyName, StringComparison.Ordinal))
  	{
  	assembly = a;
  	break;
  	}
  	}
  	}
  	}
  	if (importSuccessful == false)
  	{
  	if (importingModule)
  	{
  	assembly = Context.AddAssembly(moduleName, fileName, out error);
  	if (assembly == null)
  	{
  	if (error != null)
  	throw error;
  	found = false;
  	return null;
  	}
  	assemblyVersion = GetAssemblyVersionNumber(assembly);
  	if (string.IsNullOrEmpty(fileName))
  	modulePath = assembly.Location;
  	else
  	modulePath = fileName;
  	// Passing module as a parameter here so that the providers can have the module property populated.
  	// For engine providers, the module should point to top-level module name
  	// For FileSystem, the module is Microsoft.PowerShell.Core and not System.Management.Automation
  	if (parentModule != null && InitialSessionState.IsEngineModule(parentModule.Name))
  	{
  	iss.ImportCmdletsFromAssembly(assembly, parentModule);
  	}
  	else
  	{
  	iss.ImportCmdletsFromAssembly(assembly, null);
  	}
  	}
  	else
  	{
  	string binaryPath = fileName;
  	modulePath = fileName;
  	if (binaryPath == null)
  	{
  	binaryPath = System.IO.Path.Combine(moduleBase, moduleName);
  	}
  	BinaryAnalysisResult analysisResult = GetCmdletsFromBinaryModuleImplementation(binaryPath, manifestProcessingFlags, out assemblyVersion);
  	detectedCmdlets = analysisResult.DetectedCmdlets;
  	detectedAliases = analysisResult.DetectedAliases;
  	}
  	}
  	}

• PowerShell/src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs

  Lines 3646 to 3715 in 01634e4

  	Dbg.Assert(moduleName != null, "GetModuleSpecification should guarantee that moduleName != null");
  	foreach (PSModuleInfo module in context.Modules.GetModules(new string[] { "*" }, false))
  	{
  	if (moduleName.Equals(module.Name, StringComparison.OrdinalIgnoreCase))
  	{
  	if (!moduleGuid.HasValue || moduleGuid.Value.Equals(module.Guid))
  	{
  	if (requiredModule.RequiredVersion != null)
  	{
  	if (requiredModule.RequiredVersion.Equals(module.Version))
  	{
  	result = module;
  	loaded = true;
  	break;
  	}
  	wrongVersion = true;
  	}
  	else if (requiredModule.Version != null)
  	{
  	if (requiredModule.MaximumVersion != null)
  	{
  	if (GetMaximumVersion(requiredModule.MaximumVersion) >= module.Version && requiredModule.Version <= module.Version)
  	{
  	result = module;
  	loaded = true;
  	break;
  	}
  	else
  	{
  	wrongVersion = true;
  	}
  	}
  	else if (requiredModule.Version <= module.Version)
  	{
  	result = module;
  	loaded = true;
  	break;
  	}
  	else
  	{
  	wrongVersion = true;
  	}
  	}
  	else if (requiredModule.MaximumVersion != null)
  	{
  	if (GetMaximumVersion(requiredModule.MaximumVersion) >= module.Version)
  	{
  	result = module;
  	loaded = true;
  	break;
  	}
  	else
  	{
  	wrongVersion = true;
  	}
  	}
  	else
  	{
  	result = module;
  	loaded = true;
  	break;
  	}
  	}
  	else
  	{
  	wrongGuid = true;
  	}
  	}
  	}

• PowerShell/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs

  Lines 275 to 325 in c1c5344

  	if (x == null && y == null)
  	{
  	result = true;
  	}
  	else if (x != null && y != null)
  	{
  	if (x.Name != null && y.Name != null)
  	{
  	result = x.Name.Equals(y.Name, StringComparison.OrdinalIgnoreCase);
  	}
  	else
  	{
  	result = true;
  	}
  	if (result)
  	{
  	if (x.Guid.HasValue && y.Guid.HasValue)
  	{
  	result = x.Guid.Equals(y.Guid);
  	}
  	}
  	if (result)
  	{
  	if (x.Version != null && y.Version != null)
  	{
  	result = x.Version.Equals(y.Version);
  	}
  	else if (x.Version != null || y.Version != null)
  	{
  	result = false;
  	}
  	if (x.MaximumVersion != null && y.MaximumVersion != null)
  	{
  	result = x.MaximumVersion.Equals(y.MaximumVersion);
  	}
  	else if (x.MaximumVersion != null || y.MaximumVersion != null)
  	{
  	result = false;
  	}
  	if (result && x.RequiredVersion != null && y.RequiredVersion != null)
  	{
  	result = x.RequiredVersion.Equals(y.RequiredVersion);
  	}
  	else if (result && (x.RequiredVersion != null || y.RequiredVersion != null))
  	{
  	result = false;
  	}
  	}
  	}

To my mind, using else just before the end of a function is a useless use of else.

[SteveL-MSFT]

Does it make sense to default this to $moduleName, $guid to $actualGuid, and $version to $actualVersion since that is used by most (all?) of the tests?

[rjmholt]

Yes I think that makes sense 👍

[rjmholt]

Worth noting that the $moduleName one relies on dynamic scope. I kept the other parameters, since I think it's less confusing. But it's not quite consistent with $moduleName.

[SteveL-MSFT]

If all the negative test cases result in a terminating error, it may be simpler to have the FQErrorId as part of the testcase variation so this would simply be

$sb | Should -Throw -ErrorId $ErrodId

[rjmholt]

Just realised this second suggestion doesn't work just yet; Import-Module -MaximumVersion -MinimumVersion throws a different error to Import-Module @{ ModuleVersion = ; MaximumVersion = }.

That's what #7347 changes.

[SteveL-MSFT]

Ok

[anmenaga]

@SteveL-MSFT was the feedback addressed?

[daxian-dbw]

Reopen the PR to restart stuck CIs

[anmenaga]

@daxian-dbw this can be merged now.