Bring back 'InconclusiveCount' property (or alternative) on returned Pester.Run object

[csandfeld]

Checklist

• [X] Feature request has a meaningful title
• [X] I have searched the existing issues. See all issues
• [X] I have tested using the latest version of Pester. See Installation and update guide.

Summary of the feature request

In Pester v4 I have used Set-ItResult -Skipped and Set-ItResult -Inconclusive in tests, and distinguished between the two when generating reports of the results. In v4 it was easy because InconclusiveCount was returned, but while working on moving to v5, I noticed the property was missing, and found this breaking changes note in the docs:

Inconclusive and Pending states are currently no longer available, -Pending and -Inconclusive are translated to -Skip both on test blocks and when using Set-ItResult

What are the odds of this property - or some alternative means of reliably reporting on "Skipped reason" - to find it's way to the Pester.Run object?

How should it work?

Surface properties similar to what is available in Pester v4.

Alternatively (might be easier to implement) include some kind of "reason data" in the exception thrown by the Set-ItResult command. One option could be to include the data in the targetObject Dictionary object that is already included in the exception. In theory that logic could also allow specifying custom reasons with a parameter for the Set-ItResult command.

[fflaten]

You can still identify the tests skipped using Set-ItResult -Inconclusive by checking the errorrecord message stored in the test-object.

Either by using a reason as keyword or by searching for PENDING or INCONCLUSIVE which is added to the message by Set-ItResult when not using -Skipped. Not saying it's elegant, so just suggesting it as a workaround 🙂

$conf = New-PesterConfiguration
$conf.Output.Verbosity = 'Detailed'
$conf.Run.ScriptBlock = {
    Describe 'a1' { 
        Context 'b1' { 
            It 'i1' {
                Set-ItResult -Inconclusive -Because 'demo'
            }
            It 'i2' -Skip {
                1 | Should -Be 1
            }
        }
    }
}
$conf.Run.PassThru = $true

$run = Invoke-Pester -Configuration $conf
$run.Skipped | ? { $_.ErrorRecord.FullyQualifiedErrorId -eq 'PesterTestSkipped' -and $_.ErrorRecord -cmatch 'INCONCLUSIVE' } | ft ExpandedPath, Result, ErrorRecord

# output

ExpandedPath Result  ErrorRecord
------------ ------  -----------
a1.b1.i1     Skipped {is skipped, because INCONCLUSIVE: demo}

[csandfeld]

Thanks @fflaten, appreciate the tip. That is also the (temporary?) workaround I had in mind. But as you mention it is not very elegant, and I was not 100% sure if it would be a reliable way of doing it.

I also use the "because" message in my reporting, and was surprised to see that additional data is injected in that message. It is a little counter-intuitive that the test author no longer has full control of what is in that message.

As a user of the product I would prefer the "because" message to remain unaltered, and Pending and Inconclusive - or an alternative custom author specified reasoning to be surfaced in another attribute.

[nohwnd]

I looked at the implementation of Set-ItResult and I see that we set 'PesterTestSkipped' as the error id for any of the skip reasons.

 throw [Pester.Factory]::CreateErrorRecord(
        'PesterTestSkipped', #string errorId
        $Message, #string message
        $File, #string file
        $Line, #string line
        $LineText, #string lineText
        $false #bool terminating
    )

There are few handlers that check for this in the upper scopes and set the test results to skipped, rather than failed. e.g. the one that still has the remnants of inconclusive support: https://github.com/pester/Pester/blob/a50354e46344aa8d4b4460e55fd2137939d0a215/src/functions/Output.ps1#L268

I think if you added the switch that used to be there in Set-ItResult, so it throws different error id for inconclusive, and then handled both error ids in thosre remaining handlersr to translate to skipped (literally just adding -or or -contains), it would be a very easy change to make (hopefully :) ), and then the info also flows all the way up to your result, so we can handle the summaries at our own pace, and you still get a non-brittle way to get this info.

import-module s:/p/pester/bin/pester.psd1 -force 

$r = invoke-pester -container ( new-pestercontainer -ScriptBlock { 

    Describe "a" { 
        It "b" { 
            Set-ItResult -Inconclusive -Because 'demo'
        }
    }
}) -passThru

$r.Tests[0].ErrorRecord[0].FullyQualifiedErrorId

# output 

Starting discovery in 1 files.
Discovery found 1 tests in 6ms.
Running tests.
[+]  

    Describe "a" {
        It "b" {
            Set-ItResult -Inconclusive -Because 'demo'
        }
    }
 61ms (2ms|54ms)
Tests completed in 67ms
Tests Passed: 0, Failed: 0, Skipped: 1 NotRun: 0
PesterTestSkipped 

