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

chore: Update @babel/* deps #13422

Merged
merged 16 commits into from Oct 11, 2022
Merged

chore: Update @babel/* deps #13422

merged 16 commits into from Oct 11, 2022

Conversation

liuxingbaoyu
Copy link
Contributor

Summary

Test plan

CI GREEN

@liuxingbaoyu
Copy link
Contributor Author

image
This is technically correct, but the actual effect is not sure if it matters.
The ( of the previous function will be randomly mapped to the parent node, which may be the function name or function.

@liuxingbaoyu liuxingbaoyu marked this pull request as draft October 10, 2022 13:23
@SimenB
Copy link
Member

SimenB commented Oct 10, 2022

Hmm, we should make sure to fix that trace - we wanna point at the it

@liuxingbaoyu
Copy link
Contributor Author

An even worse news, here's a behavior change inside nodejs, too bad.🙃
image

@nicolo-ribaudo
Copy link
Contributor

nicolo-ribaudo commented Oct 10, 2022

#13422 (comment)

For comparison, this is how Node.js reports the error when not using Babel:

var it = { each() { throw new Error("Err!") } }

it.each``();
Error: Err!
    at Object.each (/home/nic/Documents/dev/jest/demo.js:1:27)
    at Object.<anonymous> (/home/nic/Documents/dev/jest/demo.js:3:8)
    at Module._compile (node:internal/modules/cjs/loader:1120:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1174:10)
    at Module.load (node:internal/modules/cjs/loader:998:32)
    at Module._load (node:internal/modules/cjs/loader:839:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
    at node:internal/main/run_main_module:17:47

which points at this character:

it.each``();
//     ^

EDIT: Or this, depending on where jest throws the error (if in it.each, or if in the function returned by it.each)

var it = { each() { return () => { throw new Error("Err!") } } }

it.each``();
Error: Err!
    at Object.each (/home/nic/Documents/dev/jest/demo.js:1:27)
    at Object.<anonymous> (/home/nic/Documents/dev/jest/demo.js:3:10)
    at Module._compile (node:internal/modules/cjs/loader:1120:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1174:10)
    at Module.load (node:internal/modules/cjs/loader:998:32)
    at Module._load (node:internal/modules/cjs/loader:839:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
    at node:internal/main/run_main_module:17:47

which points at this character:

it.each``();
//       ^

The screenshot above matches exactly this second behavior.

@liuxingbaoyu
Copy link
Contributor Author

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 assert(!!"") will output the following information

AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:

   assert(!!"")

Not sure what broke this before, causing it to just output false == true.
Now nodejs is able to partially successfully check the code at the call site, unfortunately it detects the wrong code, but I think we can just ignore this because our assert is wrapped and nodejs can never know when the real call is made code.

@liuxingbaoyu
Copy link
Contributor Author

liuxingbaoyu commented Oct 10, 2022

Wow, amazing test!
Yes, pointing to it may be technically difficult to implement, since the source map normally does not provide this information.
The first of the two cases should be able to handle, the second I'm not sure.
In the first case we can try to point near each, which will align with other behaviors.

image

EDIT: Seems a bit difficult as we are currently totally relying on the behavior of nodejs and nodejs doesn't implement this.

@SimenB
Copy link
Member

SimenB commented Oct 10, 2022

Landed #13424 which does some of what this PR does to reduce the diff. Then merged main into this branch.

) =>
function eachBind(
) => {
const error = new ErrorWithStack('', bindWrap);
Copy link
Member

@SimenB SimenB Oct 11, 2022

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

@liuxingbaoyu
Copy link
Contributor Author

There's still one problem left and I'm not quite sure how to fix it, do you have any suggestions?
test.*.each will capture and output the stack directly inside test.*, I have no chance to modify err.stack.

@@ -41,14 +41,12 @@ exports[`works with all statuses 1`] = `
28 | });
29 |
> 30 | test.failing.each([
| ^
| ^
Copy link
Member

@SimenB SimenB Oct 11, 2022

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 😀

Copy link
Contributor Author

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

Copy link
Member

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)

@liuxingbaoyu
Copy link
Contributor Author

This is really a nightmare.🙃

@liuxingbaoyu liuxingbaoyu marked this pull request as ready for review October 11, 2022 05:05
@SimenB
Copy link
Member

SimenB commented Oct 11, 2022

This is really a nightmare.🙃

I think the fix should probably be in jest-jasmine2, not jest-each

@liuxingbaoyu
Copy link
Contributor Author

This is really a nightmare.🙃

I think the fix should probably be in jest-jasmine2, not jest-each

I'm not sure this is a bug, seems to be serializing Error differently.

@liuxingbaoyu
Copy link
Contributor Author

Oops, I found the reason, jest-jasmine2 was relying on the stack string, I replaced Error:.🤦‍♂️
CI should be green now!

Copy link
Member

@SimenB SimenB left a 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> {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
): Global.EachTestFn<any> {
): Global.EachTestFn<unknown> {

Copy link
Contributor Author

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.

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@SimenB SimenB left a 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! 😅

@SimenB SimenB merged commit 7b67ecf into jestjs:main Oct 11, 2022
@SimenB
Copy link
Member

SimenB commented Oct 14, 2022

@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 Nov 14, 2022
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.

None yet

4 participants