Modify Should Throw to use dot-source operator

[dlwyatt]

Right now, if one wants to both verify whether or not a command throws an exception and test its output at the same time, the code requires a bit of a workaround:

It 'Tests something' {
    $hash = @{}
    { $hash['Result'] = Do-Something } | Should Not Throw
    $hash['Result'] | Should Be 'Some successful output'
}

By making the Should Throw command dot-source the script block, any variable assignments inside that block will persist out in the scope of the It block, allowing for a cleaner approach:

It 'Tests something' {
    { $result = Do-Something } | Should Not Throw
    $result | Should Be 'Some successful output'
}

[nohwnd]

I think that we should rather be getting rid of the Should Not Throw that embracing it's use and improving it. It's only value in Pester is that it has better error messages, but other than that it does more harm than good. Using the TDD mantra of Red-Green-Refactor cycle, you should first see your test fail. What is usually overlooked is that the test should fail in assertion, not just anywhere:

Describe " " {
   It 'Properly failed test' {
         { "abc" } | Should Throw
    }

    It 'Properly failed test' {
         2 | Should Be 1
    }

    It 'Test exception' {
        throw
    }

    It 'Test exception undercover' {
         { throw } | Should Not Throw
    }
}

If you look at the result of the tests, you will see that all of them are red, but only the first two qualify as the Red which should be followed by Green. The other two are test exceptions, which suggest that your test is broken.

Now the problem is that using the Should Not Throw you are making it look like a proper test, even though you are not testing anything valid. And that is why you want to continue by asserting on the result of the call.

---

The other suggested use case is using it with Should Throw. In that case you are expecting to get an exception. So the exception is the result, not the output of the tested function. And in that case I would much rather see syntax using PassThru than dot-sourcing magic, because it is in my opinion better alligned with PowerShell, and with other testing frameworks like xUnit:

It 'Passing exception' {
    $ex = { Throw [InvalidOperationException]"Sorry!" } | Should Throw -PassThru
    $ex.Message | Should Be "Sorry!"
}

---

So to me it would make more sense to improve error messages for all exceptions, not just those decorated with Should Not Throw. (I will have a look on it this week hopefully.). And not merge this change, as it adds more confusion to behavior that is already confusing for many people.

What do you think?

[dlwyatt]

I'm not sure I agree with taking away features just because they don't fit with a particular development method. Not everyone practices TDD, myself included.

Personally, I've always thought of Pester simply as a test framework, not as a "TDD" or "BDD" framework. What's most important is that you have tests with decent coverage, not what order you happened to write the code.

In any case, we can't get rid of Should Not Throw, because negation is a generic feature of the Should command. If Should Throw exists, so does Should Not Throw. Also, we can't add switches to individual Should operators until v4.

[nohwnd]

If throwing any exception would give you as nice output as Should Not Throw, would there be any reason to use it?

The order in which you write the code does not matter. You can write tests before or after, but you should always make your test fail in the assertion to prove that the assertion will fail when incorrect results are output.
By using Should Not Throw you are testing that the test will fail if an exception is thrown, which is just explicitly re-stating what the testing framework promised to do in the first place. :)

I understand the technical limitations, it was just a suggestion. Because dot-sourcing will change the behavior for all the legitimate uses of Should Throw, and there you don't need the result.

[dlwyatt]

If throwing any exception would give you as nice output as Should Not Throw, would there be any reason to use it?

I have no idea. :) That's a personal preference thing; I'd have to get a feel for both options first. Right now, I feel like the "unhandled" exception output is packed with useful information (stack trace, etc), because many people seem to want to be able to troubleshoot an error based purely on the console output from their CI server. Me, I'm usually happy with the shorter output from Should Not Throw, and I just debug my code locally if something fails in the test.

I'm not sure where the middle ground is there, unless we start setting up preference variables and/or parameters to Invoke-Pester that control how much data it prints out when errors happen.

[JamesWTruher]

I do like the idea of being able to both checks

my concern with Should Throw has been the check of the exception message. In some environments (esp. those that are destined for multi-language support) the exception message will change. Because of that, we've been using the following pattern:

try {
    do-it
    throw "Execution OK"
}
catch {
    $_.FullyQualifiedErrorId | Should be "<whatevertheFQEIhastobe>"
}

Since the FullyQualifiedErrorId is immutable

I'd be much happier if I could do

{remove-item -ea stop badfile}|should throw "PathNotFound,Microsoft.PowerShell.Commands.RemoveItemCommand"

[dlwyatt]

Can't change that in v3 without a breaking change, but the changes to Should in v4 will allow for this kind of easy expansion. It would look something like this:

