-
-
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: handle missing todo in test result #7779
Conversation
87cdc70
to
b5ad2ce
Compare
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's nice that Jest 24 will work seamlessly with older runners, but this should be marked as "to remove" before Jest 25
// `todos` are new as of Jest 24, and not all runners return it. | ||
// Set it to `0` to avoid `NaN` | ||
if (!testResult.numTodoTests) { | ||
testResult.numTodoTests = 0; |
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.
isn't this something that runners should add themselves for Jest 24 compat, like we did in create-jest-runner
?
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.
sure, but IMO we shouldn't require runners to do so. The fact they have to return a huge results object at all is pretty sad. The real fix IMO would be for runners to return something minimal, and we do a const result = {...defaultResultObject, ...returnValueFromRunner}
(something more clever for deep merge, but essentially this). Should be enough to return e.g. {pass: true, start: timestamp, end: timestamp}
or something like that
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.
Couldn't agree more
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
Fixes jest-community/jest-runner-eslint#64
Test plan
It works 😅 Any tests I write is gonna feel pretty convoluted...