Issue with Pester v4.0.2 Mocking a Dummy Function with a Pipeline in two consequitive Context Blocks

[PlagueHO]

Hi Guys,

It appears that Pester 4.0.2 has broken our unit tests in the xStorage DSC Resource module.

Basically, what is happening is that we are creating a dummy function of Get-Volume that accepts a pipeline parameter (this is so that it can accept a mocked partition CIM object that isn't a real CIM class - as is standard for this type of test).

The first context that Mocks this function works perfectly.

The second context that Mocks this function fails to mock correctly : the real Get-Volume is being called - generating the error because we're passing in a PSObject via the pipeline, not a real CIM Partition object.

If I use Pester 3.4.6 everything works perfectly.

Any help would be greatly appreciated.

[nohwnd]

The problem seems to occur when you place your dummy functions inside of Describe, Pester then cannot not find them for some reason. This would normally give you less cryptic message Could not find Command Get-Volume, but since you piped input you got more convoluted message which did not highlight the real issue very well. There is something broken in Pester, but as a workaround you could move you dummy definitions outside of Describe (which is where you normally import functions).

I don't know about the "standard for this type of test", but I would be very careful about shadowing real functions with different functions as your contracts might get out of sync, but that is a different discussion altogether, and I would definitely like to hear your inputs.

Here is a simple repro of the problem, it can be fixed by placing the 'a' function outside the Describe:

Describe "abc" {
    function a (){}        
    Context "first context" {
       Mock a { "mock" } 

       It "passes in first context" {
           a | Should Be "mock"
       }
    }

    Context "second context" {
        Mock a { "mock" } 
        It "passes in second context" {
            a | Should Be "mock"
        }
    }
}

#Describing abc
#
#  Context first context
#    [+] passes in first context 85ms
#
#  Context second context
#    [-] Error occurred in Context block 156ms
#      Could not find Command a
#      At C:\Program Files\WindowsPowerShell\Modules\pester\4.0.2\Functions\Mock.ps1:886 char:9

Here is simplified repro of the original problem:

$script:mockedVolume = "vol1"
Describe "abc" {
    function Get-Volume {
        Param
        (
            [CmdletBinding()]
            [Parameter(ValueFromPipeline)]
            $Partition
        )

        $Partition
    }

    Context "first context" {
       Mock Get-Volume { $script:mockedVolume } -Verifiable

       It "passes in first context" {
           $expected = "abc"
           $expected | Get-Volume | Should -Be "vol1"
       }
    }

    Context "second context" {
        Mock Get-Volume { $script:mockedVolume } -Verifiable

        It "passes in second context" {
            $expected = "abc"
            $expected | Get-Volume | Should -Be "vol1"
        }
    }
}

with output:

Describing abc

  Context first context
    [+] passes in first context 43ms

  Context second context
Get-Volume : The input object cannot be bound to any parameters for the command either because the command does not take pipeline input or the input and its properties do not match any of the 
parameters that take pipeline input.
At line:28 char:25
+             $expected | Get-Volume | Should -Be "vol1"
+                         ~~~~~~~~~~
    + CategoryInfo          : InvalidArgument: (abc:String) [Get-Volume], ParameterBindingException
    + FullyQualifiedErrorId : InputObjectNotBound,Get-Volume
 
    [-] passes in second context 99ms
      Expected: {vol1}
      But was:  {}
      at line: 28 in 
      28:             $expected | Get-Volume | Should -Be "vol1"

[PlagueHO]

Thanks so much @nohwnd for both providing repro steps as well as a work around! That is very awesome!

The only reason we're implementing the "dummy" function to replace the actual function is so we can mock it successfully - because the actual function expects a CIM object of a specific type which we can't easily mock up. This method was mentioned as one way of doing this in an issue on this repo a while back.

Anyway, as this is only affecting one repo that I know of and the changes aren't urgent it'll probably just be easiest to wait for the next version.

Thanks again!

[nohwnd]

@dlwyatt I already had a quick look on it and I suspect that, the function is either put back into wrong scope, or incorrectly put back in when exiting mock. So the best place to start is imho Exit-MockScope. It is possible that I made some error when merging the branches, but hopefully not.

[nohwnd]

@PlagueHO You are welcome.

[nohwnd]

I have spent way too much time on this, and I think that the code that causes this issue has never worked correctly. :)

I Pester v3 there are two types of Mock cleanup. The soft-cleanup that is performed when Context is left, that one only deletes items in mock call history, and hard-cleanup, that removes the mock and puts the original function back in.

In this version of Pester the hard-cleanup is done only on the bottom of Describe, and correctly it should take the PesterIsMocking_a function and rename it back to a, which happens. BUT once the script block that performs the cleanup runs out of scope, the function runs out of scope with it. So effectively the function is never put back correctly. Incidentally this is also the place where a function defined in Describe should correcly run out of scope, so nobody notices that the script does not put the function back.

