8000 Version checking tests for Import-Module by rjmholt · Pull Request #7499 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]
Skip to content

Version checki 8000 ng tests for Import-Module#7499

Merged
daxian-dbw merged 11 commits intoPowerShell:masterfrom
rjmholt:import-module-tests
Sep 28, 2018
Merged

Version checking tests for Import-Module#7499
daxian-dbw merged 11 commits intoPowerShell:masterfrom
rjmholt:import-module-tests

Conversation

@rjmholt
Copy link
Collaborator
@rjmholt rjmholt commented Aug 11, 2018

PR Summary

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

PR Checklist

This adds testing for #7125

@rjmholt rjmholt changed the title Version checking tests for Import-Module WIP: Version checking tests for Import-Module Aug 11, 2018
@rjmholt
Copy link
Collaborator Author
rjmholt commented Aug 11, 2018

Have marked as WIP while I add GUID tests

@rjmholt rjmholt changed the title WIP: Version checking tests for Import-Module Version checking tests for Import-Module Aug 13, 2018
@rjmholt rjmholt changed the title Version checking tests for Import-Module WIP: Version checking tests for Import-Module Aug 13, 2018
@rjmholt rjmholt changed the title WIP: Version checking tests for Import-Module Version checking tests for Import-Module Aug 14, 2018
@rjmholt
Copy link
Collaborator Author
rjmholt commented Aug 14, 2018

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

@rjmholt rjmholt mentioned this pull request Aug 15, 2018
Closed
11 tasks
SteveL-MSFT
SteveL-MSFT requested changes Aug 16, 2018
View reviewed changes
Copy link
Member
@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll wait for some refactoring before reviewing in detail

test/powershell/Modules/Microsoft.PowerShell.Core/ModuleConstraint.Tests.ps1 Outdated
}
if ($Guid)
{
$mod.Guid | Should -Be $actualGuid
Copy link
Member
@SteveL-MSFT SteveL-MSFT Aug 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

test/powershell/Modules/Microsoft.PowerShell.Core/ModuleConstraint.Tests.ps1 Outdated
}
if ($Guid)
{
$mod.Guid | Should -Be $actualGuid
Copy link
Member
@SteveL-MSFT SteveL-MSFT Aug 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already handled by 217

test/powershell/Modules/Microsoft.PowerShell.Core/ModuleConstraint.Tests.ps1 Outdated
}
if ($Guid)
{
$mod.Guid | Should -Be $actualGuid
Copy link
Member
@SteveL-MSFT SteveL-MSFT Aug 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is handled by 244

test/powershell/Modules/Microsoft.PowerShell.Core/ModuleConstraint.Tests.ps1 Outdated
}
if ($Guid)
{
$mod.Guid | Should -Be $actualGuid
Copy link
Member
@SteveL-MSFT SteveL-MSFT Aug 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Covered by 441

test/powershell/Modules/Microsoft.PowerShell.Core/ModuleConstraint.Tests.ps1 Outdated
}
if ($Guid)
{
$mod.Guid | Should -Be $actualGuid
Copy link
Member
@SteveL-MSFT SteveL-MSFT Aug 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Covered by 469

test/powershell/Modules/Microsoft.PowerShell.Core/ModuleConstraint.Tests.ps1 Outdated
}
if ($Guid)
{
$mod.Guid | Should -Be $actualGuid
Copy link
Member
@SteveL-MSFT SteveL-MSFT Aug 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Covered by 998

test/powershell/Modules/Microsoft.PowerShell.Core/ModuleConstraint.Tests.ps1 Outdated
}
if ($Guid)
{
$mod.Guid | Should -Be $actualGuid
Copy link
8000
Member
@SteveL-MSFT SteveL-MSFT Aug 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Covered by 1025

test/powershell/Modules/Microsoft.PowerShell.Core/ModuleConstraint.Tests.ps1 Outdated
}
if ($Guid)
{
$mod.Guid | Should -Be $actualGuid
Copy link
Member
@SteveL-MSFT SteveL-MSFT Aug 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Covered by 1251

test/powershell/Modules/Microsoft.PowerShell.Core/ModuleConstraint.Tests.ps1 Outdated

$mod = Import-Module -FullyQualifiedName $modSpec -PassThru

$mod.Name | Should -Be $moduleName
Copy link
Member
@SteveL-MSFT SteveL-MSFT Aug 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

test/powershell/Modules/Microsoft.PowerShell.Core/ModuleConstraint.Tests.ps1
if ($ModuleVersion -and $MaximumVersion -and ($ModuleVersion -ge $MaximumVersion))
{
$sb | Should -Throw -ErrorId 'ArgumentOutOfRange,Microsoft.PowerShell.Commands.ImportModuleCommand'
return
Copy link
Member
@SteveL-MSFT SteveL-MSFT Aug 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why return instead of else?

Copy link
Collaborator Author
@rjmholt rjmholt Aug 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
SteveL-MSFT requested changes Aug 16, 2018
View reviewed changes
test/powershell/Modules/Microsoft.PowerShell.Core/ModuleConstraint.Tests.ps1 Outdated
{
param(
$Module,
[string]$Name,
Copy link
Member
@SteveL-MSFT SteveL-MSFT Aug 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author
@rjmholt rjmholt Aug 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I think that makes sense 👍

Copy link
Collaborator Author
@rjmholt rjmholt Aug 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

test/powershell/Modules/Microsoft.PowerShell.Core/ModuleConstraint.Tests.ps1

if ($ModuleVersion -and $MaximumVersion -and ($ModuleVersion -ge $MaximumVersion))
{
$sb | Should -Throw -ErrorId 'ArgumentOutOfRange,Microsoft.PowerShell.Commands.ImportModuleCommand'
Copy link
Member
@SteveL-MSFT SteveL-MSFT Aug 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author
@rjmholt rjmholt Aug 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member
@SteveL-MSFT SteveL-MSFT Aug 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

@anmenaga
Copy link
anmenaga commented Sep 18, 2018

@SteveL-MSFT was the feedback addressed?

@daxian-dbw daxian-dbw self-assigned this Sep 20, 2018
@daxian-dbw daxian-dbw closed this Sep 20, 2018
@daxian-dbw daxian-dbw reopened this Sep 20, 2018
@daxian-dbw
Copy link
Member
daxian-dbw commented Sep 20, 2018

Reopen the PR to restart stuck CIs

@anmenaga
Copy link
anmenaga commented Sep 21, 2018

@daxian-dbw this can be merged now.

@daxian-dbw daxian-dbw merged commit bd5f771 into PowerShell:master Sep 28, 2018
@rjmholt rjmholt mentioned this pull request Oct 12, 2018
Merged
11 tasks
@rjmholt rjmholt mentioned this pull request Feb 21, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SteveL-MSFT SteveL-MSFT SteveL-MSFT approved these changes

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw

@BrucePay BrucePay Awaiting requested review from BrucePay

@anmenaga anmenaga Awaiting requested review from anmenaga

Assignees

@daxian-dbw daxian-dbw

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@rjmholt @anmenaga @daxian-dbw @SteveL-MSFT
0