prevent poisoned diffs from suppressing non-zero exits #5998
prevent poisoned diffs from suppressing non-zero exits #5998mostafaNazari702 wants to merge 2 commits into
Conversation
|
I used some questionable terminologies such as reslience but i want you guys to evaluate, your opinions matter as well. I just couldn't find a better word to have instead there. |
mostafaNazari702
changed the title
|
So i while i was typing "Added a space in the end of the PR title to re-run tests", the error to tests even after re-running, appeared...Can you guys tell if that is an issue caused on my side? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5998 +/- ##
==========================================
+ Coverage 82.64% 82.69% +0.05%
==========================================
Files 62 62
Lines 4668 4682 +14
Branches 1003 1006 +3
==========================================
+ Hits 3858 3872 +14
Misses 810 810 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
JoshuaKGoldberg
left a comment
JoshuaKGoldberg
left a comment
There was a problem hiding this comment.
@mostafaNazari702 this has lint failures, I'll wait to review until they're fixed.
https://github.com/mochajs/mocha/actions/runs/26314852116/job/77471707014?pr=5998
/home/runner/work/mocha/mocha/lib/reporters/base.js
Error: 106:17 error Empty block statement no-empty
Error: 110:17 error Empty block statement no-empty
Error: 490:15 error Empty block statement no-empty
JoshuaKGoldberg
added
the
status: waiting for author
6684a34 to
c657e57
Compare
JoshuaKGoldberg
removed
the
status: waiting for author
JoshuaKGoldberg
left a comment
JoshuaKGoldberg
left a comment
There was a problem hiding this comment.
Yup, this looks good to me. It's hard to verify the kind of defensive coding the issue requires but I think you got it all right. Just the one test for cleanup and I think we'd be good to go! (not including any other maintainer review)
| it("should exit non-zero when the reporter throws while handling a failure", function () { | ||
| expect(res, "to have failed"); | ||
| }); | ||
|
|
||
| it("should warn that the reporter threw instead of crashing", function () { | ||
| expect( | ||
| res.output, | ||
| "to match", | ||
| /\[mocha\] (reporter error|failed to render)/, | ||
| ); | ||
| }); | ||
|
|
||
| it("should report the failure exactly once, not double-count the reporter throw", function () { | ||
| expect(res.output, "to match", /1 failing/); | ||
| expect(res.output, "not to contain", "2 failing"); | ||
| }); |
There was a problem hiding this comment.
[Testing] These are all asserting on various aspects of a single result. That's just one unit under test. Please combine these into one unit test.
JoshuaKGoldberg
added
the
status: waiting for author
c657e57 to
b8ee8f0
Compare
github-actions
Bot
removed
the
status: waiting for author
b8ee8f0 to
87de075
Compare
2 participants
PR Checklist
status: accepting prsOverview
This PR is the last of the wave. there was was a broader issue behind #5505/#5506 where reporter-side errors could cause nocha to exit with status 0 even though the run actually failed.
if the Base reporter threw while handling a failed test. either during EVENT_TEST_FAIL or later in Base.list during the epilougue , the exception could break mocha's internal runner flow before the final failure count was reported to the CLI. The run looked failed in output, but the process still exited successfully.
the original repro using Object.create(null) was fixed already but i guess the underlying issue still existed for anything that could make utils.stringify or diff rendering throw (hostile proxies, throwing getters, weird objects, etc......).
we now got defensive handling in the lib/reporters/base.js file at three levels, stringifyDiffObjs now safely falls back if object stringification throws. the EVENT_TEST_FAIL listener is wrapped so reporter errors are logged to stderrr instead of breaking the run. Base.list now isolates rendering failures per test so one broken failure output doesn’t abort the entire epilogue