fix: improve error handling in global fixtures #5208 by TG199 · Pull Request #5275 · mochajs/mocha

TG199 · 2024-12-19T15:27:49Z

PR Checklist

Addresses an existing open issue: fixes 🐛 Bug: Error in mochaGlobalTeardown is swallowed and exits with code 0 #5208
That issue was marked as status: accepting prs
Steps in CONTRIBUTING.md were taken

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.

TG199 · 2024-12-19T17:13:17Z

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.

JoshuaKGoldberg · 2025-01-02T05:24:45Z

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.

JoshuaKGoldberg

A good start, thanks for sending this! I think there's some work to be done to flesh the PR out.

JoshuaKGoldberg · 2025-01-02T05:27:05Z

+    try {
+      await fixtureFn.call(context);
+    } catch (err) {
+      console.error(err.stack);


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

TG199 · 2025-01-02T18:21:43Z

Thanks for the review 🙏🏽. I'll make the necessary changes asap.

improve format Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>

JoshuaKGoldberg · 2025-01-28T20:05:20Z

👋 Just checking in @TG199, did you mean to re-request review?

TG199 · 2025-01-29T08:30:17Z

Oh, actually yes, had no idea I could request for a review, thanks!

JoshuaKGoldberg

Whenever tests are added, feel free to re-request review and I can take a look :)

- Add globalTeardownErrored flag to track teardown failures - Override Mocha.run to combine test failures with teardown errors

TG199 · 2025-02-06T08:18:24Z

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:
First attempt - Trying to use runner.failures:

Mocha.prototype._runGlobalFixtures = async function _runGlobalFixtures(
fixtureFns = [],
context = {}
) {
try {
await fixtureFn.call(context);
} catch (err) {
console.error(err.stack);
if (this.runner) {
this.runner.failures++;
}
throw err;
}
}
However, this.runner isn't available in this context

Second attempt - Using a fakeHook this.runner.failHook:

// In _runGlobalFixtures
if (this.runner) {
const falseHook = new this._Hook("globalTeardown", "global teardown");
this.runner.failHook(falseHook, err);
}
this is when I noticed this.runner is not available here. !!this.runner (logged: false)

Current attempt - Using a flag and run override:

Mocha.prototype._runGlobalFixtures = async function _runGlobalFixtures(
fixtureFns = [],
context = {},
) {
for (const fixtureFn of fixtureFns) {
try {
await fixtureFn.call(context);
} catch (err) {
console.error('Global fixture error:', err.stack);
this.globalTeardownErrored = true;
}
}
return context;
};

const originalRun = Mocha.prototype.run;
Mocha.prototype.run = function (fn) {
const self = this;
return originalRun.call(this, (exitCode) => {
const finalExitCode = exitCode > 0 || self.globalTeardownErrored ? 1 : 0;
if (fn) fn(finalExitCode);
});
};
This works for getting the exit code right, but I'm not sure if this is the best approach for integrating with Mocha's test reporting.
Questions:

What's the recommended way to access the runner or mark tests as failed from within _runGlobalFixtures?
Is there a better way to track these failures that would integrate better with Mocha's reporting system?
Should I be handling this at a different point in Mocha

Any guidance would be greatly appreciated! Thank you!

JoshuaKGoldberg · 2025-02-06T13:12:54Z

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?

What's the recommended way to access the runner or mark tests as failed from within _runGlobalFixtures?

Generally, the context parameter that it takes in. Although the docs on https://mochajs.org/api/mocha#globalTeardown -> https://mochajs.org/api/global#MochaGlobalFixture don't mention a parameter, you can see in code that it's given one:

mocha/lib/mocha.js

