-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
test_runner: better handle async bootstrap errors #46720
Conversation
Review requested:
|
576da3f
to
d63041d
Compare
lib/internal/test_runner/utils.js
Outdated
try { | ||
const reportersMap = await getReportersMap(reporters, destinations); | ||
for (let i = 0; i < reportersMap.length; i++) { | ||
const { reporter, destination } = reportersMap[i]; | ||
compose(testsStream, reporter).pipe(destination); | ||
} | ||
} catch (err) { | ||
throw new ERR_TEST_FAILURE(err, kAsyncBootstrapFailure); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we should probably only catch async errors, and not errors that might happen in the for loop.
try { | |
const reportersMap = await getReportersMap(reporters, destinations); | |
for (let i = 0; i < reportersMap.length; i++) { | |
const { reporter, destination } = reportersMap[i]; | |
compose(testsStream, reporter).pipe(destination); | |
} | |
} catch (err) { | |
throw new ERR_TEST_FAILURE(err, kAsyncBootstrapFailure); | |
const reportersMap = await PromisePrototypeThen( | |
getReportersMap(reporters, destinations), undefined, | |
(err) => PromiseReject(new ERR_TEST_FAILURE(err, kAsyncBootstrapFailure)), | |
); | |
for (let i = 0; i < reportersMap.length; i++) { | |
const { reporter, destination } = reportersMap[i]; | |
compose(testsStream, reporter).pipe(destination); |
or
try { | |
const reportersMap = await getReportersMap(reporters, destinations); | |
for (let i = 0; i < reportersMap.length; i++) { | |
const { reporter, destination } = reportersMap[i]; | |
compose(testsStream, reporter).pipe(destination); | |
} | |
} catch (err) { | |
throw new ERR_TEST_FAILURE(err, kAsyncBootstrapFailure); | |
let reportersMap; | |
try { | |
reportersMap = await getReportersMap(reporters, destinations); | |
} catch(err) { | |
throw new ERR_TEST_FAILURE(err, kAsyncBootstrapFailure); | |
} | |
for (let i = 0; i < reportersMap.length; i++) { | |
const { reporter, destination } = reportersMap[i]; | |
compose(testsStream, reporter).pipe(destination); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the time this code is reached, getReportersMap()
has made everything async from the test runner's point of view.
d63041d
to
1578a7d
Compare
The test runner is bootstrapped synchronously, with the exception of importing custom reporters. To better handle asynchronously throw errors, this commit introduces an internal error type that can be checked for from the test runner's uncaughtException handler. After nodejs#46707 and this change land, the other throw statement in the uncaughtException handler can be removed. This will allow the test runner to handle errors thrown from outside of tests (which currently prevents the test runner from reporting results).
1578a7d
to
59b149d
Compare
Landed in 0c90be9 |
The test runner is bootstrapped synchronously, with the exception of importing custom reporters. To better handle asynchronously throw errors, this commit introduces an internal error type that can be checked for from the test runner's uncaughtException handler. After nodejs#46707 and this change land, the other throw statement in the uncaughtException handler can be removed. This will allow the test runner to handle errors thrown from outside of tests (which currently prevents the test runner from reporting results). PR-URL: nodejs#46720 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
The test runner is bootstrapped synchronously, with the exception of importing custom reporters. To better handle asynchronously throw errors, this commit introduces an internal error type that can be checked for from the test runner's uncaughtException handler. After nodejs#46707 and this change land, the other throw statement in the uncaughtException handler can be removed. This will allow the test runner to handle errors thrown from outside of tests (which currently prevents the test runner from reporting results). PR-URL: nodejs#46720 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
The test runner is bootstrapped synchronously, with the exception of importing custom reporters. To better handle asynchronously throw errors, this commit introduces an internal error type that can be checked for from the test runner's uncaughtException handler. After nodejs#46707 and this change land, the other throw statement in the uncaughtException handler can be removed. This will allow the test runner to handle errors thrown from outside of tests (which currently prevents the test runner from reporting results). PR-URL: nodejs#46720 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
The test runner is bootstrapped synchronously, with the exception of importing custom reporters. To better handle asynchronously throw errors, this commit introduces an internal error type that can be checked for from the test runner's uncaughtException handler. After #46707 and this change land, the other throw statement in the uncaughtException handler can be removed. This will allow the test runner to handle errors thrown from outside of tests (which currently prevents the test runner from reporting results). PR-URL: #46720 Backport-PR-URL: #46839 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
The test runner is bootstrapped synchronously, with the exception of importing custom reporters. To better handle asynchronously throw errors, this commit introduces an internal error type that can be checked for from the test runner's uncaughtException handler. After #46707 and this change land, the other throw statement in the uncaughtException handler can be removed. This will allow the test runner to handle errors thrown from outside of tests (which currently prevents the test runner from reporting results). PR-URL: #46720 Backport-PR-URL: #46839 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
The test runner is bootstrapped synchronously, with the exception of importing custom reporters. To better handle asynchronously throw errors, this commit introduces an internal error type that can be checked for from the test runner's uncaughtException handler. After #46707 and this change land, the other throw statement in the uncaughtException handler can be removed. This will allow the test runner to handle errors thrown from outside of tests (which currently prevents the test runner from reporting results). PR-URL: #46720 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
The test runner is bootstrapped synchronously, with the exception of importing custom reporters. To better handle asynchronously throw errors, this commit introduces an internal error type that can be checked for from the test runner's
'uncaughtException'
handler.After #46707 and this change land, the other
throw
statement in the'uncaughtException'
handler can be removed. This will allow the test runner to handle errors thrown from outside of tests (which currently prevents the test runner from reporting results).