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

Unescape serialized snapshots for diffing #2852

Merged
merged 3 commits into from
Feb 10, 2017

Conversation

thymikee
Copy link
Collaborator

@thymikee thymikee commented Feb 9, 2017

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:

test('snap', () => {
  expect('coca cola "yoko \\son (*)\d o".').toMatchSnapshot();
  expect(/some-regex('\"\))\\.x/).toMatchSnapshot();
});

Before:

screen shot 2017-02-09 at 18 30 47

After:

screen shot 2017-02-09 at 18 30 15

Test plan

jest
cc: @cpojer @vjeux

@thymikee
Copy link
Collaborator Author

thymikee commented Feb 9, 2017

@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 received instead receivedSerialized for diffing, because the latter is processed through pretty-format with all the React plugins.

I know it's not ideal, but it works for now 🤷‍♂️

const unescape = (data: any): string => {
return data
.replace(/\\(")/g, '$1') // unescape double quotes
.replace(/\\([\\^$*+?.()|[\]{}])/g, '$1'); // then unescape RegExp
Copy link
Member

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?

Copy link
Collaborator Author

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

Copy link
Collaborator Author

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.

Copy link
Member

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.

@thymikee
Copy link
Collaborator Author

thymikee commented Feb 9, 2017

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

@rubenmoya
Copy link
Contributor

@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 :)

@thymikee
Copy link
Collaborator Author

thymikee commented Feb 9, 2017

Keep getting knowledge while you're motivated and make it a habit so you can keep doing it when motivation passes :)

@cpojer
Copy link
Member

cpojer commented Feb 10, 2017

wow our travis queues are really fucked.

@cpojer
Copy link
Member

cpojer commented Feb 10, 2017

I'm gonna merge this and deal with a fix on master if necessary.

@cpojer cpojer merged commit 7ca7dac into jestjs:master Feb 10, 2017
@thymikee thymikee deleted the unescape-serialized branch February 19, 2017 16:30
skovhus pushed a commit to skovhus/jest that referenced this pull request Apr 29, 2017
* Unescape serialized snapshots for diffing

* Normalize \r same way as \r\n by @rubenmoya

* Don't unescape regex for now
tushardhole pushed a commit to tushardhole/jest that referenced this pull request Aug 21, 2017
* Unescape serialized snapshots for diffing

* Normalize \r same way as \r\n by @rubenmoya

* Don't unescape regex for now
@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 13, 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.

4 participants