pointfreeco / swift-snapshot-testing

📸 Delightful Swift snapshot testing.
https://www.pointfree.co/episodes/ep41-a-tour-of-snapshot-testing
MIT License
3.74k stars 570 forks source link

Crash `Current context must not be nil` in `recordSnapshot` #875

Closed finestructure closed 1 month ago

finestructure commented 1 month ago

I mentioned this crash to @stephencelis a couple of weeks ago when we first saw it with Xcode 16b2 but it had "disappeared" when I revisited our tests with Xcode 16b3.

Turns out it didn't really disappear but the conditions to trigger it changed in our project (SPI-Server). The conditions in that project are now:

NB: we see the crash also with Xcode 15.4 and it seems to be related to the @Sendable changes we introduced while making the project ready for Swift 6. Reverting to a non-sendable closure makes the snapshot write succeed.

However, I've just set up a new project and it's much simpler to trigger the crash there: https://github.com/finestructure/snapshot-testing-crash

CleanShot 2024-07-19 at 13 36 32@2x

NB: This test crashes on both Xcode 15.4 and Xcode 16b3. The culprit seems to be the async attribute on the test. It does not crash without it:

    func test_crash() async throws {
        // This crashes with `Current context must not be nil`
        assertSnapshot(of: "foo", as: .lines)
    }

    func test_no_crash() throws {
        // This does not crash
        assertSnapshot(of: "foo", as: .lines)
    }
finestructure commented 1 month ago

I should add that this feels like a sendable/concurrency bug upstream but I thought I'd raise it here first in case you have some ideas where to best report/address it.

finestructure commented 1 month ago

Oh, never mind. I just saw this is a duplicate of https://github.com/pointfreeco/swift-snapshot-testing/issues/822 and that I should be marking those tests @MainActor!

finestructure commented 1 month ago

The issue is slightly more complicated than a missing @MainActor annotation and the solution might be helpful for others.

We had tests like the following:

    @MainActor
    func test_documentation_routes_current() async throws {
        ...
        try await app.test(.GET, "/owner/package/~/documentation/target") { @Sendable res in
            ...
            assertSnapshot(of: body, as: .html, named: "index")
            ...
        }
        ...
    }

and these were leading to crashes on when recording. The fix is to move the @MainActor annotation into the closure instead of the @Sendable:

    func test_documentation_routes_current() async throws {
        ...
        try await app.test(.GET, "/owner/package/~/documentation/target") { @MainActor res in
            ...
            assertSnapshot(of: body, as: .html, named: "index")
            ...
        }
        ...
    }
mbrandonw commented 1 month ago

Hey @finestructure, thanks for documenting this! We do have plans to overhaul the sendability of the library, and we even have an experimental async branch that we use on the Point-Free codebase. We just need to clean things up, and see if it's possible to do in a backwards compatible way.