Robolectric App tests get a closed Koin in composables #1557

yschimke · 2023-04-10T11:40:22Z

Describe the bug

Using Koin with Robolectric, fails with

Scope '_root_' is closed
org.koin.core.error.ClosedScopeException: Scope '_root_' is closed
	at org.koin.core.scope.Scope.resolveInstance(Scope.kt:221)
	at org.koin.core.scope.Scope.get(Scope.kt:210)
	at org.koin.androidx.viewmodel.factory.KoinViewModelFactory.create(KoinViewModelFactory.kt:25)
	at androidx.lifecycle.ViewModelProvider.get(ViewModelProvider.kt:187)
	at androidx.lifecycle.ViewModelProvider.get(ViewModelProvider.kt:153)
	at org.koin.androidx.viewmodel.GetViewModelKt.resolveViewModel(GetViewModel.kt:44)

When it fails, these return different Koin instances in the composable

KoinPlatformTools.defaultContext().get() -> the fresh instance
LocalKoinApplication.current -> the instance from the first test

To Reproduce
Steps to reproduce the behavior:

Checkout https://github.com/joreilly/Confetti/, specifically this PR Enable wear app tests joreilly/Confetti#580
Run tests in wearApp/src/test
Disable the overrides in MainActivity
Run tests again

** workaround **

In main app composable, override

