Also fail on Pester block/container failures (not just failed tests)#663
Also fail on Pester block/container failures (not just failed tests)#663nohwnd wants to merge 1 commit into
Conversation
3da9f75 to
1c3d676
Compare
katriendg
left a comment
There was a problem hiding this comment.
Thank you for picking this one up and improving edge-ai! The fix is the right call.
Things are looking good and almost ready for merged:
- Please ensure you use the
.github/PULL_REQUEST_TEMPLATE.mdas the PR description content. - Three inline comments to address consistency follow-ups, the gate and the markdown table were updated, but a few sibling outputs are still keyed on
FailedCountalone.
| Set-CIOutput -Name 'test-result' -Value $(if ($result.FailedCount -eq 0) { 'passed' } else { 'failed' }) | ||
| Set-CIOutput -Name 'test-result' -Value $(if (($result.FailedCount + $result.FailedBlocksCount + $result.FailedContainersCount) -eq 0) { 'passed' } else { 'failed' }) | ||
| Set-CIOutput -Name 'test-count' -Value $result.TotalCount.ToString() | ||
| Set-CIOutput -Name 'fail-count' -Value $result.FailedCount.ToString() |
There was a problem hiding this comment.
Consistency (non-blocking): fail-count still reports only FailedCount. With the new gate above, a run can now emit test-result = 'failed' while fail-count = 0, which is contradictory for any consumer of these outputs. Suggest summing all three counters (or reporting them separately):
Set-CIOutput -Name 'fail-count' -Value (($result.FailedCount + $result.FailedBlocksCount + $result.FailedContainersCount)).ToString()There was a problem hiding this comment.
Pls repost the suggestions as actual suggestions and not just code blocks so they can be applied.
There was a problem hiding this comment.
Something like this?
| Set-CIOutput -Name 'fail-count' -Value $result.FailedCount.ToString() | |
| Set-CIOutput -Name 'fail-count' -Value (($result.FailedCount + $result.FailedBlocksCount + $result.FailedContainersCount)).ToString() |
@katriendg @WilliamBerryiii @bindsi @agreaves-ms Have we considered suggestion blocks instead of code-blocks where applicable for the PR Review and/or Code Review agents?
| | Failed Blocks | $($result.FailedBlocksCount) | | ||
| | Failed Containers | $($result.FailedContainersCount) | |
There was a problem hiding this comment.
Consistency (non-blocking) — relates to test-summary.json generation just above this hunk (lines 78-79).
The markdown table now surfaces FailedBlocksCount / FailedContainersCount, but the persisted test-summary.json still selects only FailedCount, so the uploaded artifact disagrees with this table and the gate. Consider adding the two new counters there as well:
$result | Select-Object TotalCount, PassedCount, FailedCount, FailedBlocksCount, FailedContainersCount, SkippedCount, Duration |
ConvertTo-Json | Set-Content (Join-Path $OutputPath 'test-summary.json')There was a problem hiding this comment.
Pls repost the suggestions as actual suggestions and not just code blocks so they can be applied.
| if ($CI -and ($result.FailedCount + $result.FailedBlocksCount + $result.FailedContainersCount) -gt 0) { | ||
| exit 1 |
There was a problem hiding this comment.
Reliability / developer UX (non-blocking) — relates to Get-FailedTest and the annotation loop above (lines 52-71, 83-86).
Now that the build fails on block/container failures, there's a gap: Get-FailedTest only walks It results, so a failure that trips this new exit 1 (a throwing BeforeAll/AfterAll, or a container that fails discovery/load) produces no Write-CIAnnotation — the developer sees a red build with only the summary counts and no inline pointer to the cause. Consider annotating failed containers so the new failure modes are actionable, e.g.:
foreach ($container in $result.Containers) {
if ($container.Result -eq 'Failed') {
Write-CIAnnotation -Level 'Error' `
-Message "Container failed to run: $($container.Item) — $($container.ErrorRecord.Exception.Message)" `
-File $container.Item
}
}There was a problem hiding this comment.
Pls repost the suggestions as actual suggestions and not just code blocks so they can be applied.
tapheret2
left a comment
There was a problem hiding this comment.
Review pass
Static review of the open diff:
- Looks focused enough for a community review pass
- Please ensure tests/docs match any behavior change
- Call out breaking changes in the PR body if any
Thanks — re-submitted after rate-limit cooldown.
I've been seeing this commonly across Pester 5 build scripts and I'm fixing it where it looks like it could hide real failures.
In Pester 5 a run reports three independent failure counts:
FailedCount— failed tests (It)FailedBlocksCount— failedBeforeAll/AfterAllFailedContainersCount— files that error during discovery / fail to loadGating on only
FailedCountmeans aBeforeAllthat throws, or a test file that fails to load, leavesFailedCount = 0and the build passes green despite real failures. Pester itself derives the overall pass/fail from the sum of all three.This includes all three counts in the gate, and surfaces each count (failed tests, blocks, containers) in the reported output instead of only the failed test count.