[csandfeld]

Thanks @nohwnd - I can try to tackle that :)

[fflaten]

I think if you added the switch that used to be there in Set-ItResult, so it throws different error id for inconclusive, and then handled both error ids in thosre remaining handlersr to translate to skipped (literally just adding -or or -contains), it would be a very easy change to make (hopefully :) ), and then the info also flows all the way up to your result, so we can handle the summaries at our own pace, and you still get a non-brittle way to get this info.

We'd likely have to update Add-RSpecTestObjectProperties also to mark them as Skipped for now. If not they'll probably get NotRun as Result in the PassThru/run object.

I'm unable to verify until the weekend.

[csandfeld]

Not sure if this address your concerns @fflaten, but here is an example with the changes implemented in PR #2401.

I do notice missing is skipped, because ... output for 'c' and 'd', I'll take a look at that as well.

$r = invoke-pester -container ( new-pestercontainer -ScriptBlock {

        Describe "a" {
            It 'a' {
                $true | Should -BeTrue
            }

            It 'b' {
                $true | Should -BeFalse
            }

            It 'c' {
                Set-ItResult -Inconclusive -Because 'demo inconclusive'
            }

            It 'd' {
                Set-ItResult -Pending -Because 'demo pending'
            }

            It 'e' {
                Set-ItResult -Skipped -Because 'demo skipped'
            }
        }
    }) -passThru -Output Detailed

# output

Starting discovery in 1 files.
Discovery found 5 tests in 16ms.
Running tests.
Describing a
  [+] a 18ms (3ms|15ms)
  [-] b 9ms (7ms|2ms)
   Expected $false, but got $true.
   at $true | Should -BeFalse, :9
   at <ScriptBlock>, <No file>:9
  [!] c 7ms (4ms|2ms)
  [!] d 6ms (4ms|2ms)
  [!] e is skipped, because demo skipped 12ms (9ms|2ms)
Tests completed in 229ms
Tests Passed: 1, Failed: 1, Skipped: 3 NotRun: 0

$r | Select-Object -Property Result, *Count

# output

Result                : Failed
FailedCount           : 1
FailedBlocksCount     : 0
FailedContainersCount : 0
PassedCount           : 1
SkippedCount          : 3
NotRunCount           : 0
TotalCount            : 5

$r.Tests | ForEach-Object {
    [pscustomobject]@{
       Name      = $_.Name
       Result    = $_.Result
       ShouldRun = $_.ShouldRun
       ErrorId   = $_.ErrorRecord.FullyQualifiedErrorId
    }
}

# output

Name      : a
Result    : Passed
ShouldRun : True
ErrorId   : 

Name      : b
Result    : Failed
ShouldRun : True
ErrorId   : PesterAssertionFailed

Name      : c
Result    : Skipped
ShouldRun : True
ErrorId   : PesterTestInconclusive

Name      : d
Result    : Skipped
ShouldRun : True
ErrorId   : PesterTestPending

Name      : e
Result    : Skipped
ShouldRun : True
ErrorId   : PesterTestSkipped

[csandfeld]

Skipped message output fixed in latest commit (2e73e47) added to PR #2401.

$r = invoke-pester -container ( new-pestercontainer -ScriptBlock {

        Describe "a" {
            It 'a' {
                $true | Should -BeTrue
            }

            It 'b' {
                $true | Should -BeFalse
            }

            It 'c' {
                Set-ItResult -Inconclusive -Because 'demo inconclusive'
            }

            It 'd' {
                Set-ItResult -Pending -Because 'demo pending'
            }

            It 'e' {
                Set-ItResult -Skipped -Because 'demo skipped'
            }
        }
    }) -passThru -Output Detailed

# output

Pester v5.5.0

Starting discovery in 1 files.
Discovery found 5 tests in 147ms.
Running tests.
Describing a
  [+] a 67ms (38ms|29ms)
  [-] b 41ms (37ms|4ms)
   Expected $false, but got $true.
   at $true | Should -BeFalse, :9
   at <ScriptBlock>, <No file>:9
  [!] c is skipped, because INCONCLUSIVE: demo inconclusive 18ms (15ms|3ms)
  [!] d is skipped, because PENDING: demo pending 5ms (3ms|2ms)
  [!] e is skipped, because demo skipped 14ms (7ms|7ms)
Tests completed in 701ms
Tests Passed: 1, Failed: 1, Skipped: 3 NotRun: 0

[fflaten]

I also use the "because" message in my reporting, and was surprised to see that additional data is injected in that message. It is a little counter-intuitive that the test author no longer has full control of what is in that message.

💯

[csandfeld]