https://github.com/joreilly/Confetti/pull/580/files#diff-0548d0af1b708341bc3c142fbf71b1270fb90f0653f791f2eea9e92867d2e99f

        setContent {
            ConfettiApp(navController, intent)
            // This shouldn't be needed, but allows robolectric tests to run successfully
            // TODO remove once a solution is found or a fix in koin?
            CompositionLocalProvider(
                LocalKoinScope provides KoinPlatformTools.defaultContext().get().scopeRegistry.rootScope,
                LocalKoinApplication provides KoinPlatformTools.defaultContext().get()
            ) {
...

Expected behavior
Tests should pass as there is a fresh Koin instance for the current test

Koin project used and used version (please complete the following information):
3.4.0

The text was updated successfully, but these errors were encountered:

chankeiro · 2023-04-11T17:48:42Z

I am experiencing the same issue with koin-androidx-compose:3.4.3. With koin-androidx-compose:3.4.1 + koin-android;3.4.0 everything works fine. The error I get is exactly the same:

Caused by org.koin.core.error.ClosedScopeException: Scope '_root_' is closed
       at org.koin.core.scope.Scope.resolveInstance(Scope.kt:221)
       at org.koin.core.scope.Scope.get(Scope.kt:210)
       at org.koin.androidx.viewmodel.factory.KoinViewModelFactory.create(KoinViewModelFactory.kt:25)
       at androidx.lifecycle.ViewModelProvider.get(ViewModelProvider.kt:187)
       at androidx.lifecycle.ViewModelProvider.get(ViewModelProvider.kt:153)
       at org.koin.androidx.viewmodel.GetViewModelKt.resolveViewModel(GetViewModel.kt:44)

jjkester · 2023-04-14T14:00:25Z

I experienced this as well in an Android (connected) test. For me the first test that executes is OK, a subsequent one fails.

Did some debugging and the root cause seems to be the composition locals that Koin uses, LocalKoinApplication and LocalKoinScope.

The first time that these composition locals are read, the values are initialized. This means fetching the current Koin application from the default Koin context. All is fine.

Before starting the second test (using KoinTestRule), a new Koin application is created. However, the composition locals are not initialized again. Hence, any Koin function in Compose is returning an application which has been closed.

Only retrieving the Koin application once is fine for production scenarios, but does not work well for tests.

I found the same workaround as described in the original issue, so that works for me (outside of Robolectric) as well.

Using:

io.insert-koin:koin-android:3.4.0
io.insert-koin:koin-androidx-compose:3.4.3
io.insert-koin:koin-test:3.4.0
io.insert-koin:koin-test-junit4:3.4.0
Jetpack Compose BOM 2023.01.00

okycelt · 2023-04-18T07:40:30Z

We're experiencing this issue in production as well. In one scenario, we need to stop koin, start koin with updated modules and restart the main activity.

stopKoin()
startKoin { /* modules */ }

val componentName = ComponentName(context, MainActivity::class.java)
startActivity(Intent.makeRestartActivityTask(componentName))

When creating the new activity, we get the crash when first koinInject() is called.

Caused by: org.koin.core.error.ClosedScopeException: Scope '_root_' is closed
	at org.koin.core.scope.Scope.resolveInstance(Scope.kt:221)
	at org.koin.core.scope.Scope.get(Scope.kt:210)

Workaround described in the original issue seems to be working for us too. We're using:

io.insert-koin:koin-android:3.4.0
io.insert-koin:koin-androidx-compose:3.4.3
Jetpack Compose BOM 2023.03.00

arnaudgiuliani · 2023-05-10T16:58:53Z

need to check 👌

arnaudgiuliani · 2023-05-16T17:30:56Z

@jjkester good catch:

Did some debugging and the root cause seems to be the composition locals that Koin uses, LocalKoinApplication and LocalKoinScope.

The first time that these composition locals are read, the values are initialized. This means fetching the current Koin application from the default Koin context. All is fine.

Before starting the second test (using KoinTestRule), a new Koin application is created. However, the composition locals are not initialized again. Hence, any Koin function in Compose is returning an application which has been closed.

I've introduced LocalKoinApplication and LocalKoinScope to help Compose idiomatic way to handle Scope & Application instance for Koin. Need to check how and why it's running like this with RobotElecltric

jjkester · 2023-05-16T19:29:10Z

@arnaudgiuliani They run like that for instrumented tests on devices as well (we're not using Robolectric).

I do like the Compose API (koinInject!) by the way, and it works brilliantly in production. However, during tests the lifecycles of things is a bit different. The lifecycle of the Koin application is for example shorter than the lifecycle of the Android application. My guesstimation is that is where the issue is.

As far as I have gathered we're running into some design decision in the compose runtime. The lambda function with which you provide the default value when defining the CompositionLocal is only evaluated once, and Compose will keep the result of that evaluation for as long as possible. I have peeked at the source code of androidx.compose.runtime.CompositionLocal<T> and the factory lambda is ending up in a Kotlin lazy delegate. This is by the way also why reproviding (as in the workaround in the original post) works, because then Compose will actually reevaluate the expression to obtain the Koin context on every recomposition, and when it changes, invalidate everything that uses it so propagate that change.

For us the workaround works fine when testing a whole activity, because we only have to provide a value for the CompositionLocals once in the Activity's setContent, but when testing Composables that use Koin all test cases would need wrapping which is not that nice. Of course there is extension functions to consider, but ideally it should just work.

Compose does offer the LocalContext by default, maybe you can leverage that to rely on the Android Context to retrieve the current KoinApplication, since that should change between tests (at least, for Android instrumented tests).

The Koin application is looked up in the Context tree and the Koin CompositionLocals are provided with/from this value. This (still) requires this function to be called around the first time Koin is used from Compose. A practical place for this is the setContent function. This means that (boilerplate) code needs to be added to every Activity or Fragment using Compose, and every test case where a Composable is tested in isolation. Untested - will need verification with these test frameworks! Fixes InsertKoinIOgh-1557

jjkester · 2023-05-16T20:37:38Z

I have had a look at the use cases that Koin has for these CompositionLocals. It is clear to me that they serve an important purpose, so refactoring them out does not seem feasible.

To take another direction, I have attempted to create a composable function which will look up the appropriate Koin application and provide it to the composition, based on the workaround we already have in our code and the workaround posted in the first post. It really needs to be tested that it covers all the use cases of Koin, and that it works for the different test frameworks.

The first one was not feasible for me to have a look at today (if at all - I will need some pointers!), and the second one would introduce a whole new class of tests to this repository (instrumented tests are not straightforward to run on a CI pipeline) so that does require some thinking (or input from @arnaudgiuliani) before attempting it in my opinion.

Code can be found in my fork: jjkester@7bf2a84

arnaudgiuliani · 2023-05-17T13:11:25Z

yes interesting @jjkester the KoinApplication composable function already exist in the common koin-compose module, would require your findContextForKoin function to be sure to catchup the right instance

jjkester · 2023-05-17T14:34:40Z

Indeed, that function already exists but the difference being that the application already has been defined outside of Compose.

The context function has been based on this one from Accompanist, difference being that it is fine to fall back on the application context as that is the root one.

arnaudgiuliani · 2023-05-17T15:51:21Z

do you want to make a PR? else I can add it directly. That implies forcing to use KoinApplication composable function. Still checking if it becomes necessary or not.

jjkester · 2023-05-17T19:47:59Z

I'm happy to create a PR. I do have some more ideas though, because as I tried to explain earlier this may not cover all use cases (mainly testing standalone composables using ComposeContentTestRule).

This solution does feel like a workaround, but I'm not confident there is a real, practical solution. (I'm also not confident that there is none at all...). So maybe a bit more investigation is warranted.

If I would be contributing the PR I do want to accompany it with a (regression) test suite to show that this indeed solves the issue. Not sure if it is sufficient to try and replicate the behaviour of the test frameworks, or whether it would be better to create actual tests using these actual frameworks. But as I mentioned before, we should consider where to put those and how to run them (especially the Android instrumented tests). Do you have any ideas about that?

By throwing an exception as default factory for the CompositionLocals we signal that there is no explicit value set. This will trigger the default lookup behavior that used to be the result of the factory function, as long as the appropriate functions getKoin() and getKoinScope() are used. By using the internal Compose API we are able to catch the exception to run the lookup code. We remember the result of the try/catch block to ensure we only incur the overhead of the exception once per `getKoin()` call per composition. Performance analysis and testing necessary! Fixes InsertKoinIOgh-1557

arnaudgiuliani · 2023-05-29T15:58:02Z

yes, still wondering if it's a "robotelectric" case only of if this has to be part of the API design 🤔
I'm checking your PR

okycelt · 2023-05-29T16:16:42Z

yes, still wondering if it's a "robotelectric" case only of if this has to be part of the API design 🤔
I'm checking your PR

@arnaudgiuliani, for us it's not only a robolectric case, see #1557 (comment)

jjkester · 2023-05-29T16:41:33Z

I am not using Robolectric, so for sure not only an issue with that. Also with the Android Instrumented tests.

While running Android tests the application process stays alive, and the compositionLocal is not re-initialized after the test rule created a new Koin application. Wondering whether this is the same on Compose multiplatform actually, but I have no experience with that. Might write a small test case in my PR.

arnaudgiuliani · 2023-05-30T07:49:48Z

ok great

gdesantos · 2023-07-31T06:33:20Z

Any news about this?

arnaudgiuliani · 2023-08-04T14:35:16Z

still on hold, need to follow up for next release 👍

Kevinrob · 2023-08-11T14:16:00Z

ℹ️ We also have this crash on production.
We let the user switch the language in-app and we stop/start Koin.

Our workaround is to restart the full process with ProcessPhoenix:
ProcessPhoenix.triggerRebirth(context)

…Provider to avoid outdated link to Koin scope - Fix #1557

arnaudgiuliani · 2023-09-08T14:49:11Z

koin-compose 1.1.0 & koin-androidx-compose 3.5.0 will bring KoinContext & KoinAndroidContext to setup compose with the right CompositionLocalProvider, depending on the current context (default context or android one)

61a88bb

Let's see how it goes with this.

We can replace your snippet with KoinAndroidContext @yschimke

yschimke changed the title ~~Robolectric App tests get an closed Koin in composables~~ Robolectric App tests get a closed Koin in composables Apr 10, 2023

arnaudgiuliani added android compose test status:checking currently in analysis - discussion or need more detailed specs labels May 10, 2023

arnaudgiuliani added this to the android-3.4.1 milestone May 10, 2023

arnaudgiuliani self-assigned this May 10, 2023

jjkester mentioned this issue May 17, 2023

Avoid retaining outdated LocalKoinApplication/LocalKoinScope #1586

Draft

6 tasks

arnaudgiuliani modified the milestones: android-3.4.1, android-compose-3.4.4 May 30, 2023

arnaudgiuliani added a commit that referenced this issue Sep 8, 2023

add KoinContext & KoinAndroidContext with check over CompositionLocal…

61a88bb

…Provider to avoid outdated link to Koin scope - Fix #1557

arnaudgiuliani closed this as completed Sep 8, 2023

yschimke mentioned this issue Sep 16, 2023

Koin 3.5.0 joreilly/Confetti#875

Merged

nowakweronika mentioned this issue Jan 24, 2024

chore(deps): bump koin from 3.5.0 to 3.5.3 appunite/Loudius#200

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Robolectric App tests get a closed Koin in composables #1557

Robolectric App tests get a closed Koin in composables #1557

yschimke commented Apr 10, 2023 •

edited

chankeiro commented Apr 11, 2023 •

edited

jjkester commented Apr 14, 2023

okycelt commented Apr 18, 2023

arnaudgiuliani commented May 10, 2023

arnaudgiuliani commented May 16, 2023

jjkester commented May 16, 2023

jjkester commented May 16, 2023

arnaudgiuliani commented May 17, 2023

jjkester commented May 17, 2023

arnaudgiuliani commented May 17, 2023 •

edited

jjkester commented May 17, 2023

arnaudgiuliani commented May 29, 2023 •

edited

okycelt commented May 29, 2023

jjkester commented May 29, 2023

arnaudgiuliani commented May 30, 2023

gdesantos commented Jul 31, 2023

arnaudgiuliani commented Aug 4, 2023

Kevinrob commented Aug 11, 2023

arnaudgiuliani commented Sep 8, 2023 •

edited

Robolectric App tests get a closed Koin in composables #1557

Robolectric App tests get a closed Koin in composables #1557

Comments

yschimke commented Apr 10, 2023 • edited

chankeiro commented Apr 11, 2023 • edited

jjkester commented Apr 14, 2023

okycelt commented Apr 18, 2023

arnaudgiuliani commented May 10, 2023

arnaudgiuliani commented May 16, 2023

jjkester commented May 16, 2023

jjkester commented May 16, 2023

arnaudgiuliani commented May 17, 2023

jjkester commented May 17, 2023

arnaudgiuliani commented May 17, 2023 • edited

jjkester commented May 17, 2023

arnaudgiuliani commented May 29, 2023 • edited

okycelt commented May 29, 2023

jjkester commented May 29, 2023

arnaudgiuliani commented May 30, 2023

gdesantos commented Jul 31, 2023

arnaudgiuliani commented Aug 4, 2023

Kevinrob commented Aug 11, 2023

arnaudgiuliani commented Sep 8, 2023 • edited

yschimke commented Apr 10, 2023 •

edited

chankeiro commented Apr 11, 2023 •

edited

arnaudgiuliani commented May 17, 2023 •

edited

arnaudgiuliani commented May 29, 2023 •

edited

arnaudgiuliani commented Sep 8, 2023 •

edited