Open fflaten opened 2 months ago
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.
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.
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. π€
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.
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'
}
}
}
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.
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
Mock.RequireDefaultMock
type of setting (default true? false for v5-behavior?)-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?
I think I'm following now, you trying to handle two scenarios where the new functionality is either implicitly enabled or disabled.
AllowFallback
, InvokeOriginal
, or AllowOriginal
to allow original command to be called when there is one or more mocks for that command.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. π€
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.
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:
InvokeOriginal
to the first mock of the command.InvokeOriginal
when there is already a mock present for the command result in error.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
}
}
}
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
-AllowFallback
could reference fallback to default mock in module or script-scope-AllowFallback
is weird with multiple parameterized mocks in scope. Better to introduceMock -Throw
shortcut which can be used for both default mock and special parameterized tests?Mocking with nested Pester runs.Mocking works in nested run
PR Checklist
Create Pull Request
to mark it as a draft. PR can be markedReady for review
when it's ready.