-
-
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
chore: Update @babel/*
deps
#13422
chore: Update @babel/*
deps
#13422
Conversation
Hmm, we should make sure to fix that trace - we wanna point at the |
For comparison, this is how Node.js reports the error when not using Babel: var it = { each() { throw new Error("Err!") } }
it.each``();
which points at this character: it.each``();
// ^ EDIT: Or this, depending on where jest throws the error (if in var it = { each() { return () => { throw new Error("Err!") } } }
it.each``();
which points at this character: it.each``();
// ^ The screenshot above matches exactly this second behavior. |
I investigated and found out that the previous behavior was not the best either, nodejs would ideally output the code at the call site. For example
Not sure what broke this before, causing it to just output |
Wow, amazing test! EDIT: Seems a bit difficult as we are currently totally relying on the behavior of nodejs and nodejs doesn't implement this. |
Landed #13424 which does some of what this PR does to reduce the diff. Then merged |
packages/jest-each/src/bind.ts
Outdated
) => | ||
function eachBind( | ||
) => { | ||
const error = new ErrorWithStack('', bindWrap); |
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.
nice! I actually opened this PR now to start working on this 😀 wonderful timing
There's still one problem left and I'm not quite sure how to fix it, do you have any suggestions? |
1f8e2b5
to
c8544d1
Compare
c8544d1
to
a39810a
Compare
@@ -41,14 +41,12 @@ exports[`works with all statuses 1`] = ` | |||
28 | }); | |||
29 | | |||
> 30 | test.failing.each([ | |||
| ^ | |||
| ^ |
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 change is perfectly fine. would be even better to point at failing
, but as long as it points to the test
expression and not below the data I'm happy 😀
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 difficult unless we completely control source-map-support
ourselves.😀
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.
makes sense. And our usage of it is apparently broken anyways (see #9147)
This is really a nightmare.🙃 |
e44dc8c
to
7e73efd
Compare
I think the fix should probably be in |
I'm not sure this is a bug, seems to be serializing |
0de3b3c
to
7adfaa8
Compare
Oops, I found the reason, |
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.
Can you add a changelog entry?
) { | ||
return ( | ||
needsEachError = false, | ||
): Global.EachTestFn<any> { |
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.
): Global.EachTestFn<any> { | |
): Global.EachTestFn<unknown> { |
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.
ts seems to complain, I don't understand why.
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.
Yay! Thanks for taking the time to fix it - more work than expected! 😅
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
Test plan
CI GREEN