WIP - Block original command by default when parameterized mocks exist

[fflaten]

PR Summary

WIP

When a command is mocked by only parameterized mocks, throw for calls not matching any filters unless -AllowFallback is specified.

Fix #2166
Fix #2178

TODO

• Discuss desired behavior for precedence and multiple parameterized mocks etc. See review comments
• Discuss API
  • Do we need a global setting to disable for v5 backwards compatibility?
  • Change parameter and property name? -AllowFallback could reference fallback to default mock in module or script-scope
  • Is this the right approach? Having to add -AllowFallback is weird with multiple parameterized mocks in scope. Better to introduce Mock -Throw shortcut which can be used for both default mock and special parameterized tests?
• Troubleshoot seemingly unaffected but failing test Mocking with nested Pester runs.Mocking works in nested run
• Update error thrown when original command is blocked. Currently a placeholder.

PR Checklist

• PR has meaningful title
• Summary describes changes
• PR is ready to be merged
  • If not, use the arrow next to Create Pull Request to mark it as a draft. PR can be marked Ready for review when it's ready.
• Tests are added/update (if required)
• Documentation is updated/added (if required)

[fflaten]

@nohwnd Experimented a little with this. Not sure if this is the most intuitive direction. See comments. What do you think?

[fflaten]

How should this scenario work? Should a local parameterized mock be able to block fallback to original command when a parent mock allows it?

[fflaten]

This is likely a common use case where a command returns a default result while some parameterized mocks returns special output. Good as is or any exceptions to this behavior?

[fflaten]

How should this work? This is where -AllowFallback/AllowOriginal/AllowPassthrough gets ugly compared to an explicit Mock demo -Throw shortcut to implement a guard mock.

[fflaten]

Name suggestions for parameter and property? Fallback is also used for "fallback to default mock". AllowOriginal could give expectation of blocking stuff like & (Get-Command Get-ChildItem -CommandType Cmdlet). AllowPassthrough is unclear. Other short suggestions?

Focus on other review comments first in case this design is discarded.

[fflaten]

Do we want to support all original positional parameters or is CommandName + MockWith enough? Wouldn't expect the rest to be commonly used.

[fflaten]

Should probably update text and/or use separate line.

[nohwnd]

I don't expect the original behavior to be useful to almost anyone, I would add option to configuration for the whole testbase and that is it.

If you really need the fallback only in a specific case you can still add a default mock that calls the original command via & (Get-Command ...).

At least that is my current view, and imho the easiest way to implement this change, and we can add the disabling on a different levels if we get enough good reasons to do that.

[fflaten]

Oh... 😅

I assumed it was common to e.g. mock Invoke-RestMethod for an external service you don't control while using real calls for the rest. If so, making users write the pass through mock manually is cumbersome. Is that wrong? I rarely write tests depending on external data so just a guess on my end.

[johlju]

Not sure I'm following what you are discussing, but if it is relevant here is my 2c... I would say it is pretty normala to write a Mock <Function> -ParameterFilter { <filter> }. For example I might want Get-Item to get an actual file from disk or from the $TestDrive, but another Get-Item in the same test I want to mock using ParameterFilter. In such case I would not want the configuration of the entire testbase to prevent that. What I'm missing in Pester is a method to say that in some circumstances a call should NOT be allowed unless there is a Mock (regardless of mock having ParameterFilter or not). But in another test or a separate Context-block in the same test file I want to allow it. So setting this on or off in configuration for an entire test run doesn't sound as the best idea to me. 🤔

[johlju]

making users write the pass through mock manually is cumbersome

Yes I agree, this would just add another layer of complexity to an already hard thing to learn for folks and for me as a reviewer to grasp when reading code.

[johlju]

This is what I'm looking for:

function Get-SomeFile {
    $file = Get-Item -Path 'myFile.txt'
    Get-Content -Path $file.FullName -Raw
}

Describe 'Something' {
    BeforeAll {
        # Guard mock
        Mock Get-Item -Throw
    }

    Context 'When something happens' {
        It 'Should do something' {
            # FAILS: This fails because Get-Item is not mocked
            { Get-SomeFile } | Should-Be 'myFile content'
        }
    }

    Context 'When something else happens' {
        BeforeAll {
            Mock Get-Item -MockWith { @{ FullName = 'myFile.txt' } }
            # Guard mock 2
            Mock Get-Content -Throw
        }

        It 'Should do something' {
            Mock Get-Content -MockWith { 'myFile content' }

            # PASS: This passes because mocks are set up
            { Get-SomeFile } | Should-Be 'myFile content'
        }
    }
}

[nohwnd]

Okay, thanks for checking my assumptions. I for myself never used the fall through to real command, and only occasionally I've called the real command from a mock when calling Write-Host or similar "utility" command.

In this case I don't know what to do exactly, putting the allow parameter on all mocks in scope is complicated, and defining a default mock, just to let it call the real command sounds counterintuitive.

[fflaten]

Not sure I'm following what you are discussing, but if it is relevant here is my 2c...

That's the issue with trying to solve two separate issues with one PR 😄
You'd basically like a built-in shortcut syntax for:

# MockHelpers.ps1
function RequireMock {
    [CmdletBinding()]
    param(
        [Parameter(Mandatory)]
        [string] $CommandName,
        [ScriptBlock] $ParameterFilter,
        [string] $ModuleName
    )

    $mockScriptBlock = {
        throw "Missing mock for '$CommandName'."
    }.GetNewClosure()

    Pester\Mock @PSBoundParameters -MockWith $mockScriptBlock
}

