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 a number of causes of unhandled rejections (and ensure tests fail when unhandled rejection occurs). #9070

Merged
merged 3 commits into from Feb 29, 2020

Conversation

stefanpenner
Copy link
Contributor

@stefanpenner stefanpenner commented Feb 28, 2020

prep work to address #9007

  • fix all failures

tests/runner.js Outdated Show resolved Hide resolved
tests/unit/tasks/server/middleware/tests-server-test.js Outdated Show resolved Hide resolved
@stefanpenner stefanpenner changed the title WIP fail tests on unhandled rejections fail tests on unhandled rejections Feb 29, 2020
Copy link
Member

@mike-north mike-north left a comment

Choose a reason for hiding this comment

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

LGTM

lib/utilities/open-editor.js Show resolved Hide resolved
Comment on lines +37 to +39
let tmp = warning.line.split(':');
let file = tmp[0].trim();
let line = tmp[1];
Copy link
Member

Choose a reason for hiding this comment

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

Seems like a good place to destructure, and I'm looking for a chance to make a multi-line suggestion

Suggested change
let tmp = warning.line.split(':');
let file = tmp[0].trim();
let line = tmp[1];
let [file, line] = warning.line.split(':').map(item => item.trim());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya, that's a fine suggestion. But in general I try to avoid cosmetic cleanup in bug-fixes.

} catch (e) {
expect(e.message).to.include(`Livereload failed on 'http://localhost:1337', It may be in use.`);
} finally {
await new Promise(resolve => preexistingServer.close(resolve));
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious about whether there's there a reason you're not using Promise.resolve here?

Suggested change
await new Promise(resolve => preexistingServer.close(resolve));
await Promise.resolve(preexistingServer.close(resolve));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need a promise that fulfills only when the callback passed to prexistingServer.close is invoked.

Unfortunately the return value of preexistingServer.close is not meaningful when it comes to the above mentioned goal as preexistingServer.close(cb) === preexistingServer which when awaited fulfills on the next tick, regardless if the server has closed or not.

Two other reason to not take the alternative approach:

  • await Promise.resolve(value) can be safely reduced to await value assuming Promise is the platform provided promise, which in this case it is. So if the return value of preexistingServer.close was meaningful, we would only need to await it.
  • the resolve argument passed to close is not defined, and will trigger a thrown ReferenceError.

@rwjblue rwjblue merged commit b05d47e into master Feb 29, 2020
@delete-merged-branch delete-merged-branch bot deleted the fail-on-unhandled-rejection branch February 29, 2020 01:34
@rwjblue rwjblue changed the title fail tests on unhandled rejections Fix a number of causes of unhandled rejections (and ensure tests fail when unhandled rejection occurs). Mar 16, 2020
@rwjblue rwjblue added the bug label Mar 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants