Skip to content
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

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

TG199
Copy link
Contributor

@TG199 TG199 commented Dec 19, 2024

PR Checklist

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
Copy link
Contributor Author

TG199 commented Dec 19, 2024

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
Copy link
Member

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.

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a 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);
Copy link
Member

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.

@JoshuaKGoldberg JoshuaKGoldberg added the status: waiting for author waiting on response from OP - more information needed label Jan 2, 2025
@TG199
Copy link
Contributor Author

TG199 commented Jan 2, 2025

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

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

Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
@JoshuaKGoldberg
Copy link
Member

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

@TG199
Copy link
Contributor Author

TG199 commented Jan 29, 2025

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

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

@JoshuaKGoldberg JoshuaKGoldberg left a 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 :)

@JoshuaKGoldberg JoshuaKGoldberg added status: waiting for author waiting on response from OP - 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
- Add globalTeardownErrored flag to track teardown failures
- Override Mocha.run to combine test failures with teardown errors
@TG199
Copy link
Contributor Author

TG199 commented Feb 6, 2025

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
Copy link
Member

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:

  1. 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
  2. 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
Copy link
Contributor Author

TG199 commented Feb 7, 2025

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:

  1. 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
  2. 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!

TG199 added 2 commits February 7, 2025 22:28
properly mark failed global fixtuers test when they fail.
@TG199
Copy link
Contributor Author

TG199 commented Feb 7, 2025

Hi @JoshuaKGoldberg will this do? 🙂

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a 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`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

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 });
Copy link
Member

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.

Copy link
Member

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) {
Copy link
Member

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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Refactor] Unused code?

Suggested change
this.globalTeardownErrored = false;

lib/mocha.js Outdated
context.stats.failures++;
context.failures++;

const isSetup = this.options.globalSetup && this.options.globalSetup.includes(fixtureFn);
Copy link
Member

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?

@JoshuaKGoldberg
Copy link
Member

👋 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
Copy link
Contributor Author

TG199 commented Feb 16, 2025

Ohh, understood.

err: err
});
console.error(err);
process.exit(1);
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting for author waiting on response from OP - more information needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 Bug: Error in mochaGlobalTeardown is swallowed and exits with code 0
2 participants