improved suport for pipeline processing with begin block

[mwrock]

This fixes scenarios where a mocked function is an advanced function and initializes state in a Begin block. Here is an example:

function PipelineInputFunction {
    param(
        [Parameter(ValueFromPipeline=$True)]
        [int]$PipeInt1
    )
    begin{
        $p = 0
    }
    process {
        foreach($i in $input)
        {
            $p += 1
            write-output $p
        }
    }
}

Currently, if pester mock the above function and the function is called piping an array as input, the Begin block will be called for each item in the array.

This PR fixes this.

First, it changes the mock prototype to be wrapped in an End block and batches any pipeline input into an array. Then it filters out all bound parameters that were bound from the pipeline and finally calls the mock (or the original function if the parameter filter does not pass) with the original calling pipeline and bound parameters.

This definitely got more hairy than I would have liked. was really hoping NOT to have to written the Select-NonPipelinedBoundParameters function but could find no invocation metadata getting me what I needed. In the event that the parameter filtering breaks, I have pester fall back to the original non pipelining call and I'm hoping all the tests catch the edge cases.

[mwrock]

oops. will look into failures tonight. They all passed on my box :)

[mwrock]

@dlwyatt is it possible that TC is not calling Import-Module with -Force and is therefore woking with a "stale" module? The failures here are all complaining about the missing Select-NonPipelinedBoundParameters function but that is added to the exports in this PR.

[dlwyatt]

