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

Fixes snapshot rendering not to show backslashes #2731

Closed
wants to merge 1 commit into from

Conversation

thymikee
Copy link
Collaborator

@thymikee thymikee commented Jan 29, 2017

Summary

Back to normal snapshot rendering in terminal, without altering new snapshots escaping method.

screen shot 2017-01-29 at 01 08 51

Test plan

jest

@cpojer
Copy link
Member

cpojer commented Jan 29, 2017

Wait, I'm worried this is a hack. Shouldn't this replace "\" with "" (nothing)?

cc @vjeux. He is the expert here.

@vjeux
Copy link
Contributor

vjeux commented Jan 29, 2017

Can you tell me how to reproduce this? The fix is almost certainly wrong. If you see a \ it means that something is not correctly escaped somewhere in the process, we need to find the root cause and not blindly strip them out.

@thymikee
Copy link
Collaborator Author

I know this is hacky and doesn't solve problem entirely, sorry. Just wanted to let you know that there's something weird going on.

This will show up like this every time you change a snapshotted string value (at least on my computer). Try changing "unkwon" to something different in this test. And based on the changes made to snapshots by #2482 it escapes the same way.

@thymikee
Copy link
Collaborator Author

I'll close this PR, but let's discuss it further

@thymikee thymikee closed this Jan 29, 2017
@@ -158,7 +158,7 @@ class SnapshotState {
if (!pass) {
this.unmatched++;
return {
actual: receivedSerialized,
actual: receivedSerialized.replace(/\\"/g, '"'),
Copy link
Contributor

Choose a reason for hiding this comment

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

here you can use received

@@ -158,7 +158,7 @@ class SnapshotState {
if (!pass) {
this.unmatched++;
return {
actual: receivedSerialized,
actual: receivedSerialized.replace(/\\"/g, '"'),
count,
expected,
Copy link
Contributor

Choose a reason for hiding this comment

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

the tricky thing is this one. we only have the serialized version. The way we unserialize stuff right now is to eval. So we'd need to eval the expected part.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

is it safe to eval here?

Copy link
Member

Choose a reason for hiding this comment

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

wait, why do we need to eval here?

Copy link
Contributor

Choose a reason for hiding this comment

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

So, expected here is a serialized version. The way you unserialize a snapshot is by doing require('snapshot.js') which basically evals it.

@thymikee thymikee deleted the fix-snapshot-rendering branch February 19, 2017 16:33
@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