Repository navigation
fix(command): give Aborted and NoTests an exit severity - #340
Merged
eddie-knight merged 4 commits intoNov 11, 2026
Merged
eddie-knight merged 4 commits into
eddie-knight merged 4 commits into
Conversation
exitSeverity had no entry for Aborted or NoTests, so both scored the map's zero value and merged as TestPass. A run whose only plugin aborted therefore exited 0. Aborted now ranks between TestFail and BadUsage. NoTests ranks just above TestPass, so a plugin that evaluated nothing never reads as a pass, while a real TestFail from another plugin still wins over it. A new test reads the constants from shared/exitcodes.go and fails if any of them has no entry or shares a severity with another. Fixes privateerproj#300 Signed-off-by: Yashvardhan Thanvi <[email protected]>
jmeridth
reviewed
Oct 25, 2026
jmeridth
left a comment
jmeridth
left a comment
Member
There was a problem hiding this comment.
🤖 (summary)
Thanks for taking this on, and for following the issue so closely. The ordering and the coverage test both look right. Two small comments inline, neither blocking. I'm fine with the NoTests placement as is.
A pluginkit plugin whose suites are all NotRun still exits TestPass, so the ranking only covers plugins that return NoTests themselves. Signed-off-by: Yashvardhan Thanvi <[email protected]>
The coverage test parsed only shared/exitcodes.go, so an exit code added to another file of the package would skip the check and merge as TestPass. It now type-checks the const declarations of every non-test file in shared. Signed-off-by: Yashvardhan Thanvi <[email protected]>
Signed-off-by: Eddie Knight <[email protected]>
Contributor
|
Hey @Thanatos9404 — Your test revealed that we had this logic in the wrong place, or rather that the old place wasn't well suited anymore. Hugely appreciate you doing the work to reveal that. I pushed a commit to this PR moving the location of the logic and adjusting a bit while staying true to the heart of your proposal here. Thanks! |
jmeridth
approved these changes
Nov 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
exitSeverity had no entry for Aborted or NoTests, so both scored zero in mergeExitCode and a run whose only plugin aborted exited 0.
Aborted now sits between TestFail and BadUsage, as the issue suggests. NoTests goes just above TestPass, so a plugin that returns NoTests never reads as a pass, while a real TestFail from another plugin still wins. Happy to move it higher if you prefer.
A new test reads the exit code constants from package shared and fails if any lacks an entry or shares a severity.
Fixes #300