Try adding it to the module manifest as well. (Come to think of it, I'm not sure why we bother to use Export-ModuleMember at all, but whatever. :) )

[mwrock]

Ahh right! Thanks man. That makes total sense.

[mwrock]

ok. This is much more in line with failures I would expect - broke PSv2. Will get that fixed later.

[mwrock]

OK. v2 (and others) are passing now. Its ready for review.

[dlwyatt]

I'm trying to wrap my head around what problem this solves. In the current version of Pester, Mocks don't have begin blocks or end blocks, and I can't see a reason why it should matter whether the original function does or doesn't, since you're mocking it anyway.

The one situation where this might get goofy is if you use a parameter filter on the mock, and the filter matches for some of the input objects but not others (causing the original command to be called several times in the same pipeline, instead of once.)

Do you have an example of tests that aren't working the way you'd like them to without this change?

[mwrock]

Sorry for being too abstract and not making my exact problem more clear. The issue is not about allowing mocks to have begin blocks. Rather, the problem is related when the original implementation of a function has a begin block and is being mocked. If the mock is either out of scope or the -ParameterFilter does not match, when the original implementation is invoked, its behavior is altered.

In my case I am mocking Invoke-Command which is called by Send-File (see https://github.com/mwrock/boxstarter/blob/master/Boxstarter.Chocolatey/Send-File.ps1). This calls invoke-command with the following script block:

$remoteScript = {
    param($destination, $length)

    ## Convert the destination path to a full file system path (to support
    ## relative paths)
    $Destination = $executionContext.SessionState.`
        Path.GetUnresolvedProviderPathFromPSPath("$env:temp\$Destination")

    ## Create a new array to hold the file content
    $destBytes = New-Object byte[] $length
    $position = 0

    ## Go through the input, and fill in the new array of file content
    foreach($chunk in $input)
    {
        [GC]::Collect()
        [Array]::Copy($chunk, 0, $destBytes, $position, $chunk.Length)
        $position += $chunk.Length
    }

    ## Write the content to the new file
    [IO.File]::WriteAllBytes($destination, $destBytes)

    [GC]::Collect()
}

Now this does not have an explicit begin block but the symptom is the same.$positionis reset to0on each iteration of$inputwhen running this viaInvoke-Mock` and running the original implementation.

I am upgrading to the latest Pester from version 2.2. In 2.2 I did not run into this because it handled scoping differently. Invoke-Command is mocked in a different context from the one where Send-File is called. In v2.2, if the mock was in a separate context block,that mock was cleared from the mock table so other contexts calling the function would call the true function and not pass through the mock prototype. In v3, the mock is preserved but other contexts will simply call the original implementation wrapped in Invoke-Mock. I think that is done to handle deeply nested contexts? which seems sensible. So this aims to protect the behavior of the wrapped original implementation.

I could have changed Send-File and forced Invoke-Command to call the actual powershell function by prefixing the module. However, I thought it would be useful to have Pester "just work" so users are not left wondering why behavior has changed and need to "rejigger" their code.

Does this make sense?

Have a look at the function I created for the tests I added. The tests call the function before it is mocked then mock it and set a ParameterFilter to avoid the mocked behavior so the wrapped original implementation is invoked and then assert that the parameters are bound the same and return the same values and the begin block is initialized just as it is without the mock. Without the new Mock code, those tests would show the begin block being called for each value in the pipeline resetting the variable set there.

I realize this increases the complexity of mock.ps1 which I am not crazy about and I respect your judgement on whether to introduce the added complexity or not. I learned some things I did not know about parameter binding and pipeline flow through working on this.

[dlwyatt]

OK, that makes more sense. I'll play around with the code in the PR with that in mind. :) If I have any ideas for making it work with less complexity, I'll let you know.

[dlwyatt]

While these loops do result in fewer lines of code, the output of a failed test is less useful (since we'd be seeing $_ in the logs). Would it be worth it to just explicitly check each property of your function's hashtable? Lots of duplication, but much clearer failure messages.

[mwrock]

Yup. I agree.

[dlwyatt]

Brainstorming a bit on a possible alternate approach:

• Have the mock bootstrap function call Invoke-Mock from all three blocks, Begin/Process/End, with a new parameter indicating from which block the call came.
• When Invoke-Mock is called from the Begin block, we initialize some new state information in the $mockTable entry.
• When Invoke-Mock is called from the Process block, if no matching ParameterFilter is found, instead of immediately invoking the original function, we append the pipelined input object (if present) to our new state (and probably set a boolean flag as well, so we cover cases where we're not using pipeline input)
• When Invoke-Mock is called from the End block, if we need to invoke the original implementation, go ahead and do that (piping in any of the input objects we've cached, if present.)

I think with that approach, we should be able to solve the problem ( so we only call the original command zero or one times ), without needing the overhead or complexity of Select-NonPipelinedBoundParameters.

[mwrock]

Thanks @dlwyatt I will give this a shot.

[mwrock]

I was just looking over your suggestion again and I'm not sure this gets around the need for Select-NonPipelinedBoundParameters. When we finally invoke the original implementation, piping the input, the arguments bound from the pipeline must be unbound first. Otherwise a ParameterBindingException will be thrown trying to pipe in arguments that are also included in $PSBoundParameters.

[dlwyatt]

Are there still pipeline-bound parameters when the End block runs? If so, we can just cache a copy of $PSBoundParameters as it existed when the Begin block ran. Calling Invoke-Mock from all three blocks gives us lots of flexibility.

[mwrock]

Maybe I'm misunderstanding a fundamental truth (or more) regarding parameter binding. I have not been as entrenched in the powershell world as I once was. My understanding is that the binding occurs once and not separately per block. At any rate, I'll experiment and report back. Thanks!

[dlwyatt]

As I understand it, pipeline binding only takes place in the process block (and takes place multiple times, if there are more than one input object.) You can see this in action with Trace-Command.

[mwrock]

Maybe I'm misunderstanding a fundamental truth (or more) regarding parameter binding. I have not been as entrenched in the powershell world as I once was. My understanding is that the binding occurs once and not separately per block. At any rate, I'll experiment and report back. Thanks!

[dlwyatt]

Going to take a shot at this tonight, see what happens. I've pulled your Mock.Tests.ps1 file as-is, and will try to make the tests pass using the approach I was thinking of. If it works, we can compare notes :)

[mwrock]

oh great. It was on my list of stuff to do for the weekend but let me know what you find.

[dlwyatt]

Appears to work, though I've only tested PSv5 and PSv2 on my computer; it won't go through the build server unless it's part of a branch or pull request in the main repo.

master...dlwyatt:PipelineTest

Invoke-Mock was overdue for quite a bit of refactoring, and I did some of that to make the new code clearer.

[mwrock]

Beautiful! Works with my boxstarter unit tests too.