To make this worse the clean up script works correctly when the function is defined in the global scope, in that case the function is put back correctly and everything works fine. And that is also why it works when we define the function before Describe.

In version 4, there is only a single type of cleanup, the hard-cleanup, because the Context and Describe are the same. This allows the problem with the clean up to become visible, because we are defining a function in scope of Describe and then supposedly putting it back, but that is not what is happening. Instead the function is gone after the cleanup (I think the rename puts it in FunctionTable of the CurrentScope, and that scope is destroyed when the scrpt block is exited, but I need to debug this a bit more too see if that is how it actually works) and we no longer have access to the function.

Now to fix this I don't see any simple way. I need to look a bit more on what the Rename (and the other methods) do on invoke provider, and come up with a better way to do this.

Ideally the behaviour would be the same as when you redefine a function inside of scope, it would shadow the original function with a new behavior and would run out of scope in the same manner as functions. Something like this:

function a () { "top" }

&{
    "should be 'top' -> " + (a)
    function a () { "mocked by this" } 

    &{
        "should be 'mocked by this' -> " + (a)

        function a () { "then by this" }
        "should be 'then by this' -> " + (a)
    }

    "should be 'mocked by this' -> " + (a)

}

"should be 'top' -> " + (a)

How to do that to make it work with the Mock function I don't know yet. But Invoke-Expression is able to define functions from text in the current scope, so the answer must exist.

function a () { "top" }

&{
    "should be 'top' -> " + (a)
    Invoke-Expression 'function a () { "mocked by this" }'

    &{
        "should be 'mocked by this' -> " + (a)

        Invoke-Expression 'function a () { "then by this" }'
        "should be 'then by this' -> " + (a)
    }

    "should be 'mocked by this' -> " + (a)

}

"should be 'top' -> " + (a)

[nohwnd]

Okay, now I can define a function in the caller scope by simply reimplementing the Invoke-Expression in my custom module (Only tested on PowerShell 5). This would get rid of the scoping issues because Mock would only live in it's containing scope. Now let's see how to automatically generate the function, and how calls would be counted.

[nohwnd]

Here is the code:

using System.Management.Automation;
using System.Reflection;

namespace Pester
{
    [Cmdlet(VerbsCommon.New, "Mock")]
    public sealed class Mock : PSCmdlet
    {

        [Parameter(Position = 0, Mandatory = true, ValueFromPipeline = true)]
        public string Command { get; set; }

        protected override void ProcessRecord()
        {

            var myScriptBlock = InvokeCommand.NewScriptBlock(Command);

            var method = myScriptBlock.GetType().GetMethod("InvokeUsingCmdlet", BindingFlags.Instance | BindingFlags.NonPublic);

            var emptyArray = new object[0];
            var automationNull = new PSObject();
            const int writeToCurrentErrorPipe = 1;

            var @params = new object[]
                {this, false, writeToCurrentErrorPipe, automationNull, emptyArray, automationNull, emptyArray};


            method.Invoke(myScriptBlock, @params);
        }
    }
} 

the original implementation is available here.

I also tried to do it in powershell direcly but it did not work, it defined the function inside of the Mck function not in the caller scope. Maybe I just don't understand how to get the "this" refence correctly in the function, I am using the $PSCmdlet to get the "this" reference, but I might be wrong.

### DOES NOT WORK!
function Mck {
    [CmdletBinding()]
    param ($Command)

    $scriptBlock = $ExecutionContext.InvokeCommand.NewScriptBlock($Command)
    $flags = [System.Reflection.BindingFlags]'Instance,NonPublic'

    [object]$i = $scriptBlock

    [object[]]$emptyArray = @()
    [psobject]$automationNull  = $null
    $WriteToCurrentErrorPipe = 1
    
    
    [object[]] $p = $PSCmdlet, $false, $WriteToCurrentErrorPipe, $automationNull, $emptyArray, $automationNull, $emptyArray
    
    $m = $scriptBlock.GetType().GetMethod('InvokeUsingCmdlet', $flags)
    $m.Invoke([object] $i, [object[]] $p)

    abc # it is defined here, but we need it outside
}

Remove-Item 'function:\abc'

$definition = "function abc () { 'defined' }"

# this defines the function inside of itself
Mck $definition
# Invoke-Expression $definition # this defines the function in caller scope

abc

[dlwyatt]

I was thinking it might work to inject an alias instead of a function. (The function would have a random name. ). Then we just delete the alias when removing the mock, and the original function's scope never gets broken.

[nohwnd]

That would be great if that worked. Do you have time to do that? I can do it myself (I hope :) ), I am asking just to avoid duplicating our effort.

