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

Escaped characters detection in literals #231

Merged
merged 2 commits into from Jun 18, 2019
Merged

Escaped characters detection in literals #231

merged 2 commits into from Jun 18, 2019

Conversation

ferranpujolcamins
Copy link
Contributor

@ferranpujolcamins ferranpujolcamins commented May 17, 2019

Followup of #228, Fixes #230

This PR fixes inline snapshot tests containing special characters. AssertInlineSnapshot now checks whether the snapshot contains special characters or not. If it does, escapes the snapshot literal appropriately.

Known issue: AssertInlineSnapshot still searches only for "" and multiline string literals (it does not search #""" """# for example)

Copy link
Member

@stephencelis stephencelis left a comment

Choose a reason for hiding this comment

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

Wow @ferranpujolcamins, you took this to the next level!

This is looking really good! I cloned your branch and played around with it and it feels very slick! 😄 Just a few small comments.

Known issue: AssertInlineSnapshot still searches only for "" and multiline string literals (it does not search #""" """# for example)

One interesting side effect is that it'll automatically insert more #s than are needed when re-recording. Do you think you'll have time to address this soon?

SnapshotTesting.xcodeproj/project.pbxproj Outdated Show resolved Hide resolved
@ferranpujolcamins
Copy link
Contributor Author

Do you think you'll have time to address this soon?

I'm giving it a try

@ferranpujolcamins
Copy link
Contributor Author

One interesting side effect is that it'll automatically insert more #s than are needed when re-recording

I managed to make it work. Not without pain! Substrings are tricky. Mutating self in AssertInlineSnapshot.replaceFirstOccurrence produced crazy results, I don't exactly know why.

@stephencelis
Copy link
Member

Hey @ferranpujolcamins! Looks like we're close but not quite there yet. If you run the following test block:

  func testAny() {
    struct User { let id: Int, name: String, bio: String }
    let user = User(id: 1, name: ##"Bl#""#obby"##, bio: "Blobbed around the world.")
    assertSnapshot(matching: user, as: .dump)
    _assertInlineSnapshot(matching: user, as: .dump, with: #####"""
    """#####)
  }

The leading #####""" correctly becomes ##""", but the trailing """##### ends up appending an extra coupla #s to become """#######. I think the problem is the use of multiLineLiteralEndIndex for replacement, which only matches against """.

The logic you're building on is definitely the most complicated part of the lib, sorry! Let us know if we can help in any way.

@ferranpujolcamins
Copy link
Contributor Author

I see! This won't be an easy change after all. Let's do this:
I'll open other PRs with small refactors that will make our live easier. My goal is to be able to unit test the inline snapshot feature, so I will separate code transformation from file operartions.

After that, we can keep working on this one.

@ferranpujolcamins
Copy link
Contributor Author

Would be cool to add swift-prelude as dependency?

@ferranpujolcamins
Copy link
Contributor Author

Mmmm I see it depends on this package so we can't use it here :(

@ferranpujolcamins ferranpujolcamins changed the title Escaped characters detection in literals [WIP] Escaped characters detection in literals Jun 5, 2019
@ferranpujolcamins
Copy link
Contributor Author

Ok, I've rebased onto master.
I've added some tests. Your counter example is now working properly.
I still need to add some more, but I'd appreciate early feedback.

The tests work as follows (it's a little bit confusing):
The tests in InlineSnapshotTests assert the correctness of the source file modifications that the inline snapshot test machinery does. To do so, we snapshot test the modified source.

The recorded snapshots, are in turn well formed tests that are added to the test target.

  • Thus, we check the source code after an inline snapshot is recorded is valid.
  • They assert that the recorded snapshot is escaped properly.
    Please see comment at the end of InlineSnapshotTests.swift.

Copy link
Member

@stephencelis stephencelis left a comment

Choose a reason for hiding this comment

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

This is really fun stuff! @mbrandonw and I looked over it today and would love to help you land it in any way possible. The subject of the PR is still a "WIP" and you mention that you still have some things you wanted to work on. Can you let us know what they are and how we can unblock you?

assertSnapshot(source: newSource, diffable: diffable)
}

// TODO: add more tests like this with different amount of lines
Copy link
Member

Choose a reason for hiding this comment

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

Just wanna flag this before merging.

}
}

// Class that is extended with the generated code to check that it builds.
Copy link
Member

Choose a reason for hiding this comment

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

Cool stuff!

@ferranpujolcamins ferranpujolcamins changed the title [WIP] Escaped characters detection in literals Escaped characters detection in literals Jun 12, 2019
@ferranpujolcamins
Copy link
Contributor Author

Yeah I just added a couple of tests. Should be ready to merge. If we find some corner case later let's add a regression test!

Thanks guys!

@ferranpujolcamins
Copy link
Contributor Author

Gentle ping ;)

@stephencelis stephencelis merged commit dc14bff into pointfreeco:master Jun 18, 2019
@stephencelis
Copy link
Member

Beautiful work! This'll eliminate a major gotcha for folks. Thanks again for contributing!

@ferranpujolcamins
Copy link
Contributor Author

Thank you guys for working on this useful project!

@stephencelis stephencelis mentioned this pull request Sep 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inline Snapshots: Escape Strings
2 participants