# Demo.tests.ps1
BeforeAll {
    . "$PSScriptRoot/MockHelpers.ps1"
}
Describe 'BigScriptWithALotOfMocks' {
    BeforeAll {
        RequireMock -CommandName Get-Something1
        RequireMock -CommandName Get-Something2
    }

    Context 'When testing scenario 1' {
        BeforeAll {
            Mock -CommandName Get-Something1
            Mock -CommandName Get-Something2
        }

        It 'Should return $true' {
            BigScriptWithALotOfMocks | Should -BeTrue
        }
    }
}

The other request wanted to make this implicit when any mock exists for the command. Implicit behavior would be a global setting, but IMO we'd also need a override doing the opposite as your design, which is a little more messy with multiple mocks. E.g.

It 'Who wins? Do we allow original command or not?' {
    Mock demo -ParameterFilter { $name -eq 'world' } -MockWith { 'mocked' } -AllowFallback
    Mock demo -ParameterFilter { $name -eq 'Wisconsin' } -MockWith { 'mocked2' }
    demo -name 'you' | Should -Be 'hello you'
}

It 'Or maybe a separate parameter-set to enable it like a flag at that scope?' {
    Mock demo -AllowFallback
    Mock demo -ParameterFilter { $name -eq 'Wisconsin' } -MockWith { 'mocked2' }
    demo -name 'you' | Should -Be 'hello you'
}

It 'Or just a shortcut syntax to re-enable passthrough' {
    Mock demo -InvokeOriginal # Default mock will passthrough
    Mock demo -InvokeOriginal -ParameterFilter { $name -eq 'world' } # Passthrough
    Mock demo -ParameterFilter { $name -eq 'Wisconsin' } -MockWith { 'mocked2' }
    demo -name 'you' | Should -Be 'hello you'
}

I see the value of both though I'd probably wouldn't use either often. My suggestions:
Alternative 1 - Support all

• Add a Mock.RequireDefaultMock type of setting (default true? false for v5-behavior?)
• Add two shortcut alternatives to -MockWith (predefined scriptblocks) to simplify local overrides:
  • Mock -Throw -CommandName ... -ParameterFilter ...
  • Mock -InvokeOriginal -CommandName ... -ParameterFilter ...

Alternative 2 - Require explicit code
Update docs with examples. Consider Mock -Throw or publish a utility module to implement RequireMock and InvokeOriginal helpers?

[johlju]

I think I'm following now, you trying to handle two scenarios where the new functionality is either implicitly enabled or disabled.

• Scenario implicitly Enabled:
  • Your suggestion is that we have AllowFallback, InvokeOriginal, or AllowOriginal to allow original command to be called when there is one or more mocks for that command.
• Scenario implicitly Disabled:
  • Your suggestion is that we have Throw that prevents a command to be called when that command has one or more mocks.

If the above is alternative 1 then I vote for alternative 1. 🙂

If alternativ 2 demands users to write even complex mocks to be able to call original command then I suggest we avoid that alternative.

But would be okay for Throw part (RequireMock command) in alternative 1 to be in a utility module. Although that would be yet another module to maintain. 🤔

[fflaten]

If the above is alternative 1

Correct

If alternativ 2 demands users to write even complex mocks to be able to call original command then I suggest we avoid that alternative.

My alt. 2 was to not change the current behavior. So you'd only write a mock to call the original command if there's already a manual guard mock defined, e.g. in a root BeforeAll. As that's a explicit choice I'd assume you'd might consider just moving the guard mock to a more specific block instead.

But would be okay for Throw part (RequireMock command) in alternative 1 to be in a utility module. Although that would be yet another module to maintain. 🤔

Yeah, publishing it as a module requires a little session state trickery to support all scenarios.

[johlju]

The main reason for adding a guard mock in a test I made (and the reason for issue #2166) was so an initial author of a test (me in this example) in the Describe/BeforeAll-block can say "This command we call must always be mocked in all context-blocks, otherwise bad things will happen to the dev machine". Other contributors will not end up in the pit because of the guard mock. For that reason the Mock ... -Throw could be a solution, but I instead used a workaround by adding a "default mock" (a first mock that throws) that is then overridden by another mock. If that functionality to override an existing mock persist in Pester 6 then maybe it is not worth the effort to add Throw if it add complexity.

If we in Pester 6 simplify this PR to:

• default to not allow a call original command if there is a mock for that command
• only way to call the original command is to pass InvokeOriginal to the first mock of the command.
• if passing InvokeOriginal when there is already a mock present for the command result in error.
• (optionally) a setting to revert to Pester 5 functionality (the above no longer works)

This would be a breaking change with Pester 6 for existing tests unless it possible to disable it.

Then this would also hopefully still be allow to create a default (guard) mock:

function MySuperCommand { Do-PreSetup; Do-DevastatingWork }

Describe {
    BeforeAll {
        Mock -CommandName Do-DevastatingWork -MockWith { throw 'This must be mocked' }
    }

    Context 'Test pre setup' {
        It 'should call Do-PreSetup' {
            Mock -CommandName Do-PreSetup
            Mock -CommandName Do-DevastatingWork -MockWith { '1' }

            MySuperCommand
        }
    }

    Context 'Test pre setup' {
        It 'should call Do-PreSetup' {
            Mock -CommandName Do-PreSetup
            Mock -CommandName Do-DevastatingWork -MockWith { '2' }

            MySuperCommand
        }
    }
}