Lines 1228 to 1231 in deb8679

    
           Mocha.prototype._runGlobalFixtures = async function _runGlobalFixtures( 
        
             fixtureFns = [], 
        
             context = {} 
        
           ) {

I also found this this by:

Putting a console.log(new Error().stack) in the global-fixture example's mochaGlobalTeardown: https://github.com/mochajs/mocha-examples/blob/48f723f4a6f28870ae1c49cffb74d228786b11e6/packages/global-fixture/fixture.js#L27: to see that it's called by mocha/lib/mocha.js:1234
Putting a console.log(context) in `mocha/lib/mocha.js:1234: to see what's in the context

Is there a better way to track these failures that would integrate better with Mocha's reporting system?
Should I be handling this at a different point in Mocha

I'm not sure but it seems reasonable to me to look there. runGlobalTeardown -> _runGlobalFixtures is the main pair of functions that calls the teardown fixtures. Either they or something that calls them would be a good place to try adding the logic.

Something to consider: runGlobalTeardown is specific to teardown; _runGlobalFixtures is for both setup and teardown. Maybe that's a hint we should also handle setup functions too? What if they error?

TG199 · 2025-02-07T00:52:49Z

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 🙂

I think I need to checkout Github docs more often

Second, it's hard to help from just some standalone code. Can you try pushing your attempt(s) up as Git branches or commits?

What's the recommended way to access the runner or mark tests as failed from within _runGlobalFixtures?

Generally, the context parameter that it takes in. Although the docs on https://mochajs.org/api/mocha#globalTeardown -> https://mochajs.org/api/global#MochaGlobalFixture don't mention a parameter, you can see in code that it's given one:

mocha/lib/mocha.js

Lines 1228 to 1231 in deb8679

Mocha.prototype._runGlobalFixtures = async function _runGlobalFixtures(

fixtureFns = [],

context = {}

) {

I also found this this by:

Putting a console.log(new Error().stack) in the global-fixture example's mochaGlobalTeardown: https://github.com/mochajs/mocha-examples/blob/48f723f4a6f28870ae1c49cffb74d228786b11e6/packages/global-fixture/fixture.js#L27: to see that it's called by mocha/lib/mocha.js:1234

Putting a console.log(context) in `mocha/lib/mocha.js:1234: to see what's in the context

Oh, okay, I try this out.

Is there a better way to track these failures that would integrate better with Mocha's reporting system?
Should I be handling this at a different point in Mocha

I'm not sure but it seems reasonable to me to look there. runGlobalTeardown -> _runGlobalFixtures is the main pair of functions that calls the teardown fixtures. Either they or something that calls them would be a good place to try adding the logic.

Something to consider: runGlobalTeardown is specific to teardown; _runGlobalFixtures is for both setup and teardown. Maybe that's a hint we should also handle setup functions too? What if they error?

True, I'll keep that in mind, infinite thanks!

properly mark failed global fixtuers test when they fail.

TG199 · 2025-02-07T21:54:41Z

Hi @JoshuaKGoldberg will this do? 🙂

JoshuaKGoldberg

Progress, thanks for continuing on this! 🚀

JoshuaKGoldberg · 2025-02-16T22:00:59Z


 /**
- * Run global fixtures sequentially with context `context`
+ * Run global fixtures sequentially with context `context`.


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

@TG199 this piece of feedback is still open

JoshuaKGoldberg · 2025-02-16T22:04:09Z

+  const testFile = 'test/fixtures/global-teardown/test.js';
+
+  before(async function() {
+    await fs.mkdir(path.dirname(setupFile), { recursive: true });


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

@TG199 this piece of feedback is still open

JoshuaKGoldberg · 2025-02-16T22:05:11Z

+    try {
+      await fixtureFn.call(context);
+    } catch (err) {
+      if(context.stats) {


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

JoshuaKGoldberg · 2025-02-16T22:05:24Z

  options = {...mocharc, ...options};
  this.files = [];
  this.options = options;
+  this.globalTeardownErrored = false;


[Refactor] Unused code?

Suggested change

this.globalTeardownErrored = false;

JoshuaKGoldberg · 2025-02-16T22:09:20Z

+        context.stats.failures++;
+        context.failures++;
+
+        const isSetup = this.options.globalSetup && this.options.globalSetup.includes(fixtureFn);


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

JoshuaKGoldberg · 2025-02-16T22:10:22Z

👋 Just checking in @TG199, did you mean to re-request review?

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

TG199 · 2025-02-16T22:16:14Z

Ohh, understood.

…s and use static fixtures Refactored tests to use fixture files instead of dynamically creating and deleting them.

JoshuaKGoldberg · 2025-02-20T15:25:44Z

+        err: err
+      });
+      console.error(err);
+      process.exit(1);


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

JoshuaKGoldberg · 2025-04-07T17:37:58Z

@TG199 did you mean to re-request review?

mark-wiemer

Same changes requested as Josh, it's just those two active comments!

TG199 added 2 commits December 19, 2024 16:19

fix: improve error handling in global fixtures mochajs#5208

b9874b5

remove test file

bc63cb0

JoshuaKGoldberg mentioned this pull request Jan 2, 2025

🛠 Repo: Failures on main for test-node-lts: test-node:unit and test-node:integration #5278

Closed

5 tasks

JoshuaKGoldberg requested changes Jan 2, 2025

View reviewed changes

JoshuaKGoldberg added the status: waiting for author waiting on response from OP or other posters - more information needed label Jan 2, 2025

TG199 and others added 2 commits January 3, 2025 13:03

Update lib/mocha.js

f0d9524

improve format Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>

mark failed test

ae2c3a3

JoshuaKGoldberg added status: needs review a maintainer should (re-)review this pull request and removed status: waiting for author waiting on response from OP or other posters - more information needed labels Jan 30, 2025

JoshuaKGoldberg requested changes Jan 30, 2025

View reviewed changes

Comment thread lib/mocha.js

JoshuaKGoldberg added status: waiting for author waiting on response from OP or other posters - more information needed and removed status: needs review a maintainer should (re-)review this pull request labels Jan 30, 2025

TG199 added 6 commits January 31, 2025 23:04

create false hook to represent global teardown failure

2319487

Add test

4649a6a

correct assertion

041536d

fix test file path, correct global teardown type

d6ff7cc

Changes:

b00630c

- Add globalTeardownErrored flag to track teardown failures - Override Mocha.run to combine test failures with teardown errors

Add tests

848f6a7

TG199 added 2 commits February 7, 2025 22:28

Mark failed test:

2731673

properly mark failed global fixtuers test when they fail.

exit with code 1

38dfe9b

JoshuaKGoldberg requested changes Feb 16, 2025

View reviewed changes

TG199 added 4 commits February 19, 2025 09:43

refactor: pass 'phase' as an argument to _runGlobalFixtures

a026f9d

chore: document phase parameters for _runGlobalFixtures

f8ddf9c

fix: correct test expectations for _runGlobalFixtures in mocha.spec.js

6c380e5

test: rename global-teardown-errors.spec.js to global-fixtures.spec.j…

a22ea8a

…s and use static fixtures Refactored tests to use fixture files instead of dynamically creating and deleting them.

TG199 requested a review from JoshuaKGoldberg February 20, 2025 11:01

JoshuaKGoldberg reviewed Feb 20, 2025

View reviewed changes

refactor: throw global setup errors when running globalfixtures

6628f7f

mark-wiemer self-requested a review April 9, 2025 15:38

mark-wiemer requested changes Apr 10, 2025

View reviewed changes

JoshuaKGoldberg closed this May 6, 2025

Uh oh!

Conversation

TG199 commented Dec 19, 2024

PR Checklist

Overview

Uh oh!

TG199 commented Dec 19, 2024

Uh oh!

JoshuaKGoldberg commented Jan 2, 2025

Uh oh!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TG199 commented Jan 2, 2025

Uh oh!

JoshuaKGoldberg commented Jan 28, 2025

Uh oh!

TG199 commented Jan 29, 2025

Uh oh!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TG199 commented Feb 6, 2025

Uh oh!

JoshuaKGoldberg commented Feb 6, 2025

Uh oh!

TG199 commented Feb 7, 2025

Uh oh!

TG199 commented Feb 7, 2025

Uh oh!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoshuaKGoldberg commented Feb 16, 2025

Uh oh!

TG199 commented Feb 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoshuaKGoldberg commented Apr 7, 2025

Uh oh!

mark-wiemer left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants