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

Add support for multiple snapshotting strategy for each assert #150

Merged
merged 1 commit into from
Dec 21, 2018
Merged

Conversation

mackoj
Copy link
Contributor

@mackoj mackoj commented Dec 19, 2018

Attempt to fix #116

as: [snapshotting],
named: name,
record: recording,
timeout: timeout)
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the PR! Stephen and I will discuss this sometime soon, but in the meantime there is a quick thing you can do to get tests passing. Make sure to pass along the file, testName and line to the assert snapshot helpers so that files names are recorded properly.

We also prefer to have all the function arguments on a newline if they don't fit on online, as opposed to this hanging argument style.

Also there's an extra image that was added to the repo that could be deleted:

assertSnapshot-matching-as-named-record-timeout-file-testName-line.1.png

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the comments, I will do the modification

@stephencelis
Copy link
Member

stephencelis commented Dec 20, 2018

Hey @mackoj, thanks for the PR! One thing that I think needs to change is that assertSnapshots needs to lose the named parameter. The name there replaces the auto-incrementing default name of 1, 2, etc (which means if you use named it'll fail as snapshots collide). With that in mind assertSnapshots will have to call assertSnapshot instead.

We wrote these helpers in the Point-Free repo awhile back:

https://github.com/pointfreeco/pointfreeco/blob/18ec4ac7486f9c817d31e20dbebf0cc597e6adba/Sources/PointFreeTestSupport/PointFreeTestSupport.swift#L558-L603

Feel free to grab em and use em in this PR! Will be happy to merge when you do 😄

And thanks for taking the time to help!

@stephencelis
Copy link
Member

Oh! And unfortunately we have to define these helpers twice right now (you can scroll above the last link to see them defined inside the Linux SnapshotTestCase class). Linux Foundation currently lacks the XCTWaiter APIs, so we need to use a subclass there.

@mackoj
Copy link
Contributor Author

mackoj commented Dec 21, 2018

Ok I will fix it today. Thanks for the comment @stephencelis

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.

Looks great!

@stephencelis stephencelis merged commit f73aa80 into pointfreeco:master Dec 21, 2018
@stephencelis
Copy link
Member

Thanks again! 😄

szotyi pushed a commit to szotyi/swift-snapshot-testing that referenced this pull request Jan 3, 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.

Group helper: assertSnapshots(matching:as:)
3 participants