pointfreeco / swift-dependencies

A dependency management library inspired by SwiftUI's "environment."
https://www.pointfree.co
MIT License
1.54k stars 118 forks source link

Flakkines on Testing Target #75

Closed onurhuseyincantay closed 11 months ago

onurhuseyincantay commented 1 year ago

Description

Hello everyone,

I am facing an issue regards to the dependencies library and was wondering what might be the cause:

the project that I am working is a highly modular application, let say I have a dependency called var sum (Int, Int) -> String and I add this to dependencies library as \.sum sometimes some modules that are not even depending on this test are failing due to the unimplemented case.

My tests are running in parallel Its not always failing some of the dependencies are injected in the module it self some are on the application target

Project -> Contains some dependency + Injects them to Dependencies Module A -> Contains some dependency + Injects them to Dependencies

Checklist

Expected behavior

When I provide the dependency on TestStore.dependencies I would expect to make use of the value mock version that I provide.

Actual behavior

When I provide the dependency on TestStore.dependencies I would fall into the testValue which is the failing implementation.

Steps to reproduce

Dependencies version information

0.1.2

Destination operating system

iOS 16

Xcode version information

14.2

Swift Compiler version information

5.7.1

james-rant commented 1 year ago

I've run into this issue as well. We have a similar setup, a highly modular application. One module contains a dependency which is used in other modules via @Dependency.

In a unit test case we have several tests that make use of mocked version (via dependency overrides on the TestStore). When run in isolation every test passes 100% of the time. But when run as part of the test bundle a single test will fail citing unimplemented dependencies.

If I instruct the Test Plan to execute in random order and re-run failed tests, then the test that fails will always succeed when run the second time, as far as my observations have shown.

I'm at a loss as to where to begin resolving this, as we have used @Dependency in other tests without issues for a long time.

james-rant commented 1 year ago

Quick update... I may have found a workaround (for my case, anyway).

I still don't really know why the tests would fail before now, but I made a change and now all my tests pass reliably.

I had a test that was using store.exhaustivity = .off as this test didn't care about any of the resulting actions (testing them in this test would just be duplicating effort, as they were already tested in another case). Removing this line and implementing the test as an exhaustive test (receiving the resulting actions and state changes etc), makes all the other tests pass reliably. 🤯

Again, I still don't know why this is the case, but removing the store.exhaustivity = .off has worked around it for me.

mbrandonw commented 1 year ago

Hi @onurhuseyincantay, sorry we missed this issue when it was first posted. Can you share a project that reproduces the problem? We haven't run into this issue yet. If the project is private but you are willing to share it, feel free to invite me and @stephencelis to it and/or email us at support@pointfree.co.

And just to be sure I'm understanding, both of you, @onurhuseyincantay and @james-rantmedia, are running the tests for a specific SPM module, and not the app target's tests at all?

james-rant commented 1 year ago

And just to be sure I'm understanding, both of you, @onurhuseyincantay and @james-rantmedia, are running the tests for a specific SPM module, and not the app target's tests at all?

Correct. I'm running the test target for the specific module, not the app target.

mbrandonw commented 1 year ago

Ok thanks for confirming. It's hard to diagnose the problem without a reproducing project, so any help with that would be great.

james-rant commented 1 year ago

I'll see what I can do... replicating the exact setup may prove tricky as this is a large codebase for a live app... I'll let you know.

onurhuseyincantay commented 1 year ago

Hi @mbrandonw , its totally fine. Unfortunately the project is private and cant share details around it. On my case the tests are running on application target in a random and parallel order. There are certain modules that registers dependencies on module target and expose them to the main application. Currently I am on vacation so I wouldn't have the time to recreate a similar structure and will be back on track in 2 weeks or so in case @james-rantmedia can provide I would appreciate the effort from everyone and thanks again picking up this issue lets fix it together!

mbrandonw commented 1 year ago

On my case the tests are running on application target in a random and parallel order.

@onurhuseyincantay Ah ok, then in this case it may actually be something quite simple. If you didn't know already, when tests are run in an app target, the app actually boots up and executes its entry point. That can cause dependencies to be accessed, which will cause a test failure since you are technically accessing dependencies in a test context without overriding them.

We document this gotcha here. The fix is to stop executing your app code while running in tests, which the article linked provides one way of doing it, but there are also more.

onurhuseyincantay commented 1 year ago

@mbrandonw in our case the application has a main.swift file where we assign different appdelegate that doesn't have any dependencies being registered so a TestDelegate not sure if it would still solve the issue

james-rant commented 1 year ago

While writing the reproduction project, I happened to realise what the problem is...

The test that used exhaustivity = .off was still accessing the dependency, but hadn't overridden the unimplemented version (an oversight on my part). But instead of failing that test and showing the error where the problem actually is, Xcode decided to fail the previous test and put the error there, completely throwing me off the trail.

The reason removing exhaustivity = .off appeared to resolve the problem was because it forced me to override the dependency as I had to handle the actions that were resulting from it. 🤦‍♂️

So, I think there is still an issue here (in that the failure is displaying on the wrong test), but I don't know if that's swift-dependencies' issue, or Xcode's issue...

In any case, I have included the project here for you to see @mbrandonw, in case you see something I don't and disagree with my analysis? DependenciesTestIssue.zip

Screenshot 2023-04-28 at 16 39 17 Screenshot 2023-04-28 at 16 39 28

Interestingly, I noticed as well that I had forgotten to include the async keyword on the test that used exhaustivity = .off. Adding it, causes the test failure to appear in the correct place. 🤷‍♂️

