Switch to SnapshotTesting 📸

[mluisbrown]

As a result of this slack conversation I've added a proposal that we switch our snapshot testing library to SnapshotTesting.

📸

[ilyapuchka]

👍 I have one question though about tolerance. If we had to do that (increase tolerance) for currently used library why wouldn't we need to do that with new one as well and if we will have - does it have an API for that?

[mluisbrown]

@ilyapuchka yes, SnapshotTesting allows you to set the tolerance.

This is set on the Strategys for image types, where a tolerance makes sense.

[ilyapuchka]

in fact it can be configured with launch arguments, we just do it like so for some reason

[nunogoncalves]

Actually Took me six months to have a Pr merged where you can actually set each test folder.

[sergdort]

[wltrup]

I'm in favour, except for the misspelling of becuase...

[Ben-Henshall]

you could snapshot a URLRequest

Is there any discussion about new terminology for this type of snapshotting?
The term snapshotting does make sense for this (you're taking a snapshot of an object), but the term snapshot testing is pretty much exclusively used for UI-based testing. It could be confusing without a new term.

[stephencelis]

In the JavaScript/React community, snapshot testing most commonly refers to the text-based version of snapshotting. We think it’s appropriate for any kind of data and hope that the Swift community will get used to expanding the term’s use 😄

[Ben-Henshall]

Another con: It's very annoying to view changes between new and old snapshots in PR diffs due to them being images. I haven't seen a git diff of a hierarchal-snapshot yet, but it sounds like it'd be much easier to view the differences.

On the other hand, the initial adding of snapshots using the new library would be difficult to tell if it's correct from a PR reviewers point of view, but the initial snapshot is usually correct because it's sole intention is that it's a reference point for future regression.

EDIT:
This would depend on if we test views using UIImage, or with recursiveDescription.

[mluisbrown]

I think we would still continue to use image snapshots for UIViews. We might use recursiveDescription also, but IMO the images are important.

[stephencelis]

This changes today 😄

https://www.pointfree.co/blog/posts/23-snapshottesting-1-0-delightful-swift-snapshot-testing

[mluisbrown]

Awesome! 👏

[mbrandonw]

Thanks for these benchmarks!

I think this could be explained by the fact that we support async snapshots (for snapshotting things that require callbacks or delegates, like WKWebView), and so every assertion is using an XCTWaiter behind the scenes. We could probably skip that for assertions that we know are not async.

We'll investigate this soon.

[ilyapuchka]

This will remove some headache from our side as well as we had to implement exactly this case on our side (waiting for web view to load in snapshot tests) 🎉

[mluisbrown]

Oh wow, I totally missed the async snapshot support. This might explain why in the few async snapshot scenarios we have using SnapshotTesting are causing a crash (EXC_BAD_ACCESS). I didn't mention this before as I was trying to nail down the cause a bit better and get an easily reproducible case.

Curiously, the test runs fine, but it's when the XCTest UIApplication is torn down that there is an objc_msgSend crash. I will see if switching to the Async API solves this 👍

[stephencelis]

This is good to know! Thanks for benchmarking. There are definitely some avenues to explore here for optimization.

[RuiAAPeres]

@mluisbrown were you able to make any progress on 2ef7513?

[stephencelis]

@mluisbrown We have a proof-of-concept branch here that optimizes how XCTWaiter is used to avoid unnecessary XCTest expectations: https://github.com/pointfreeco/swift-snapshot-testing/compare/optimize-async

It might help avoid the crash you were seeing and hopefully will improve performance? Care to re-run your benchmark and let us know?

[mluisbrown]

@RuiAAPeres I made some progress on the crashing issue by using Async pullback, but it’s still not totally resolved. I think it is solvable though, just needs a bit more work.

@stephencelis thanks for the POC. I’ll try out as soon as I have time, probably next week 👍

[mluisbrown]

@stephencelis I just tried the XCWaiter POC from the optimize-async branch and there's is no change in the benchmark unfortunately, still running around 15s each time for the same batch of tests 😕

It also didn't help with the crashing, although I have made more progress here. I discovered two things:

• if we have any tests that use Nimble's toEventually() async testing they crash in Nimble's code to manage the runloop
• With those tests disabled, the test suite completes, but when the app is shutting down after testing (in UIApplicationMain) we get a crash trying which appears to me in a call to objc_msgSend.

I enabled Zombie object diagnostics and in both of the above situations just before the crash I see this in the console:

*** -[CALayer setNeedsLayout]: message sent to deallocated instance 0x600000582da0

I'm now convinced that this is the root cause of both the issues above. In fact it's likely the cause of all the crashing we have seen.

In all our codebase, the only two places where setNeedsLayout is being called directly on a CALayer are in CALayer.swift in SnapshotTesting here and here.

It's also relevant since we are doing all our snapshot tests using CALayer instead of UIView or UIViewController as that was the only way I could ensure the entire window, including the nav bar, was included in the snapshot.

I still can't figure why this would be happening, but we're getting closer to the root cause!

[mluisbrown]

It's also relevant since we are doing all our snapshot tests using CALayer instead of UIView or UIViewController as that was the only way I could ensure the entire window, including the nav bar, was included in the snapshot.

@stephencelis I tried changing our tests to use UIView and UIViewController instead of CALayer but the problem persisted.

At this point I think it's probably better if I open an issue on the SnapshotTesting repo to continue the discussion about the async crashes.

[mluisbrown]

For now, the issue with crashing when running with other async tests is insurmountable so I'm closing this proposal 😞

[mluisbrown]

On second thoughts, I'm going to keep this open but mark it as WIP.

[RuiAAPeres]

Any update on this @mluisbrown ?

[mluisbrown]

Unfortunately not @RuiAAPeres 😢 I have a branch where I want to try some more debugging to try and isolate the issue with the crashing. It's not as simple as just adding a single async test.

There are targets where we have a bunch of snapshot tests (and not many other types of test) where this isn't an issue at all. If I add a single async test to this target, it still doesn't crash. So there has to be something more. I'm going to try to find out what it is.

[mluisbrown]

Ok, I have narrowed down the problem to the async snapshot tests that we are doing in IntroViewControllerSnapshotTests and PrivacyNoticeViewControllerSnapshotTests (and possibly others). I created a sample project which repros the issue every time: https://github.com/mluisbrown/SnapshotBug

We could now adopt SnapshotTesting if we decide we can abandon (or somehow remove the async nature of) the tests which are causing the issue.

[RuiAAPeres]

Any update on this @mluisbrown ?

[mluisbrown]

@RuiAAPeres see my previous comment.

We could now adopt SnapshotTesting if we decide we can abandon (or somehow remove the async nature of) the tests which are causing the issue.

The crash situation is identified, and we can work around it by removing or re-working the small handful of async snapshot tests that we have. So, IMO if there is agreement, we can go ahead with implementing this.

[mluisbrown]

we can work around it by removing or re-working the small handful of async snapshot tests that we have

Turns out I had already done this for one of our async tests whilst investigating the issue, so we already have a workaround for async tests using .asyncPullback.

For example: (expand for source code)

final class PrivacyNoticeViewControllerSnapshotTests: SnapshotTestCase {
    let asyncWebViewSnapshotting: Snapshotting = Snapshotting<CALayer, UIImage>.image
        .asyncPullback { (window: UIWindow) in
            Async { callback in
                guard let nav = window.rootViewController as? UINavigationController,
                    let vc = nav.viewControllers.first as? PrivacyNoticeViewController
                    else {
                        fatalError()
                    }

                vc.reactive.signal(for: #selector(vc.webView(_:decidePolicyFor:decisionHandler:)))
                    .observe(on: QueueScheduler.main)
                    .take(duringLifetimeOf: vc)
                    .observeValues { _ in
                        DispatchQueue.main.asyncAfter(deadline: DispatchTime.now() + TimeInterval(0.5)) {
                            callback(window.layer)
                        }
                }
            }
        }

    override func setUp() {
        super.setUp()
        self.recordMode = false
    }

    func testViewControllerWithSummary() {
        Device.all.forEach { device in
            let viewController = PrivacyNoticeViewController(
                viewModel: PrivacyNoticeTestViewModel(),
                appConfiguration: makeAppConfiguration(),
                visuals: MockVisualDependencies.make(),
                shouldHideToolbar: false,
                shouldShowCloseButton: false,
                showSummary: true)

            let navigation = BabylonNavigationController(rootViewController: viewController)
            let window = self.makeWindow(for: navigation, device: device)

            assertSnapshot(
                matching: window,
                as: asyncWebViewSnapshotting,
                named: device.indentifier,
                record: self.recordMode
            )
        }
    }

So there really is no reason to not adopt SnapshotTesting now.