pester / Pester

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

Throw Message Assertion is not working in all cases #282

Closed ghostsquad closed 9 years ago

ghostsquad commented 9 years ago

Repro script

Describe 'Test' {
    It 'Test a Throw 1' {
        $o = new-object psobject
        $o | Add-Member -MemberType ScriptMethod -Name 'Test' -Value {
            throw (new-object System.Exception('my message'))
        }

        { $o.Test() } | Should Throw 'Assert failed on "Test" with "0" argument(s): "my message"'
    }

    It 'Test a Throw 2' {
        $o = new-object psobject
        $o | Add-Member -MemberType ScriptMethod -Name 'Test' -Value {
            throw (new-object System.Exception('my message'))
        }

        { $o.Test() } | Should Throw 'Exception calling "Test" with "0" argument(s): "my message"'
    }
}

Output (slightly altered to get expected vs actual messages to line up)

Describing Test
 [-] Test a Throw 1 36ms
   Expected: the expression to throw an exception with message 
{Assert failed on "Test" with "0" argument(s): "my message"}, an exception was raised, message was 
{Assert failed on "Test" with "0" argument(s): "my message"}
       from C:\temp\pesterthrow.tests.ps1:8 char:11
       +         { $o.Test() } | Should Throw 'Assert failed on "Test" with "0" argument( ...
       +           ~~~~~~~~~
   at line: 8 in C:\temp\pesterthrow.tests.ps1
 [+] Test a Throw 2 13ms
Tests completed in 50ms
Passed: 1 Failed: 1 Skipped: 0 Pending: 0

Notice how the actual exception message is actually different that that which Pester is showing us? If Pester were showing us the real exception message, the first test would pass, and the second test would fail. Clearly those 2 strings in the first test are identical.

dlwyatt commented 9 years ago

Well that's just weird... the culprit is this line in Get-PesterResult (in It.ps1):

$testResult.failureMessage = $failureMessage -replace "Exception calling", "Assert failed on"

I have no idea why it does that. This replacement happens after the code that compares the actual exception text to the arguments that were passed to Should Throw.

It's probably fine to fix this, since it only affects the output of the test run, and in a very minor way.

dlwyatt commented 9 years ago

That string replacement has been in the code in one form or another since 2011: https://github.com/pester/Pester/commit/d5dcb63c7e7882fb7d8e8fc6dedada0c50408b24#diff-4b292ba341f46e6e958c28bd0b818ca0

If there's any good reason for it to be there, it's being done in the wrong place. That replacement should be done down in the PesterThrow function before any comparison is done with the argument.

ghostsquad commented 9 years ago

I removed the offending "-replace", but I'm not sure how to get a passing test for this. I wrote this, but it's clear from the write-host that something else is going on.

It 'records same exception message as in provided exception object' {
    $myexception = New-Object Exception('Exception calling "foo"')
    $actualResult = Get-PesterResult $null $null $myexception

    write-host $myexception.message -fore cyan
    write-host $actualResult.FailureMessage -fore cyan

    #both of these assertions are equivalent, and throw an exception.
    #$myexception.Message -eq $actualResult.FailureMessage | Should Be $true
    #$actualResult.FailureMessage | Should Be $myexception.Message
}

Output

Describing Get-PesterResult
 [+] records the correct stack line number of failed tests 149ms
Exception calling "foo"
System.Exception: Assert failed on "foo"
 [-] records same exception message as in provided exception object 13ms
   Expected: {True}
   But was:  {False}
   at line: 19 in C:\dev\pester\Functions\It.Tests.ps1
dlwyatt commented 9 years ago

That's not really a useful test for testing Should Throw; Get-PesterResult needs to be passed an ErrorRecord (the $Exception parameter name is a bit misleading there), and that ErrorRecord should come from calling New-ShouldException (also not named very well.)

dlwyatt commented 9 years ago

BTW, if you open a pull request to remove that -replace operation, I think we'll just merge it.

ghostsquad commented 9 years ago

PR: https://github.com/pester/Pester/pull/284

I still think this change needs a test, I'm just not sure how to test Pester FROM Pester. Seems like generally a bad idea to use a testing framework to test itself. ??

ghostsquad commented 9 years ago

Ah, good call. That's probably the problem.

On Thu, Feb 19, 2015, 10:19 AM Dave Wyatt notifications@github.com wrote:

Closed #282 https://github.com/pester/Pester/issues/282 via 89a06e3 https://github.com/pester/Pester/commit/89a06e3b753c698893c6eff5e5f3ddfd02ae54c4 .

— Reply to this email directly or view it on GitHub https://github.com/pester/Pester/issues/282#event-238576274.