pester / Pester

Pester is the ubiquitous test and mock framework for PowerShell.
https://pester.dev/
Other
3.08k stars 470 forks source link

Shouldn't the value of parameters available in Assert-MockCalled -ParameterFilter be the same as the values passed to the Mock? #325

Closed alx9r closed 9 years ago

alx9r commented 9 years ago

The following code demonstrates that the parameters that are available to Assert-MockCalled -ParameterFilter are in the state they become after the execution of the -MockWith. This is surprising because it seems to preclude directly testing the state of parameters passed to the Mock.

Shouldn't the -ParameterFilter be evaluated with the value of parameters bound to the Mock as they are prior to any code being executed in the Mock?

module.psm1

function f1 
{
    [CmdletBinding()]
    param
    (
        [parameter(ValueFromPipeline=$true)]
        $object1
    )
    process {}
}

module.Tests.ps1

Import-Module module

InModuleScope module {
    Describe 'accessing property from Assert-MockCalled filter' {
        BeforeEach {
            $object = New-Object PSObject -Property @{
                a = $null
                b = $null
        }            
        }
        Mock f1 -Verifiable {
            $object1.a = 'a'
            $object1.b = 'b'
        }
        Context 'context' {
            It 'a and b are originally null, and are changed by mock.' {
                $object.a | Should beNullOrEmpty
                $object.b | SHould beNullOrEmpty

                $object | f1

                $object.a | Should be 'a'
                $object.b | Should be 'b'
            }
            It 'properties of object1 inside mock filter are tested prior to executing mock.' {
                Assert-MockCalled f1 -Exactly -Times 1 {
                    $null -eq $object1.a -and
                    $null -eq $object1.b
                }
            }
            It 'properties of object1 inside mock filter are tested after executing mock.' {
                Assert-MockCalled f1 -exactly -Times 1 {
                    $object1.a -eq 'a' -and 
                    $object1.b -eq 'b'
                }
            }
            It 'calls f1 exactly once.' {
                Assert-MockCalled f1 -Exactly -Times 1
            }
        }
    }
}
dlwyatt commented 9 years ago

This isn't really a bug in Pester, as I see it. You passed in an object reference to your mock, and that's what gets recorded in the call history. The fact that you modified the object in memory later is a defect in the test code, and a common "gotcha" in object-oriented programming.

Instead of modifying the object that was passed in as a parameter, you can structure your code so the input and "output" of your Mock use different objects. Something like this:

Import-Module $psscriptRoot\module.psm1 -Force

InModuleScope module {
    Describe 'accessing property from Assert-MockCalled filter' {
        BeforeEach {
            $hash = @{
                a = $null
                b = $null
            }

            $object = New-Object PSObject -Property $hash
        }

        Mock f1 {
            $hash.a = 'a'
            $hash.b = 'b'
        }

        Context 'context' {
            It 'a and b are originally null, and are changed by mock.' {
                $hash.a | Should beNullOrEmpty
                $hash.b | SHould beNullOrEmpty

                $object | f1

                $hash.a | Should be 'a'
                $hash.b | Should be 'b'
            }

            It 'properties of object1 inside mock filter are tested prior to executing mock.' {
                Assert-MockCalled f1 -Exactly -Times 1 {
                    $null -eq $object1.a -and
                    $null -eq $object1.b
                }
            }

            It 'calls f1 exactly once.' {
                Assert-MockCalled f1 -Exactly -Times 1
            }
        }
    }
}
alx9r commented 9 years ago

@dlwyatt This makes sense. I guess I should have assumed that Pester isn't deep-copying objects or reading ahead to find and run Assert-MockCalled -ParameterFilter blocks.

FWIW, this situation actually arose where the function-under-test called the mock twice. (An idempotent Set-Widget function naturally calls Test-Widget twice, with changes to the widget in between.) Based on your post, it seems that in that situation, the values of the relevant properties of each object passed to the mock must be stored explicitly by the mock for later testing. Does that sound accurate to you?

dlwyatt commented 9 years ago

Yep, I think so. Instead of just using a hashtable, you could set up an ArrayList or something along those lines, and add copies of the objects to that list in the mock. Then your assertion code could check to see that the list contains 2 (or more) entries, and that the properties changed from the first to second call. In that case, you wouldn't even need Assert-MockCalled at all, as a series of Should Be assertions should be fine.

alx9r commented 9 years ago

@dlwyatt Here's a working example of an explicit call history. I appreciate any suggestions or improvements you might have to the style and strategy.

module.psm1

function Set-Widget 
{
    [CmdletBinding()]
    param
    (
        [parameter(ValueFromPipeline=$true)]
        $ExistingWidget,

        $a,

        $b
    )
    process {}
}

modules.Tests.ps1

Import-Module module

