-
-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: improve error handling in global fixtures #5208 #5275
base: main
Are you sure you want to change the base?
Conversation
All tests pass locally using npm run test-node:unit on both the main branch and my working branch. However, they fail in CI. This suggests an environment-specific issue. I'm investigating further and would appreciate any guidance on differences between the CI and local setups. |
Ah, this looks like #5278. Sorry for the confusing errors here - you can ignore them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A good start, thanks for sending this! I think there's some work to be done to flesh the PR out.
lib/mocha.js
Outdated
try { | ||
await fixtureFn.call(context); | ||
} catch (err) { | ||
console.error(err.stack); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Functionality] This is a good start, but I think we'll want to do more than just log the error to the console. I think we'll want to mark the test as failed too. It's easy to miss console logs that don't change the exit code.
Thanks for the review 🙏🏽. I'll make the necessary changes asap. |
improve format Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
👋 Just checking in @TG199, did you mean to re-request review? |
Oh, actually yes, had no idea I could request for a review, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whenever tests are added, feel free to re-request review and I can take a look :)
hi @JoshuaKGoldberg, need a little help here, I've been running into challenges trying to mark failed test. these are the different approaches I've tried: Mocha.prototype._runGlobalFixtures = async function _runGlobalFixtures( Second attempt - Using a fakeHook this.runner.failHook: // In _runGlobalFixtures Current attempt - Using a flag and run override: Mocha.prototype._runGlobalFixtures = async function _runGlobalFixtures( const originalRun = Mocha.prototype.run; What's the recommended way to access the runner or mark tests as failed from within _runGlobalFixtures? Any guidance would be greatly appreciated! Thank you! |
Glad to see you're making progress on this! A couple quick tips to help me help you... First, it's easier to read code snippets if you format code blocks as code. See https://docs.github.com/en/get-started/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntax#quoting-code. A lot easier to read 🙂 Second, it's hard to help from just some standalone code. Can you try pushing your attempt(s) up as Git branches or commits?
Generally, the Lines 1228 to 1231 in deb8679
I also found this this by:
I'm not sure but it seems reasonable to me to look there. Something to consider: |
I think I need to checkout Github docs more often
Oh, okay, I try this out.
True, I'll keep that in mind, infinite thanks! |
properly mark failed global fixtuers test when they fail.
Hi @JoshuaKGoldberg will this do? 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Progress, thanks for continuing on this! 🚀
@@ -1219,22 +1221,40 @@ Mocha.prototype.runGlobalTeardown = async function runGlobalTeardown( | |||
}; | |||
|
|||
/** | |||
* Run global fixtures sequentially with context `context` | |||
* Run global fixtures sequentially with context `context`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still some unrelated changes: a blank line above, this .
, an extra ,
later, etc. - https://www.joshuakgoldberg.com/blog/split-out-unrelated-changes -> https://www.joshuakgoldberg.com/blog/split-out-unrelated-changes/#separate-logical-and-stylistic-changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TG199 this piece of feedback is still open
const testFile = 'test/fixtures/global-teardown/test.js'; | ||
|
||
before(async function() { | ||
await fs.mkdir(path.dirname(setupFile), { recursive: true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Refactor] Err, most fixture files are checked in to the repository. This would be the first instance in the test/
directory of fs.mkdir
. -1 from me on this strategy of writing test files dynamically and then removing them after the tests are done. Please switch to having fixtures checked in as regular files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TG199 this piece of feedback is still open
lib/mocha.js
Outdated
try { | ||
await fixtureFn.call(context); | ||
} catch (err) { | ||
if(context.stats) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Question] When would context.stats
not exist?
I'm wondering if the parameters to the _runGlobalFixtures
function can be assumed to exist. I.e. that the = []
and = {}
are unnecessary. I don't see any way in code to have them not exist.
lib/mocha.js
Outdated
@@ -184,6 +184,7 @@ function Mocha(options = {}) { | |||
options = {...mocharc, ...options}; | |||
this.files = []; | |||
this.options = options; | |||
this.globalTeardownErrored = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Refactor] Unused code?
this.globalTeardownErrored = false; |
lib/mocha.js
Outdated
context.stats.failures++; | ||
context.failures++; | ||
|
||
const isSetup = this.options.globalSetup && this.options.globalSetup.includes(fixtureFn); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Refactor] Trying to query this
properties to determine the phase is a little awkward: it's several lines of code and relies on other implementation details.
_runGlobalFixtures
is called by places that know what phase they're in: runGlobalTeardown
and runGlobalSetup
. How about having them pass in the phase as an argument?
Tip: I'm trying to convey a request to please use the re-request review when you'd like to. I get a lot of notifications and this helps me keep them organized. 🙂 |
Ohh, understood. |
…s and use static fixtures Refactored tests to use fixture files instead of dynamically creating and deleting them.
err: err | ||
}); | ||
console.error(err); | ||
process.exit(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Bug] We can't manually process.exit(1)
ourselves. Someone might be calling the Mocha API manually and expect to get results returned back to them. If you want to mark all tests as failed, you'll have to find another way.
PR Checklist
status: accepting prs
Overview
Description
This PR addresses an issue with the handling of errors in global teardown functions. Previously, if a global teardown function threw an error, the error was not logged clearly, and the process exit code might not reflect the failure.
Changes Made
Updated the _runGlobalFixtures method in lib/mocha.js to:
Log errors with a clear and descriptive message.
Set the process exit code to 1 when a global teardown function fails.
Improved debugging output to aid in identifying which function caused the error.
New Behavior
When a global teardown function fails:
A descriptive error message is logged.
The process exits with a non-zero code, signaling the failure.