The code that I have now seems to have great potential, but there is alot to develop around it, so your way is much closer to solving this problem.

[nohwnd]

Seems to work a bit better with aliases, but still the alias is removed from all the scopes not just the current scope.

function a () { "top" }
function mock1 () { "mocked by this" }
function mock2 () { "then by this" }


&{
    [CmdletBinding()]
    param()
    
    "should be 'top' -> " + (a)
    $ExecutionContext.InvokeProvider.Item.Set('alias:\a', 'mock1')
    "should be 'mocked by this' -> " + (a)
    
    &{
        "should be 'mocked by this' -> " + (a)

        $ExecutionContext.InvokeProvider.Item.Set('alias:\a', 'mock2')
        "should be 'then by this' -> "  + (a)
    }
    $ExecutionContext.InvokeProvider.Item.Remove('alias:\a', $false) 
       

    "should be 'mocked by this' -> " + (a)

}
$ExecutionContext.InvokeProvider.Item.Remove('alias:\a', $false) 


"should be 'top' -> " + (a)

should be 'top' -> top

CommandType     Name                                               Version    Source                                                                                                                                                                                                                                                                                                                                                                              
-----------     ----                                               -------    ------                                                                                                                                                                                                                                                                                                                                                                              
Alias           a -> mock1                                                                                                                                                                                                                                                                                                                                                                                                                                        
should be 'mocked by this' -> mocked by this
should be 'mocked by this' -> mocked by this
Alias           a -> mock2                                                                                                                                                                                                                                                                                                                                                                                                                                        
should be 'then by this' -> then by this
should be 'mocked by this' -> top <----------- the alias was removed completely, not just from single layer
Exception calling "Remove" with "2" argument(s): "Cannot find path 'Alias:\a' because it does not exist."
At line:26 char:1
+ $ExecutionContext.InvokeProvider.Item.Remove('alias:\a', $false)
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : NotSpecified: (:) [], MethodInvocationException
    + FullyQualifiedErrorId : ItemNotFoundException
 
should be 'top' -> top

[PlagueHO]

Thank you guys for all your work on this. I really appreciate it.

[dlwyatt]

PR #744 to fix. Waiting on tests to run.

[dlwyatt]

@nohwnd I'm not following the bit about removing multiple aliases; I can't think of a situation where we'd ever have more than one defined. When you mock a command multiple times, it just adds more blocks to the mock table; the alias creation stuff only happens when the mock didn't already exist.

[nohwnd]

@dlwyatt by removing "all" aliases I mean that it would remove the mock from all scopes, in the same fashion as with functions, but for mocking this is not a problem because you are not touching the aliases at all. So there is no chance you would remove an alias that was defined in parent scope. Instead you are aliasing the function, so in the worst case the parent scope alias calls the mocking alias and that then calls the mock. I would be a problem if you could have alias and function with the same name ( alias a to function a ) but that errors out in powershell. That was my thinking in theory :)

But in real code I am not sure anymore how exactly the InvokeProvider.Remove works. For aliases it seems to remove them only from the current scope, but for functions it seems to remove them globally (which caused the problem with the mocking in the first place). So in fact the .Remove in my example is redundant and only causes problems.

Another thing that I don't fully understand is why the internal pester scope (and mock scope as well unless it comes from a different module) is bound to $PSCmdlet.SessionState. At least it seems to make this work a bit differently than in my example where the Set-Alias only sets the alias in the current scope (similarly to calling function a () {}) so it does not have to be removed explicitly, while in Pester when I remove the calls to the .Remove it breaks the session. Possibly because we are running in the same scope all the time and hence the aliases need to be manipulated explicitly instead of running out of scope automatically. I guess there is a reason or an explanation for this :)

# just some cleanup to make the tests repeatable
if (Test-Path 'alias:\_g')
{
    Remove-Item 'alias:\_g'
}

# .ps1

# in code I define function g and alias _g
function g { "i am g" }
Set-Alias -Name _g -Value g

_g # <- i am g
g  # <- i am g


# tests.ps1

&{
    # in tests I decide to mock so I create a mock function
    function g_mock () { "mock" } 
    # and alias it to hide the real functions and alias
    Set-Alias -Name _g -Value g_mock 
    Set-Alias -Name g -Value g_mock   
    
    _g # <- mock
    g  # <- mock
}

# here I cleanup after describe and it removes all aliases, not just
# the one defined in the previous scope so the original alias is no longer available
# but this is not a problem with aliases anyway because when you define them via Set-Alias
# they get defined in the current scope and they run out of scope

# in pester this seems to behave a bit differently, maybe because the scope of the mock is bound
# to pscmdlet scope of Describe

#if you comment this out it works as expected
$ExecutionContext.InvokeProvider.Item.Remove('alias:\_g', $false)


# alias is not available here
_g