InModuleScope module {
    Describe 'explicit call history' {
        BeforeEach {
            $widget = New-Object PSObject -Property @{
                a = 'init a'
                b = 'init b'
            }            
        }
        BeforeAll { $callHistory = [System.Collections.ArrayList]@() }
        Mock Set-Widget -Verifiable {
            $callHistory.Add(@{
                Name = 'Set-Widget'
                a = $a
                b = $b
                ExistingWidget = $ExistingWidget.PsObject.Copy()
            }) | Out-Null
            $ExistingWidget.a = $a
            $ExistingWidget.b = $b
        }
        Context 'first scenario' {
            It 'a and b are changed by mock, twice.' {
                $callHistory.Clear()

                $widget | Set-Widget -a $null -b $null

                $widget.a | Should beNullOrEmpty
                $widget.b | Should beNullOrEmpty

                $widget | Set-Widget -a 'a' -b 'b'

                $widget.a | Should be 'a'
                $widget.b | Should be 'b'
            }
            It 'calls Set-Widget twice.' {
                $callHistory.Count | Should be 2
            }
            It 'calls Set-Widget correctly the first time.' {
                $callHistory |
                    ? {
                        $_.a -eq $null -and
                        $_.b -eq $null -and
                        $_.ExistingWidget.a -eq 'init a' -and
                        $_.ExistingWidget.b -eq 'init b'
                    } |
                    measure | % count |
                    Should be 1
            }
            It 'calls Set-Widget correctly the second time.' {
                $callHistory | 
                    ? { 
                        $_.Name -eq 'Set-Widget' -and
                        $_.a -eq 'a' -and
                        $_.b -eq 'b' -and
                        $null -eq $_.ExistingWidget.a -and
                        $null -eq $_.ExistingWidget.b
                    } |
                    measure | % count |
                    Should be 1
            }
        }
        Context 'second scenario' {
            It 'sets up and runs another test.' {
                $callHistory.Clear()

                # ...
            }
        }
    }
}
nohwnd commented 9 years ago

As Dave explained you are dealing with mutable reference objects and hence you can change their properties without the mock knowing. And that's okay the mock doesn't and shouldn't care. Let me explain why:

You use the Assert-MockCalled to check how many times was the Mock called with a concrete values/objects (as arguments to parameters).

like this:

# -- Arrange
Mock g {}
$value = 1

# -- Act
g -Value 1

# -- Assert
Assert-MockCalled g -ParameterFilter { $value -eq 1 } -Exactly 1

But here lies the problem. You are checking the identity of the Object incorrectly. (I will refer to your Object with capital O. And to general object with small o.)

You are saying: Every object that has a set to "a" and b set to "b" is considered the same as my Object. That is obviously not true, because your Object was created with both a and b set to null and you still consider it the same object. So identifying the argument to the Mock $object1 parameter using this code: $object1.a -eq 'a' -and $object1.b -eq 'b' must also be incorrect.

Luckily your Object has it's default identity defined by it's reference so you can use that, and simply do:

Assert-MockCalled f1 -exactly -Times 1 {
    $object1 -eq $object
}

Wait wait that does not work! It does but you are creating a new object before every It. And this new object obviously does not have the same identity as the previous one. One solution is to reuse the same object -- makes sense in this case since all your test are assuming the same object testing if the object is the same anyway.

function f1 
{
    [CmdletBinding()]
    param
    (
        [parameter(ValueFromPipeline=$true)]
        $object1
    )
    process {}
}

Describe 'accessing property from Assert-MockCalled filter' {
    #removed BeforeEach   
    $object = New-Object PSObject -Property @{
        a = $null
        b = $null
    }

    Mock f1 -Verifiable {
        $object1.a = 'a'
        $object1.b = 'b'
    }

    Context 'context' {
        It 'a and b are originally null, and are changed by mock.' {
            $object.a | Should beNullOrEmpty
            $object.b | SHould beNullOrEmpty

            $object | f1

            $object.a | Should be 'a'
            $object.b | Should be 'b'
        }
        It 'properties of object1 inside mock filter are tested prior to executing mock.' {
            Assert-MockCalled f1 -Exactly -Times 1 {
                $object1 -eq $object #this stops failing after fixing the code to compare the references
            }
        }
        It 'properties of object1 inside mock filter are tested after executing mock.' {
            Assert-MockCalled f1 -exactly -Times 1 {
                $object1 -eq $object
            }
        }
        It 'calls f1 exactly once.' {
            Assert-MockCalled f1 -Exactly -Times 1
        }
    }
}

Or you can still create a new object for every It and you can assign it an explicit identity, using and Id for example:

function f1 
{
    [CmdletBinding()]
    param
    (
        [parameter(ValueFromPipeline=$true)]
        $object1
    )
    process {}
}

Describe 'accessing property from Assert-MockCalled filter' {
    BeforeEach {
        $object = New-Object PSObject -Property @{
            a = $null
            b = $null

            id = 1 #add id to define the identity of object for example 

           <#Exmaple: 
            User:
                Name = Jane
                SureName = Doe
                Id = 2

            is the same person even after marriage
            User:
                Name = Jane
                SureName = Black
                Id = 2 #>

    }            
    }
    Mock f1 -Verifiable {
        $object1.a = 'a'
        $object1.b = 'b'
    }
    Context 'context' {
        It 'a and b are originally null, and are changed by mock.' {
            $object.a | Should beNullOrEmpty
            $object.b | SHould beNullOrEmpty

            $object | f1

            $object.a | Should be 'a'
            $object.b | Should be 'b'
        }
        It 'properties of object1 inside mock filter are tested prior to executing mock.' {
            Assert-MockCalled f1 -Exactly -Times 1 {
                $object1.id -eq $object.id #again this stops failing
            }
        }
        It 'properties of object1 inside mock filter are tested after executing mock.' {
            Assert-MockCalled f1 -exactly -Times 1 {
                $object1.id -eq $object.id
            }
        }
        It 'calls f1 exactly once.' {
            Assert-MockCalled f1 -Exactly -Times 1
        }
    }
}