Skip to content
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

Merged
merged 1 commit into from
Feb 2, 2019

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Feb 2, 2019

Summary

Fixes jest-community/jest-runner-eslint#64

Test plan

It works 😅 Any tests I write is gonna feel pretty convoluted...

Copy link
Collaborator

@thymikee thymikee left a 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;
Copy link
Collaborator

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?

Copy link
Member Author

@SimenB SimenB Feb 2, 2019

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't agree more

@SimenB SimenB merged commit 4ea8462 into jestjs:master Feb 2, 2019
@SimenB SimenB deleted the handle-missing-todo-in-result branch February 2, 2019 09:37
captain-yossarian pushed a commit to captain-yossarian/jest that referenced this pull request Jul 18, 2019
@github-actions
Copy link

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reports "NaN total"
3 participants