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

Make sure invalid jest-each points to user code #6347

Merged
merged 3 commits into from
May 30, 2018

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented May 29, 2018

Summary

On invalid Each argument, point back to the actual invocation of each from user code.

/cc @mattphillips

Test plan

Updated test

'\n\n' +
`Missing ${RECEIVED_COLOR(`${data.length % keys.length}`)} arguments`,
);

return cb(title, () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

@mattphillips the idea is to create the error within the same async tick as the method call from the user - the moment you cross an async barrier (like executing the body of a test) the original stack is lost

Copy link
Contributor

Choose a reason for hiding this comment

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

mind blown

Copy link
Member Author

@SimenB SimenB May 29, 2018

Choose a reason for hiding this comment

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

throwing a string instead of a new error would probably work as well, as we'd discover there was no stack, and use the one we keep around for async errors. Throwing an error though gives us a stack - it just doesn't point to user code

'Received:\n' +
RECEIVED_COLOR(pretty(data)) +
'\n\n' +
`Missing ${RECEIVED_COLOR(`${data.length % keys.length}`)} arguments`,
Copy link
Member Author

Choose a reason for hiding this comment

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

last s here should be gone, when 1. We've got a pluralize helper or two sticking around somewhere

Copy link
Contributor

Choose a reason for hiding this comment

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

Want me to send a PR for this? I can't amend this branch ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

yes please 🙂

10 | \${true} | \${true}
11 | \${true} |

at packages/jest-each/build/bind.js:80:23
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be lovely if we could strip this internal call

Copy link
Contributor

Choose a reason for hiding this comment

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

That would be awesome!

Copy link
Member Author

Choose a reason for hiding this comment

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

it's 2 or 3 layers up, so a bit painful (we'd have to create the error in index and pass it into bind)

Copy link
Member Author

Choose a reason for hiding this comment

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

HAH, I lied. Pushed 🙂

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.

😍

@mattphillips
Copy link
Contributor

This is so freaking cool! Amazing stuff @SimenB ❤️

@codecov-io
Copy link

codecov-io commented May 29, 2018

Codecov Report

Merging #6347 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6347      +/-   ##
==========================================
+ Coverage   63.63%   63.64%   +0.01%     
==========================================
  Files         226      226              
  Lines        8648     8651       +3     
  Branches        4        4              
==========================================
+ Hits         5503     5506       +3     
  Misses       3144     3144              
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-each/src/bind.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2307643...b7deda9. Read the comment docs.

@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.

6 participants