@nohwnd, @fflaten - thank you for merging #2401 👍

I have looked into what changes are needed to bring back PendingCount and InconclusiveCount (plus Pending and Inconclusive generic lists) properties, and think I have a decent understanding of it by now.

Would you approve of me working on adding that? If yes, are there any issues I should be aware of that kept them out of v5?

[fflaten]

Since it's listed here I assume it was just pending some discussion. @nohwnd ?

Inclusive makes sense imo.

As for Pending, do we need it (yet)? Especially Set-ItResult -Pending is confusing to me. When would we use it?

Pending feels like a initial result for tests with ShouldRun=$true Executed=$False. If so, is it necessary at all in the summary and Set-ItResult? Or is it mostly useful once we start exposing the test state during runtime.

[csandfeld]

I'm wondering if we really need Pending. Isn't it the same as Executed=$False? Any obvious use cases it would solve?

The use case for Pending is likely the same as the -Pending switch on It: https://github.com/pester/Pester/blob/acc66a965219a8e70981977289227a211af4c70e/src/functions/It.ps1#L30

    -Pending [<SwitchParameter>]
        Use this parameter to explicitly mark the test as work-in-progress/not implemented/pending when you
        need to distinguish a test that fails because it is not finished yet from a tests
        that fail as a result of changes being made in the code base. An empty test, that is a
        test that contains nothing except whitespace or comments is marked as Pending by default.

You could argue that if It already has a -Pending switch, there would be no need for Set-ItResult to support it. But the Set-ItResult has the switch, it is just currently not hooked up to anything but the ErrorId change implemented in #2401. And if we opt to reimplement Inconclusive, it looks simple to add Pending as well while we are at it.

Another argument for reimplementing both, is a degree of increasedcbackwards compatibility.

[fflaten]

Fair point. That's what I get from using mobile app to comment (didn't check the code) 😄

Still, the result name is ambiguous and the same function could easily be achieved using Skip with a reason or comment. So does it warrant being a first-class result?

Just want to raise the discussion now considering it's easier to leave it out than removing later if it feels like clutter.

[csandfeld]

Just want to raise the discussion now considering it's easier to leave it out than removing later if it feels like clutter.

I agree

[johlju]

I never used Pending in either Pester 4 or tried to use it in Pester 5. I do use Skip a lot, for example when having PowerShell modules that is used cross-platform. Some code does not work on (mostly) Linux and macOS so tests testing that code are skipped when on incompatible platform. But I thought about if there were a scenario when Pending could be used and I could only think of one, if some test code are not finished then Pending would be better than Skip, but then why merge an unfinished test, what would the purpose be for that? I would not trust an unfinished test so I wouldn't merge it. I have not needed Pending in the work I do in the years I written Pester tests, not yet anyway. Would be curious in what scenarios Pending have been used.

[billgothacked]

nohwnd: Moved.

I’m not a developer I run a nonprofit and a Pester Power Shell is running on my laptop. How did that happen?

On Sat, Nov 11, 2023 at 12:19 PM Christian Sandfeld < @.***> wrote:

@nohwnd https://github.com/nohwnd, @fflaten https://github.com/fflaten

• thank you for merging #2401 https://github.com/pester/Pester/pull/2401 👍

I have looked into what changes are needed to bring back PendingCount and InconclusiveCount (plus Pending and Inconclusive generic lists) properties, and think I have a decent understanding of it by now.

Would you approve of me working on adding that? If yes, are there any issues I should be aware of that kept them out of v5?

— Reply to this email directly, view it on GitHub https://github.com/pester/Pester/issues/2400#issuecomment-1806896597, or unsubscribe https://github.com/notifications/unsubscribe-auth/BA7FZAWIJU3NP2M27ZV2Z3TYD7FT5AVCNFSM6AAAAAA6X55CE2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBWHA4TMNJZG4 . You are receiving this because you are subscribed to this thread.Message ID: @.***>

[nohwnd]

Pending was an idea I had, and briefly found useful only to never use it. So imho killing it off is the best. I would keep it as the -Pending parameter on Set-ItResult, and remove it in v6. It is supposed to be a temporary marker for tests in progress, so removing it should not be a big problem.

[nohwnd]

@csandfeld Yes, please work on adding Inconclusive count back. But leave out Pending.

[fflaten]

I would keep it as the -Pending parameter on Set-ItResult, and remove it in v6.

👍 Maybe add a deprecation notice at the end of the run if we detect it's been used.

[csandfeld]

@csandfeld Yes, please work on adding Inconclusive count back. But leave out Pending.

@nohwnd, my proposed implementation is in #2405. At your convenience please have a looked at it, and let me know if you see a need for additional changes etc.