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(jest-snapshot): Fix a potential bug when using babel and improve performance #14036

Merged
merged 3 commits into from
Mar 31, 2023

Conversation

liuxingbaoyu
Copy link
Contributor

@liuxingbaoyu liuxingbaoyu commented Mar 29, 2023

Summary

While I was fixing a babel issue, I discovered that jest was expecting buggy behavior.

retainLines will try to keep the original code and the new code on the same line.

expect(a).toMatchInlineSnapshot(`123
456`)

When we replace 123\n456 with the single-line content 123456.
The previous babel would ignore the position of ) and generate this.

expect(a).toMatchInlineSnapshot(`123456`)

And new babel will generate

expect(a).toMatchInlineSnapshot(`123456`
)

jest was relying on the previous behavior, the PR fixes this.
Ref: babel/babel#15515

In addition, I also found that @babel/traverse is being used in the current code to traverse the AST generated by other than @babel/parser, which is a bit unexpected.
But since it's been that long, it seems like it's good enough for this simple usage.
I just replaced it with traverse from @babel/types, which has very simple functionality and is much faster.(Maybe 10x)

Test plan

CI green

) {
throw new Error('Jest: no snapshot insert location found');
}

// A hack to prevent unexpected line breaks in the generated code
node.loc!.end.line = node.loc!.start.line;
Copy link
Contributor Author

@liuxingbaoyu liuxingbaoyu Mar 29, 2023

Choose a reason for hiding this comment

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

Actually only this line is the fix, the rest are refactorings.😃

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.

Awesome, thanks! 👍

@SimenB SimenB merged commit 257f250 into jestjs:main Mar 31, 2023
@github-actions
Copy link

github-actions bot commented May 1, 2023

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 1, 2023
@SimenB
Copy link
Member

SimenB commented Jul 4, 2023

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.

3 participants