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

Watch for test files are crashed #4614

Merged
merged 2 commits into from
Apr 25, 2021

Conversation

outsideris
Copy link
Contributor

Description of the Change

In watch mode, the watcher crashed when there are syntax errors in test files during require('test file').
It only happens in serial mode and Node v15+.

This try - catch statement removed when we introduce parallel mode.
I think we need try - catch here.

And I added test cases for the crash case to prevent regression.

Why should this be in core?

Watching should be worked.

Benefits

Users can use watch mode when the test code is broken.

Applicable issues

Fix #4580

@@ -470,6 +470,7 @@ async function runMochaWatchJSONAsync(args, opts, change) {
// eslint-disable-next-line no-control-regex
.replace(/\u001b\[\?25./g, '')
.split('\u001b[2K')
.filter(x => x)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I filtered output because the output is an empty string('') when a test file is crashed.

Copy link
Contributor

@juergba juergba left a comment

Choose a reason for hiding this comment

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

Your bug fix lgtm, but the tests are a little weird.

test/integration/options/watch.spec.js Outdated Show resolved Hide resolved
test/integration/options/watch.spec.js Outdated Show resolved Hide resolved
test/integration/options/watch.spec.js Outdated Show resolved Hide resolved
@juergba juergba added type: bug a defect, confirmed by a maintainer area: node.js command-line-or-Node.js-specific semver-patch implementation requires increase of "patch" version number; "bug fixes" labels Apr 9, 2021
@juergba juergba added this to the next milestone Apr 9, 2021
@outsideris outsideris force-pushed the fix-watch-crash-case branch from f0f7458 to b6b4eef Compare April 25, 2021 09:22
@outsideris
Copy link
Contributor Author

Fixed

@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 94.204% when pulling b6b4eef on outsideris:fix-watch-crash-case into 34643e4 on mochajs:master.

@juergba juergba added the run-browser-test run browser tests on forked PR if code is safe label Apr 25, 2021
@github-actions github-actions bot removed the run-browser-test run browser tests on forked PR if code is safe label Apr 25, 2021
@juergba juergba merged commit 8285910 into mochajs:master Apr 25, 2021
Copy link
Contributor

@juergba juergba left a comment

Choose a reason for hiding this comment

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

I forgot to approve this PR. Yes, approved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: node.js command-line-or-Node.js-specific semver-patch implementation requires increase of "patch" version number; "bug fixes" type: bug a defect, confirmed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Watcher crashes when reloading code that throws (regression mocha@7 to mocha@8)
3 participants