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

Don't cache preview app entry point dependencies #321

Merged
merged 4 commits into from
Dec 26, 2024

Conversation

stephencelis
Copy link
Member

We have a longstanding gotcha where we tell folks to avoid instantiating their root model on their app entry point because any dependencies on the root model may negatively affect any preview in their application.

This PR detects when a dependency is accessed in a SwiftUI preview app entry point and prevents it from influencing the cache using the call stack symbols available. While this isn't a silver bullet, and any async work kicked off from the app entry point could still affect things negatively, this should hopefully be mostly an improvement on the status quo and maybe we won't have to refer folks to this gotcha as often in the future.

We have a longstanding gotcha where we tell folks to avoid instantiating
their root model on their app entry point because any dependencies on
the root model may negatively affect any preview in their application.

This PR detects when a dependency is accessed in a SwiftUI preview app
entry point and prevents it from influencing the cache using the call
stack symbols available. While this isn't a silver bullet, and any async
work kicked off from the app entry point could still affect things
negatively, this should hopefully be mostly an improvement on the status
quo and maybe we won't have to refer folks to this gotcha as often in
the future.
@stephencelis stephencelis merged commit 85f89f5 into main Dec 26, 2024
6 checks passed
@stephencelis stephencelis deleted the detect-preview-entry-point branch December 26, 2024 20:36
@niorko
Copy link

niorko commented Jan 17, 2025

@stephencelis @mbrandonw

We’ve encountered an issue with stateful testValue in tests when running test suites in parallel. It seems that testValue is being cached, which causes shared state between tests. To address this, we wrapped each test case with withDependencies and overrode the dependency explicitly. However, this adds unnecessary boilerplate.

This behavior appears to be related to caching introduced in PR #78, though I may be mistaken. Ideally, testValue should not be cached and should reset automatically for each test case, ensuring isolation even during parallel execution.

Could we consider revisiting this behavior to ensure testValue is not shared across tests? This would align better with typical testing practices. Thank you.

@MasekHJ
Copy link

MasekHJ commented Jan 17, 2025

Adding to @niorko comment:

To be more specific, it seems that issue is that Swift-Testing no longer resets cache.

XCTest previously executed this block

        let testCaseWillStartBlock: @convention(block) (AnyObject) -> Void = { _ in
          DependencyValues._current.cachedValues.cached = [:]
        }

This no longer applies to Swift-Testing.
But even with cache reset in our findings it would not work with parallel run:

final class MultipleParallelTests {
    
    init() {
        DependencyValues._current.resetCache()
    }
}

Because some Dependencies in the code were resolved before cache was reset (maybe like race condition) due to being run in parallel.

using this will work - but this is just a workaround.

@Suite(.serialized)
final class MultipleSerialTests {
    
    init() {
       DependencyValues._current.resetCache()
    }
}

Is there a possibility that Swift Testing @test methods could be isolated from each other (use own DependencyValues / cache) when being run in parallel?

Of course as @niorko mentioned we can add withDependencies to every test and keep the nice-to-have parallel testing, but this adds boilerplate and developers have to think what dependencies to override, because they will be cached (maybe unexpectedly)

@niorko
Copy link

niorko commented Jan 17, 2025

We were using an older version that lacked support for Swift Testing. After updating, it appears to work correctly - apologies for the earlier comments. However, it seems to have some issues with repeated tests, which are likely demonstrated here 7d89478#diff-86f2ce713ec4eb2b2a5301f1b558597d3d362f26a1152c66fb117e2580402c2a.

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.

4 participants