Screenshot 2023-04-28 at 16 40 37
tgrapperon commented 1 year ago

Hey @james-rantmedia! Thanks for the repro, but the file you're attaching isn't complete. It seems it lacks the MyLibrary package that was not included in the archive.

james-rant commented 1 year ago

@tgrapperon Apologies, not sure what happened there -- the package was in another location outside of the project I'd created for it... Try this one: DependenciesTestIssue.zip

tgrapperon commented 1 year ago

Hey @james-rantmedia! No worries, thanks for the update! In your second test, the failure is expected because you're not overriding the dependency value and its testing value is unimplemented by default. It happens automatically when you use the trailing closure of TestStore, but you can also set it directly:

let store = TestStore(…)
store.dependency.myDependency.getID = { UUID() }
store.send(…) 

This is independent of exhaustivity, as this only affects checks. The dependency will be hit anyway, and this will trigger a test failure. Whether or not exhaustivity should affect unimplemented dependencies is indeed interesting, but I'm afraid it's not possible to make it work in the general picture, as unimplemented dependencies will likely produce unexpected/meaningless values anyway, even if they don't fail the test while doing so.

Please note that setting the dependency by acting on TestStore.dependencies is not strictly equivalent to the first test with a trailing closure, because the dependencies you're defining here will also surround initialState's creation. In the second case, they'll only surround reducers. So I'd recommend to use the trailing closure in general.

That being said, I don't know how much this issue was related to the original question.

james-rant commented 1 year ago

@tgrapperon I understand that the failure is expected because the second test wasn't overriding the dependency (I eventually realised that was the problem while writing the reproduction case). But my confusion is more to do with the fact that the failure appears on the first test when it should appear on the second (when async isn't included on the function declaration). And also that when the async isn't included, the test passes when run in isolation. What's going on there? (That part may be more related to the original question, where they state that unrelated tests that do not depend on the dependency exhibit failures due to unimplemented state).

tgrapperon commented 1 year ago

Ah yeah, I can see that now. Yeah, this can probably be improved. My guess is that the sync test runs first and its dependencies logic is leaking and resetting the first test dependencies in flight in some way, between the TestStore creation and the dependency access. The success is kind of expected for the sync test because the test reaches its end without failing and you disabled exhaustivity, but it shouldn't affect the other tests.

onurhuseyincantay commented 1 year ago

Hello everyone, Thanks for taking the time and also putting some effort into it. I made a simple project and was able to reproduce the failure. the project makes use of Tuist in order to start the project you will have to take 2 actions.

  1. make install (installs Tuist)
  2. make project (fetches dependencies and generates xcworkspace)

if you run module specific unit tests you will see that they are passing if you run app specific unit tests you will see that they are not passing.

dependencies-issue.zip

mbrandonw commented 1 year ago

Hi @onurhuseyincantay, we don't personally use Tuist, so is it possible to reproduce the problem in a plain Xcode project just to make sure it's an issue with our library and not Tuist? We have had issues with Tuist in the past that had nothing to do with our libraries.

FWIW, I copied and pasted your code into a fresh, plain Xcode project and it works just fine. So it would seem to me to be an error with how Tuist is configuring your project.

onurhuseyincantay commented 1 year ago

@mbrandonw that might be a reason indeed nevertheless the behaviour is not so clear where it comes from would you be kind to share the project where you couldn't reproduce so that I can compare on my side what exactly is missing ?

tgrapperon commented 1 year ago

Hey @bocato & @onurhuseyincantay! Can you both confirm that you're not seeing any message like:

Class _TtC12Dependencies[…] is implemented in both […] and […]. One of the two will be used. Which one is undefined.

in the console? (cf. the last section of the Testing Gotchas). The issue seems very close to Tuist's sphere of influence.

bocato commented 1 year ago

Did the same test you did @mbrandonw, but with SPM modules. Also found no issues… My hunch wast that the issue is related to the way tuist is linking the targets. I changed the way the target is linked from static (default on tuist) to dynamic, and it all worked fine. I just had to explicitly declare Dependencies to be linked as a dynamic framework in tuist.

let dependencies: Dependencies = .init(
    swiftPackageManager: .init(
        [
            .remote(
                url: "https://github.com/pointfreeco/swift-dependencies",
                    requirement: .upToNextMinor(from: "0.1.2")
            )
        ],
        productTypes: [
            "Dependencies": .framework
        ]
    ),
    platforms: [.iOS]
)

Here is @onurhuseyincantay's demo with the change: dependencies-issue_fixed.zip

onurhuseyincantay commented 1 year ago

@mbrandonw @tgrapperon the initial issue seems to be relevant to leaking dependencies and project structure in general but nevertheless when the test cases are run in parallel and random order even when there is a leak the failure is shown in unrelevant test cases if that could be solved that would be very nice as mentioned here

Ah yeah, I can see that now. Yeah, this can probably be improved. My guess is that the sync test runs first and its dependencies logic is leaking and resetting the first test dependencies in flight in some way, between the TestStore creation and the dependency access. The success is kind of expected for the sync test because the test reaches its end without failing and you disabled exhaustivity, but it shouldn't affect the other tests.

mbrandonw commented 11 months ago

This issue was reference from #127 where another issue of leaking dependencies was brought up. I am still of the mindset that this is not an issue with the library. It is most likely either an issue with how the library is being linked (as mentioned in the docs), or dependencies are being accessed in an escaping closure, which makes it possible for one test's execution to bleed over into another.

I am going to close this issue for now, but please do open a new discussion if you find something new.