Mock module qualified calls

[dlwyatt]

Proposed fix for #431 .

Wound up having to change a lot more code than I expected, because we were using module-qualified calls inside of Pester specifically to avoid triggering a user's mock. For the most part (where scopes don't interfere), this is now addressed by invoking CmdletInfo objects stored in the $script:SafeCommands hashtable, with the call operator & instead.

When dot-sourcing test files (such as by pressing F5 in the ISE), lots of things started blowing up, so I went through the module and made sure that almost all of the calls to built-in cmdlets are made safe using the new SafeCommands table. All seems to be working fine now on PowerShell v2 and v5 on my system (both through Invoke-Pester and with dot-sourcing test scripts), so we should be all set, assuming v3 and v4 behave properly on the build server.

[KirkMunro]

If you want to, you can switch on the PS version for the proper module for Out-Null in your safe command table. IIRC in PSv4+ it is in Microsoft.PowerShell.Core, and in earlier versions it is in Microsoft.PowerShel.Utility.

[dlwyatt]

In PSv2, it's not in a module at all (probably still loaded in a PSSnapin or something), which is why I just took out that parameter when the tests blew up on v2 before I pushed my commits up to GitHub.

Didn't seem worth the effort to track down behavior on psv3 / v4 at the time, and even if someone does somehow replace Out-Null with another Cmdlet, it's probably not going to break anything.

[dlwyatt]

Oh, I see now. The Snapin / Module thing doesn't matter; you're right, it's in Utility in v2. Will try to update as you suggested and see what happens.

[nohwnd]

I think I found a better way to implement this. Create an internal PesterSafeCommands module that imports all modules we use internally. That way you automatically have a reference to all the commands in those modules. You can invoke them normally without the &, you will keep intellisense, and you don't need to create a table of commands.

InModuleScope Pester { # not needed but closer to how it will behave
    $null = New-Module -Name PesterSafeCommands -ScriptBlock {
        Import-Module  Microsoft.PowerShell.Management } 
    Describe "Mocking FQCN" {
        it "mocks FQCN but keeps the commands working for internal purposes" {
            Mock Get-ChildItem { "I am mock" }
            New-Alias Microsoft.PowerShell.Management\Get-ChildItem Get-ChildItem

            Get-ChildItem | should be "I am mock"
            Microsoft.PowerShell.Management\Get-ChildItem | Should Be "I am mock"
            $o = PesterSafeCommands\Get-ChildItem 
            $o | should not be "i am mock"

            Write-Host ($o | Out-String)
        }
    }
    Get-Module PesterSafeCommands | Remove-Module
}

[dlwyatt]

Hmm, it's certainly more appealing that way. Can anyone think of a way to break things in that approach so that a mock gets invoked instead of the desired original cmdlets?

[nohwnd]

I quickly tested it by replacing Microsoft\.[^\\]*\\ by PesterSafeCommands\ in all the *.ps1 files and adding this to describe.ps1, and it seem to work pretty fine.

    Microsoft.PowerShell.Core\Get-Module PesterSafeCommands |  Microsoft.PowerShell.Core\Remove-Module
    Microsoft.PowerShell.Core\New-Module  -Name PesterSafeCommands -ScriptBlock { 
        Import-Module Microsoft.PowerShell.Core, Microsoft.PowerShell.Management, Microsoft.PowerShell.Utility 
    } | Microsoft.PowerShell.Core\Import-Module
    $Pester.EnterDescribe($Name)

There is some problem with writing errors, but 90 percent of the tests work. I don't see any obivous way to break this.

[dlwyatt]

Running into some problems with Snapins vs Modules in that approach. Microsoft.PowerShell.Core is a PSSnapin in all current versions, and back in PSv2, all of these are snapins rather than modules.
Import-Module doesn't work on those, and Add-PSSnapin also appears to fail (though I'm not sure why).

How did you account for those in your test?

Edit: We can still use this approach without relying on the default import behavior, by using proxy commands in our PesterSafeCommands module. They can be dynamically generated when the Pester module is imported, which will account for differences in parameters / etc between versions. (Though we may lose intellisense with that approach, not sure.)

[nohwnd]

I did not. I did not even know that those used to be snapins. :)

[dlwyatt]

Let me look into the proxy functions idea. I think Intellisense will be okay so long as we import the PesterSafeCommands module into our session when writing the code. That should give us the best of both worlds.

[dlwyatt]

It's blowing up in very weird ways, not sure why yet. At this point, do you think it would be better to commit this PR as-is and then look into alternative approaches later, or is this a blocker that you'd rather have changed before adding in the new feature?

[nohwnd]

Is there anyone waiting for this feature to be implemented, or is it a nice-to-have feature? If someone is waiting for it I would put it in the release. The implementation details may change in the future. If it's a nice to have feature I would wait a bit, maybe someone else will come up with a better solution.

[dlwyatt]

I'd like to have it! :D This came about after a discussion from https://www.sapien.com/blog/2015/10/23/using-module-qualified-cmdlet-names/ , and that module-qualifying calls would cause them to be unmockable in Pester.

Another thing to keep in mind is that we only really run into problems in an edge case: Someone runs a Tests.ps1 script, not with Invoke-Pester, but by dot-sourcing it into the global scope of the session. Unfortunately, this is what happens when someone hits F5 in the ISE.

Now, if we were to take away the ability to run Tests.ps1 scripts directly (which was added in v3 as a "nice to have"), and just go back to telling people to use Invoke-Pester, then Mocks should never interfere with Pester's own internal command resolution.

[nohwnd]

Cool then :) Put it in! I just hope there will be better solution in the future.

[dlwyatt]

Just rebased to fix conflicts with other PRs that were merged, will merge this one if the build succeeds.