-
-
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
Unescape serialized snapshots for diffing #2852
Conversation
@vjeux I thought a bit about evaling these snapshot, but it would be problematic for jsx and regexp at least. We also cannot really use I know it's not ideal, but it works for now 🤷♂️ |
packages/jest-snapshot/src/utils.js
Outdated
const unescape = (data: any): string => { | ||
return data | ||
.replace(/\\(")/g, '$1') // unescape double quotes | ||
.replace(/\\([\\^$*+?.()|[\]{}])/g, '$1'); // then unescape RegExp |
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.
wait, do we really need to do this? Doesn't this also unescape strings that have regex characters in them?
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.
yes it will, that's why I stated it's not ideal
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.
We could avoid unescaping regex, it will result in many backslashes, but it's still gonna be usable.
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.
Yeah, I think we should not do this for now. Let's consider doing this later.
cea766b
to
49fd771
Compare
@rubenmoya We're kind of rushing things for the next release, hence we decided to rebase your commit and apply it here as you were not responding for couple of days, so I'm sorry! (but we made sure to put your name on the commit!) And I'm thankful for your work, it's great. Hope we'll get to merge something from you in the future 🙂 |
@thymikee completely understandable, don't worry about my commit ^^ and I hope it too, I wish I had the same knowledge as I have motivation :) |
Keep getting knowledge while you're motivated and make it a habit so you can keep doing it when motivation passes :) |
wow our travis queues are really fucked. |
I'm gonna merge this and deal with a fix on master if necessary. |
* Unescape serialized snapshots for diffing * Normalize \r same way as \r\n by @rubenmoya * Don't unescape regex for now
* Unescape serialized snapshots for diffing * Normalize \r same way as \r\n by @rubenmoya * Don't unescape regex for now
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
This is a followup of #2731.
I didn't want to alter serialisation improvements which @vjeux introduced, so I thought it would be safer to bring back an
unescape
in a little different form and use it only to generate un-escaped output to diff tool. Thanks to this we avoid having extra backslashes in console output.Given test:
Before:
After:
Test plan
jest
cc: @cpojer @vjeux