-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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: disallow hook definitions in tests #9957
Conversation
5b8382b
to
2983c35
Compare
Codecov Report
@@ Coverage Diff @@
## master #9957 +/- ##
=======================================
Coverage 64.06% 64.07%
=======================================
Files 293 293
Lines 12457 12459 +2
Branches 3069 3070 +1
=======================================
+ Hits 7981 7983 +2
- Misses 3833 3834 +1
+ Partials 643 642 -1
Continue to review full report at Codecov.
|
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.
👍
Leave it to you to merge pending @jeysal's input.
7243953
to
b212f99
Compare
); | ||
break; |
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.
pushing into the errors
array is essentially what happened before, except we don't go through the handler again into the error
case
break; | ||
} else if (hasStarted) { | ||
state.unhandledErrors.push( | ||
new Error( |
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.
no need to mutate the message in asyncError
, this is sync code
@@ -214,7 +214,7 @@ export type TestError = Exception | [Exception | undefined, Exception]; // the e | |||
export type TestEntry = { | |||
type: 'test'; | |||
asyncError: Exception; // Used if the test failure contains no usable stack trace | |||
errors: TestError; | |||
errors: Array<TestError>; |
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.
it was any
before... Array<any>
is more accurate, but still not particularly good 😅
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.
This is weird, now it can be an array of tuples? Look at TestError
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.
that fits reality
https://github.com/facebook/jest/blob/3375ac3b515e62ca1450e9a154ffdbdf7dc7e1f7/packages/jest-circus/src/eventHandler.ts#L147
https://github.com/facebook/jest/blob/3375ac3b515e62ca1450e9a154ffdbdf7dc7e1f7/packages/jest-circus/src/eventHandler.ts#L150
https://github.com/facebook/jest/blob/3375ac3b515e62ca1450e9a154ffdbdf7dc7e1f7/packages/jest-circus/src/eventHandler.ts#L179
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.
you can see it used in formatNodeAssertErrors.ts
@@ -16,6 +16,7 @@ FAIL __tests__/asyncDefinition.test.js | |||
14 | }); | |||
15 | }); | |||
|
|||
at eventHandler (../../packages/jest-circus/build/eventHandler.js:143:11) |
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.
should be stripped out from stack trace
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.
I tried, it was painful 😀 We could copy the stack from asyncError
I guess
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
Not breaking after #8096, just a cleaner error in this case.
Test plan
Test added