[Circus] Add timeout event, and don't print both timeout and expect.assertions error #7201
Conversation
|
There are a few ways to fix the bug but adding this event seemed to make the most sense -- if this doesn't fit into the vision of jest-circus I'll happily try a different strategy |
| @@ -160,7 +160,12 @@ const _callCircusTest = ( | |||
|
|
|||
| return callAsyncCircusFn(test.fn, testContext, {isHook: false, timeout}) | |||
| .then(() => dispatch({name: 'test_fn_success', test})) | |||
| .catch(error => dispatch({error, name: 'test_fn_failure', test})); | |||
| .catch(message => { | |||
| if (/^(Error: )?Exceeded timeout/.test(message)) { | |||
There was a problem hiding this comment.
Not crazy about string matching here, any alternatives?
There was a problem hiding this comment.
We might consider adding a property to the error we reject with, which can be set to timeout, assertion etc? Kind of like code for node core errors. Not sure
There was a problem hiding this comment.
I thought to do that but we don't reject with an Error we reject with a message (oops)
rickhanlonii
changed the title
rickhanlonii
changed the title
| @@ -79,6 +79,7 @@ export const makeTest = ( | |||
| asyncError, | |||
| duration: null, | |||
| errors: [], | |||
| expired: false, | |||
There was a problem hiding this comment.
I wonder if timedOut would be better name?
There was a problem hiding this comment.
I was between both, not strongly opinionated either way
There was a problem hiding this comment.
more of a fan of failureReason: 'timeout' | 'error' | 'assertion-error' or something.
error being anything not JestAssertionError (and maybe node core assertion)
|
@rickhanlonii possible to bring this over the line? |
|
Oh wow I forgot about this. I won't have time to look at this until next year |
|
We'll make circus the default in 27, would be nice to include this |
|
Would be nice to get out before the major as well, as it seems non-breaking, and then we can be confident we make a version of circus the default that has already been proven |
|
AFAIK we can rebase and land this as-is? |
|
@rickhanlonii I still like my feedback here #7201 (comment) but other than that I think so yeah 😀 |
| @@ -233,6 +233,8 @@ const eventHandler = (event: Event) => { | |||
| }; | |||
|
|
|||
| const _addExpectedAssertionErrors = (test: TestEntry) => { | |||
| if (test.expired) return; | |||
There was a problem hiding this comment.
instead of a new prop here, we could also check if test.errors > 0 and then not add from expect?
Summary
This PR adds a new event for jest-circus called
test_fn_timeoutdispatched when a test times out.This event will be fired in addition to
test_fn_failure(that choice is up for discussion)Motivation
The motivation for this change is to fix #3742
We do this by:
test.expiredtoTestEntrytest.expired = truein the newtest_fn_timeouthookTest plan
Added e2e tests