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

Rethrow unhandled promise rejections in tests #23

Closed
wants to merge 1 commit into from

Conversation

foolip
Copy link
Member

@foolip foolip commented Oct 27, 2017

In https://travis-ci.org/whatwg/misc-server/builds/293854231 this
happened without failing the build:

(node:4452) UnhandledPromiseRejectionWarning: Unhandled promise
rejection (rejection id: 1): FetchError: request to
http://resources.whatwg.org/ failed, reason: connect ETIMEDOUT
165.227.248.76:80
(node:4452) [DEP0018] DeprecationWarning: Unhandled promise rejections
are deprecated. In the future, promise rejections that are not handled
will terminate the Node.js process with a non-zero exit code.

@foolip foolip requested a review from domenic October 27, 2017 22:32
@domenic
Copy link
Member

domenic commented Oct 27, 2017

I think the more correct way to do this is

test().catch(e => {
  console.error(e.stack);
  process.exit(1);
});

As written right now the process will exit the moment any rejection goes unhandled, which works poorly with your perform-all-requests-in-parallel-then-report-all-results-cumulatively strategy.

@foolip
Copy link
Member Author

foolip commented Oct 27, 2017

That does look nicer, but also results in the test exiting at the first error. I think that's OK though, to deal with network timeouts I think I'll add in some retry logic.

Any reason to use e.stack as opposed to just e? They result in the same output in my throw new Error('banan cake') test, but just e would work if something other than an Error is used.

In https://travis-ci.org/whatwg/misc-server/builds/293854231 this
happened without failing the build:

(node:4452) UnhandledPromiseRejectionWarning: Unhandled promise
rejection (rejection id: 1): FetchError: request to
http://resources.whatwg.org/ failed, reason: connect ETIMEDOUT
165.227.248.76:80
(node:4452) [DEP0018] DeprecationWarning: Unhandled promise rejections
are deprecated. In the future, promise rejections that are not handled
will terminate the Node.js process with a non-zero exit code.
@foolip foolip force-pushed the unhandledRejection branch from b7cbcfd to be71dc5 Compare October 27, 2017 22:46
@domenic
Copy link
Member

domenic commented Oct 27, 2017

Hmm, I thought it was different in some cases, but I could be wrong and just cargo-culting myself from years ago. Sticking with e seems fine.

I'm quite surprised that exits at first error given how you catch all errors and turn them into non-OK status/ok values.

@domenic
Copy link
Member

domenic commented Oct 27, 2017

Actually, wait, I don't understand the OP. You process.exit(1) on non-OK status. So how did that rejection get triggered in the first place without failing the build?

@domenic
Copy link
Member

domenic commented Oct 27, 2017

I see, I was confusing the two test scripts. You should probably make them more parallel in structure, e.g. try/catching failed fetches in both cases.

@foolip
Copy link
Member Author

foolip commented Oct 29, 2017

Rather than continue to make the testing framework more fancy I went to find a popular framework and used that: #25

This means the tests are no longer run in parallel, but I think it was totally worth it. Closing this in favor of that.

@foolip foolip closed this Oct 29, 2017
@foolip foolip deleted the unhandledRejection branch October 29, 2017 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants