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 17 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 25 additions & 4 deletions lib/mocha.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;

// root suite
this.suite = new exports.Suite('', new exports.Context(), true);
this._cleanReferencesAfterRun = true;
Expand Down Expand Up @@ -987,6 +988,7 @@ Mocha.prototype.run = function (fn) {
if (this.files.length && !this._lazyLoadFiles) {
this.loadFiles();
}

var suite = this.suite;
var options = this.options;
options.files = this.files;
Expand Down Expand Up @@ -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

* @private
* @param {MochaGlobalFixture[]} [fixtureFns] - Fixtures to run
* @param {object} [context] - context object
* @returns {Promise<object>} context object
*/
Mocha.prototype._runGlobalFixtures = async function _runGlobalFixtures(
fixtureFns = [],
context = {}
context = {},
) {
for await (const fixtureFn of fixtureFns) {
await fixtureFn.call(context);
for (const fixtureFn of fixtureFns) {
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.

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?

const phase = isSetup ? 'Global Setup' : 'Global Teardown';

context.emit('fail', {
title: phase,
err: err
});
console.error(err);
process.exit(1);
}
}
}
return context;
};


/**
* Toggle execution of any global setup fixture(s)
*
Expand Down Expand Up @@ -1330,3 +1350,4 @@ Mocha.prototype.hasGlobalTeardownFixtures =
* @param {Array<*>} impls - User-supplied implementations
* @returns {Promise<*>|*}
*/

9 changes: 9 additions & 0 deletions test/integration/fixtures/global-teardown-error.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
'use strict'

const { it } = require('../../../lib/mocha');

it('should pass', () => {});

exports.mochaGlobalTeardown = async function () {
throw new Error('Teardown problem')
}
102 changes: 102 additions & 0 deletions test/integration/global-teardown-errors.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
'use strict'

const assert = require('assert');
const path = require('path');
const { spawnSync } = require('child_process');
const fs = require('fs').promises;

describe('Global Teardown Error Handling', function() {
this.timeout(5000);

const setupFile = 'test/fixtures/global-teardown/setup.js';
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


await fs.writeFile(setupFile, `
exports.mochaGlobalTeardown = async function () {
throw new Error('Teardown failure');
};
`);

await fs.writeFile(testFile, `
describe('Test Suite', function() {
it('passing test', function() {
// This test passes
});
});
`);
});

after(async function() {
await fs.rm(path.dirname(setupFile), { recursive: true, force: true });
});

it('should fail with non-zero exit code when global teardown fails', function() {
const result = spawnSync('node', [
'bin/mocha',
'--require', setupFile,
testFile
], {
encoding: 'utf8'
});

assert.strictEqual(result.status, 1, 'Process should exit with code 1');

assert(result.stderr.includes('Teardown failure') ||
result.stdout.includes('Teardown failure'),
'Should show teardown error message');
});

it('should combine test failures with teardown failures', async function() {

await fs.writeFile(testFile, `
describe('Test Suite', function() {
it('failing test', function() {
throw new Error('Test failure');
});
});
`);

const result = spawnSync('node', [
'bin/mocha',
'--require', setupFile,
testFile
], {
encoding: 'utf8'
});

assert.strictEqual(result.status, 1, 'Process should exit with code 1');

const output = result.stdout + result.stderr;
assert(output.includes('Test failure'), 'Should show test error');
assert(output.includes('Teardown failure'), 'Should show teardown error');
});

it('should pass with zero exit code when no errors occur', async function() {
await fs.writeFile(setupFile, `
exports.mochaGlobalTeardown = async function () {
// Success case
};
`);

await fs.writeFile(testFile, `
describe('Test Suite', function() {
it('passing test', function() {
// This test passes
});
});
`);

const result = spawnSync('node', [
'bin/mocha',
'--require', setupFile,
testFile
], {
encoding: 'utf8'
});

assert.strictEqual(result.status, 0, 'Process should exit with code 0');
});
});