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

Crash Current context must not be nil in recordSnapshot #875

Closed
finestructure opened this issue Jul 19, 2024 · 4 comments
Closed

Crash Current context must not be nil in recordSnapshot #875

finestructure opened this issue Jul 19, 2024 · 4 comments

Comments

@finestructure
Copy link
Contributor

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:

  • a test is recording a snapshot in a closure marked @Sendable
  • the test must be be actually trying to record a snapshot (i.e. unless we had snapshot changes the crash wouldn't trigger)
  • it only crashes when snapshots are created within Xcode (due to the __XCODE_BUILT_PRODUCTS_DIR_PATHS env variable check), not when the test are run via swift test

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
Copy link
Contributor Author

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
Copy link
Contributor Author

Oh, never mind. I just saw this is a duplicate of #822 and that I should be marking those tests @MainActor!

@finestructure
Copy link
Contributor Author

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
Copy link
Member

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.

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

No branches or pull requests

2 participants