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

[csandfeld]

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

[nohwnd]

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

[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.

[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

[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.

[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.

[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}

[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:

Pester/src/functions/Output.ps1

Line 268 in a50354e

if (@('PesterAssertionFailed', 'PesterTestSkipped', 'PesterTestInconclusive', 'PesterTestPending') -contains $ErrorRecord.FullyQualifiedErrorID) {

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]

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:

Pester/src/functions/It.ps1

Line 30 in acc66a9

.PARAMETER Pending

    -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

[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 <#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 <#2400 (comment)>, 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.

[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.