Ensure test run failures crash worker #1265
Conversation
This allows errors from the exit logic to propagate to the runner.
Errors that occur inside the runner are treated as uncaught exceptions. This prevents them from being swallowed in the promise chain, causing the test to hang.
novemberborn
force-pushed
the
ensure-test-run-failures-crash-worker
branch
from
7e37f44 to
539c741
Compare
novemberborn
changed the title
|
Added tests to last commit. Let's hope they pass in CI 😄 |
lib/main.js
Outdated
| // Avoid using serializeError | ||
| const err = new Error('Runner failed with an unserializable exception'); | ||
| exception = { | ||
| name: 'Error', |
There was a problem hiding this comment.
Super nitpick, but would prefer err.name here just for consistency with the other props and more future proof if we decide to change the error type or something.
lib/main.js
Outdated
| exception = serializeError(err); | ||
| } catch (_) { | ||
| // Avoid using serializeError | ||
| const err = new Error('Runner failed with an unserializable exception'); |
There was a problem hiding this comment.
I wonder, should we put this logic in serializeError so it handles reporting about unserializable exceptions everywhere?
There was a problem hiding this comment.
It's only called in three places. I'd prefer letting the error propagate correctly, and then treating it as an uncaught exception.
That said I've changed this code to process.emit('uncaughtException', err) and moved the fallback stuff to the uncaughtException handler in test-worker.js.
| import test from '../../../'; | ||
|
|
||
| test(() => { | ||
| return Promise.reject(new Error('Hi :)')); |
There was a problem hiding this comment.
Would look better to use async function here and throw instead. Same with the below fixture.
The first commit is there to catch more errors. The second commit ensures errors like #1263 are surfaced without the test hanging.