$someScriptBlock | Should -Throw -FullyQualifiedErrorId 'PathNotFound,Microsoft.PowerShell.Commands.RemoveItemCommand'

[dfch]

@dlwyatt I understand that you want to avoid breaking changes, so why don't we add another Throw-derived keyword such as ThrowException (or ThrowWithFullyQualifiedErrorId):

{ $someScriptBlock } | Should ThrowWithFullyQualifiedErrorId 'PathNotFound,Microsoft.PowerShell.Commands.RemoveItemCommand'

# or

{ $someScriptBlock } | Should ThrowException 'CommandNotFoundException'

This would go in-line with the existing syntax and we could get rid of the tedious synatx that @JamesWTruher mentioned.

[nohwnd]

You can extend Pester with custom assertions if you want. Otherwise I would suggest you wait for version 4. Adding a very specific new assertion to Pester v3 is imho not the way to go, next week someone else will request Should ThrowWithFullyQualifiedErrorIdOrContainInMessage and so on.

[dfch]

@nohwnd Cool! I wasn't aware of that. I read the article and tried this and it in general works as I expected. However I came across a strange behaviour when using the NOT keyword along with my assertion.

When I write {1 * 0 } | Should Not ThrowException System.DivideByZeroException the test should actually succeed, but fails. In addition, the NotFailureMessage does not reflect that the test was actually invoked with the NOT keyword.

Is this something where I got the custom assertion wrong? Or does this not work with ScriptBlocks?

Here is the test output from the tests in the ThrowException assertion:

Describing Test-ThrowException.Tests
   Context Test-ThrowException
    [+] Warmup 156ms
    [+] AssertionExists 334ms
    [+] GettingHelp-ShouldSucceed 47ms
    [+] ThrowExceptionWithFullQualifiedExpectedExceptionSucceeds 45ms
    [+] ThrowExceptionWithExpectedExceptionSucceeds 43ms
    [+] ThrowExceptionWithPartialExpectedExceptionSucceeds 41ms
    [+] ThrowExceptionWithRegexExpectedExceptionSucceeds 34ms
    [-] ThrowExceptionWithUnexpectedExceptionIsSupposedToFail 40ms
      Test was expected to throw exception of type 'CommandNotFoundException', but was not thrown.
      58:                       { 1 / 0 } | Should ThrowException CommandNotFoundException;
      at <ScriptBlock>, C:\Github\biz.dfch.PS.Pester.Assertions\src\ThrowException.Tests.ps1: line 58
    [+] ThrowExceptionWithoutExpectedExceptionSucceeds 50ms
    [-] ThrowExceptionWithoutExceptionIsSupposedToFail 44ms
      RuntimeException: Test was supposed to throw exception 'System.DivideByZeroException', but was not thrown.
      at PesterThrowException, C:\Program Files\WindowsPowerShell\Modules\biz.dfch.PS.Pester.Assertions\ThrowException.ps1: line 84
      at Get-TestResult, C:\Program Files\WindowsPowerShell\Modules\Pester\Functions\Assertions\Should.ps1: line 45
      at <ScriptBlock>, C:\Github\biz.dfch.PS.Pester.Assertions\src\ThrowException.Tests.ps1: line 70
    [-] ThrowExceptionWithoutExceptionShouldSucceedButActuallyFails 52ms
      RuntimeException: Test was supposed to throw exception 'System.DivideByZeroException', but was not thrown.
      at PesterThrowException, C:\Program Files\WindowsPowerShell\Modules\biz.dfch.PS.Pester.Assertions\ThrowException.ps1: line 84
      at Get-TestResult, C:\Program Files\WindowsPowerShell\Modules\Pester\Functions\Assertions\Should.ps1: line 45
      at <ScriptBlock>, C:\Github\biz.dfch.PS.Pester.Assertions\src\ThrowException.Tests.ps1: line 76

Thanks for your reply. Or should I ask this in a separate issue (as this is not really related to the PR)?

[nohwnd]

@dfch did not notice this until today :/ Should -Throw is supposed to first check that you thrown an exception, and then optionally refine the selection of the exception by the aditional filters. Should -Not -Throw is supposed to check that an exception was not thrown, and since throwing any exception automaticaly fails the Should -Not throw, there is no need for additional filters. The whole Should -Not -Throw is something I would like to get avoid, but cannot because of the way the api is written (and because it would break compatibility). Every line of code is implicit Should -Not -Throw, so using it explicitly means that you are writing incomplete test. We are using code for it's return values or it's side effects, so we should validate those, not that the code did not fail.

[it-praktyk]

Guys, should it be still open?

[nohwnd]

Most likely not. Won't implement this, it would change the behavior, and should not throw is bad practice.