pester / Pester

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

Compound assertions as default & default PassThru on assertions #1325

Closed renehernandez closed 4 years ago

renehernandez commented 5 years ago

Writing tests with multiple assertions is not-recommended because it gives you just partial information on failure. The way to do this correctly is to write multiple It blocks, each having just a single assertion. Or we could allow assertions to tell the framework that there was an error, but not actually fail. This combined with outputting the asserted object, and maybe some extra assertions, would allow for chained assertions that only fail when the test is done, not at the first failed assertion:

Describe "d1" { 
    It "i1" {
        $user = Get-User

        $user | Should -NotBeNullOrEmpty -ErrorAction Stop

        $user | 
            Should -HaveProperty Name -Value "Jakub" |
            Should -HaveProperty Age  -Value 30 
    }
}

So here the first assertion would behave like the normal Pester assertion, it would fail the whole test, because it uses -ErrorAction Stop. The next two assertions would inform the framework that there is a new error and the test would fail in the end, but both of the assertions would run, not just the first one to fail.

I think this leads to a very readable tests, and would be a huge improvement over the current approach.

This issues drives the implementation for compound assertions from issue #1319. Further feature discussions are encouraged to take place here

renehernandez commented 5 years ago

@nohwnd What about the following scenario?

Describe "d1" { 
    It "i1" {
        $user = Get-User

        $user | Should -Not -BeNullOrEmpty

        $user | Should -HaveProperty Name -Value "Jakub"
    }
}

What happens if the first Should assertion fails? Do we want the behaviour of "just" informing the error be the default for any scenario, or just for chained assertions?

nohwnd commented 5 years ago

@renehernandez Informing of the error should be default for all assertions. Chaining can be used to provide a more fluent syntax, but it is not mandatory to use it.

renehernandez commented 5 years ago

Possible scenarios and expected behaviors:

Scenario 1: ErrorAction specification:

Describe "d1" { 
    It "i1" {
        $user = Get-User
        $user | Should -Not -BeNullOrEmpty -ErrorAction Stop
    }

    It 'i2' {
        $user = Get-User
        $user | Should -Not -BeNullOrEmpty
    }
}

Behaviors:

Scenario 2: Chained assertions:

Describe "d1" { 
    It "i1" {
        $user = Get-User

        $user | 
            Should -HaveProperty Name -Value "Jakub" |
            Should -HaveProperty Age  -Value 30
    }

    It "i2" {
        $user = Get-User

        $user | 
            Should -HaveProperty Name -Value "Jakub" -ErrorAction Stop |
            Should -HaveProperty Age  -Value 30
    }

    It "i3" {
        $user = Get-User

        $user | 
            Should -HaveProperty Name -Value "Jakub" |
            Should -HaveProperty Age  -Value 30 -ErrorAction Stop
    }
}

Behaviors:

Scenario 3: Chained assertions after single assertion

Describe "d1" { 
    It "i1" {
        $user = Get-User
        $user | Should -Not -BeNullOrEmpty -ErrorAction Stop

        $user | 
            Should -HaveProperty Name -Value "Jakub" |
            Should -HaveProperty Age  -Value 30
    }

    It 'i2' {
        $user = Get-User
        $user | Should -Not -BeNullOrEmpty

        $user | 
            Should -HaveProperty Name -Value "Jakub" |
            Should -HaveProperty Age  -Value 30 
    }
}

Behaviors: TBD

renehernandez commented 5 years ago

@nohwnd I am not sure what should be the expected behaviors for Scenario 3. Could you please fill in the details and expand on any other possible scenario you can think of?

nohwnd commented 5 years ago

@renehernandez -ErrorAction Stop means that Should throws an exception instead of just letting Pester know about the error and continuing. Pester catches the error and prints all errors + the thrown error. It is no different from the previous scenarios, just in some of them the throwing assertion comes last, so there is no difference in the perceived behaviour, but the mechanism that communicates the error to Pester is different.

So i1 either fail in the guard assertion, or runs to completion. i2 will always run to completion. (Given that no other exception is thrown from the rest of the code).

renehernandez commented 5 years ago

@nohwnd In the Invoke-Assertion function, some data may be returned at Should.ps1 line 143. Couple of questions that stem from my work:

nohwnd commented 5 years ago

What is the purpose of this?

From what I see in the code the only purpose of that is to pass the data from the PesterThrow function to the Should and then return it to the output when -PassThru on that assertion was specified (look here). So it's most likely coming from the time when I monkey patched that feature in there.

With compound assertions, should both error records and data be returned?

No the output of Should should be either the input data (e.g. for Should -Be ), or the error record for Should -Throw, or the data for Should -Not Throw, but not both. The purpose of returning the data by default is that we can chain the assertions, but if no-one captures the data they go to $null.

The actual "failure" of the assertion should be communicated to the framework out of bounds, not by outputting it to the pipeline.

nohwnd commented 4 years ago

Revisited the concept yesterday and I think the chained assertions bring very little additional value while still introducing breaking change. I have hard time explaining the usefulness of them to myself, as well as coming up with scenarios that are otherwise not easily possible.

Additionaly the runtime now collects the standard output so the idea that we can output everything because it is thrown away anyway is not valid anymore.

In the PR will revert the parts that output the values and put -PassThru back to -Throw.

The rest of the idea is still valid, and imho pretty nice to use. ☺️

nohwnd commented 4 years ago

One more open question here, is if ErrorActionPreference should make the Should fail, or if just the -ErrorAction and possibly ShouldActionPreference (and respective global Pester option should do the same). The reasoning here is that Eap will have impact on all the code that we are running and not just on the Assertions, and will also prevent the multiple assertions from working if eap Stop is used to force the rest of the code to fail.

I am inclined to do it this way:

Thoughts?

nohwnd commented 4 years ago

The scope on this was reduced, because the rest did bring much benefit. What is possible is that you have multiple Shoulds per It and instead of throwing they record the error and continue.

This is implemented in conjunction with the new configuration. The default behavior is the original behavior where it stops on first error (by throwing from Should). This is less harmful than the other option, but it is easy to opt in, either by providing configuration (either via -Configuration or via $PesterPreference = [PesterConfiguration]{ Should = @{ ErrorAction = 'Stop' }}). Should is also setup to always use 'Stop' when inside of Mock ParameterFilter to further reduce the migration pain.

I decided not to: