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.

Sign up for GitHub

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

Jump to bottom

Fix: wrong error thrown while loading reporter #4842

Merged
merged 3 commits into from
Mar 11, 2022
Merged

Fix: wrong error thrown while loading reporter #4842

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
73f547a
fix: wrong error thrown while loading reporter
juergba Mar 9, 2022
cb5399c
tests
juergba Mar 9, 2022
2aafdb3
update webpack config
juergba Mar 9, 2022
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
20 changes: 10 additions & 10 deletions lib/cli/run-helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,18 +225,18 @@ exports.validateLegacyPlugin = (opts, pluginType, map = {}) => {

// if this exists, then it's already loaded, so nothing more to do.
if (!map[pluginId]) {
let foundId;
try {
map[pluginId] = require(pluginId);
foundId = require.resolve(pluginId);
map[pluginId] = require(foundId);
} catch (err) {
if (err.code === 'MODULE_NOT_FOUND') {
// Try to load reporters from a path (absolute or relative)
try {
map[pluginId] = require(path.resolve(pluginId));
} catch (err) {
throw createUnknownError(err);
}
} else {
throw createUnknownError(err);
if (foundId) throw createUnknownError(err);

// Try to load reporters from a cwd-relative path
try {
map[pluginId] = require(path.resolve(pluginId));
} catch (e) {
throw createUnknownError(e);
}
}
}
Expand Down
34 changes: 12 additions & 22 deletions lib/mocha.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ var Suite = require('./suite');
var esmUtils = require('./nodejs/esm-utils');
var createStatsCollector = require('./stats-collector');
const {
warn,
createInvalidReporterError,
createInvalidInterfaceError,
createMochaInstanceAlreadyDisposedError,
Expand Down Expand Up @@ -335,35 +334,26 @@ Mocha.prototype.reporter = function (reporterName, reporterOptions) {
}
// Try to load reporters from process.cwd() and node_modules
if (!reporter) {
let foundReporter;
try {
reporter = require(reporterName);
foundReporter = require.resolve(reporterName);
Copy link
Contributor

Choose a reason for hiding this comment

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

@juergba this line caused a warning when trying to bundle mocha/mocha.js using webpack

Copy link
Contributor Author

@juergba juergba Mar 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@AviVahl This one: Critical dependency: the request of a dependency is an expression?
Is this a problem? I need to check wether the module exists, and I can't use a fs function either.
In a browser environment we shouldn't get up to this line anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, exactly that one.
The warning actually broke several internal projects we have that bundle mocha into the browser. There was infra code there that verified no errors/warnings from bundling.

If the branch is node-specific, would it be possible to completely separate it out of the browser bundle?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How can I achieve this, exclude a specific code block from the bundle?

Btw the transpiled ES5 bundle (mocha.js) will be dropped in a few weeks, and the ES2018 bundle (mocha-es2018.js renamed to mocha.js) will be the one and only. If you have any input, would be welcome.

Copy link
Contributor

@AviVahl AviVahl Mar 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I can think of several possible ways to achieve this.

I usually separate the isomorphic parts of code from the Node-specific parts.
So: the cli, config loading, reporter loading, dynamic require/require.resolve, etc would all belong to the Node-specific part.
The mocha.js bundle would only include the runner itself and associated APIs.

Another, perhaps trickier, way to achieve this is to specify "browser": { "./my-node-specific-file.js": null} which would cause bundlers to skip that file and inject an empty object instead. Runtime checks would be needed to protect against the replaced API.

Regarding es2018? My team welcomes this change. :)

Copy link

Choose a reason for hiding this comment

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

This issue is present even now in v10. Have you found a solution @AviVahl ? I can supress these errors in webpack config but it doesn't seem like the right solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

We've stopped bundling mocha from our end because of the many bundling regressions it exhibited across the years/versions. Our tool (mocha-play) uses the prebuilt bundle (in the npm package) and injects it to the page.

reporter = require(foundReporter);
} catch (err) {
if (err.code === 'MODULE_NOT_FOUND') {
// Try to load reporters from a path (absolute or relative)
try {
reporter = require(path.resolve(utils.cwd(), reporterName));
} catch (_err) {
_err.code === 'MODULE_NOT_FOUND'
? warn(`'${reporterName}' reporter not found`)
: warn(
`'${reporterName}' reporter blew up with error:\n ${err.stack}`
);
}
} else {
warn(`'${reporterName}' reporter blew up with error:\n ${err.stack}`);
if (foundReporter) {
throw createInvalidReporterError(err.message, foundReporter);
}
// Try to load reporters from a cwd-relative path
try {
reporter = require(path.resolve(reporterName));
} catch (e) {
throw createInvalidReporterError(e.message, reporterName);
}
}
}
if (!reporter) {
throw createInvalidReporterError(
`invalid reporter '${reporterName}'`,
reporterName
);
}
this._reporter = reporter;
}
this.options.reporterOption = reporterOptions;
// alias option name is used in public reporters xunit/tap/progress
// alias option name is used in built-in reporters xunit/tap/progress
this.options.reporterOptions = reporterOptions;
return this;
};
Expand Down
2 changes: 1 addition & 1 deletion test/browser-specific/fixtures/webpack/webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ module.exports = {
plugins: [
new FailOnErrorsPlugin({
failOnErrors: true,
failOnWarnings: true
failOnWarnings: false
})
]
};
17 changes: 17 additions & 0 deletions test/node-unit/cli/run-helpers.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,23 @@ describe('helpers', function () {
{message: /wonky/, code: 'ERR_MOCHA_INVALID_REPORTER'}
);
});

it('should fail and report the original "MODULE_NOT_FOUND" error.message', function () {
expect(
() =>
validateLegacyPlugin(
{
reporter: require.resolve('./fixtures/bad-require.fixture.js')
},
'reporter'
),
'to throw',
{
message: /Error: Cannot find module 'fake'/,
code: 'ERR_MOCHA_INVALID_REPORTER'
}
);
});
});
});

Expand Down
32 changes: 3 additions & 29 deletions test/node-unit/mocha.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ describe('Mocha', function () {

it('should load from current working directory', function () {
expect(function () {
mocha.reporter('./spec.js');
mocha.reporter('./lib/reporters/spec.js');
}, 'not to throw');
});

Expand All @@ -255,7 +255,7 @@ describe('Mocha', function () {
expect(
function () {
mocha.reporter(
'../../test/node-unit/fixtures/wonky-reporter.fixture.js'
'./test/node-unit/fixtures/wonky-reporter.fixture.js'
);
},
'to throw',
Expand All @@ -264,19 +264,6 @@ describe('Mocha', function () {
}
);
});

it('should warn about the error before throwing', function () {
try {
mocha.reporter(
'../../test/node-unit/fixtures/wonky-reporter.fixture.js'
);
} catch (ignored) {
} finally {
expect(stubs.errors.warn, 'to have a call satisfying', [
expect.it('to match', /reporter blew up/)
]);
}
});
});
});

Expand All @@ -292,7 +279,7 @@ describe('Mocha', function () {
expect(
function () {
mocha.reporter(
'./test/node-unit/fixtures/wonky-reporter.fixture.js'
'../test/node-unit/fixtures/wonky-reporter.fixture.js'
);
},
'to throw',
Expand All @@ -301,19 +288,6 @@ describe('Mocha', function () {
}
);
});

it('should warn about the error before throwing', function () {
try {
mocha.reporter(
'./test/node-unit/fixtures/wonky-reporter.fixture.js'
);
} catch (ignored) {
} finally {
expect(stubs.errors.warn, 'to have a call satisfying', [
expect.it('to match', /reporter blew up/)
]);
}
});
});
});
});
Expand Down