patrykandpatrick / vico

A light and extensible chart library for Android.
https://patrykandpatrick.com/vico/wiki
Apache License 2.0
2.08k stars 125 forks source link

Not able to grab a screenshot test of a graph #537

Closed aleksandra-krzemien closed 8 months ago

aleksandra-krzemien commented 8 months ago

How to reproduce

Open project from here and run task recordRoborazziDebug.

The test being run is RoborazziTest and the screenshot can be found in build/outputs/roborazzi/

Observed behavior

I was experimenting with simple screenshot tests with roborazzi, and I noticed that Vico graphs are not being captured by screenshot.

I'm not fully sure if this is an issue with vico, but in the runtime everything is ok, and all other composable components are being screenshoot correctly, it seems like vico issue. Zrzut ekranu 2024-01-12 o 18 14 09

Expected behavior

I'd expect the graph to be drawn in a similar way it's drawn on the preview Zrzut ekranu 2024-01-12 o 18 15 53

Vico version(s)

Latest stable version

Android version(s)

Android 13

Additional information

No response

Gowsky commented 8 months ago

Hi @aleksandra-krzemien, I suppose you're using ChartEntryModelProducer in your tests. If so, that's the reason. Vico processes data asynchronously, and Roborazzi (or Paparazzi we're using) may not allow Vico to render itself after it processes the data.

To solve this problem, I'd suggest creating ChartEntryModel manually, like we do here for a chart preview like this used for Paparazzi tests. I hope that it resolves your problem.

patrickmichalik commented 8 months ago

Hello! Generally, @Gowsky’s solution would be the simplest approach, but if you’d like to use ChartEntryModelProducer here, it should be possible. I’ve taken a look at your code and noticed that you’ve commented out a solution that incorporates LaunchedEffect and setEntriesSuspending. This should be compatible with screenshot tests, and it works with Paparazzi in the Vico project. Just to confirm, did you face issues with this approach too? (I was unable to test this myself because when I tried to run recordRoborazziDebug in your project, I got a relatively cryptic error message about abc_vector_test.)

I also wanted to let you know about an issue with your setup. This is unrelated to the test problem, but it’s generally noteworthy. When using ChartEntryModelProducer, rather than creating a ChartEntryModel, one should create a ChartEntry list. I’d suggest using the following and passing the output to setEntries.

entriesOf(*dataPoints)

In your current setup, two ChartEntryModelProducers and two ChartEntryModels are created, with the first ChartEntryModel’s generation blocking the main thread. The main benefit of ChartEntryModelProducer is thus lost. Both the above and page.dataPoints.map would best be moved off the main thread.

aleksandra-krzemien commented 8 months ago

To solve this problem, I'd suggest creating ChartEntryModel manually

The case is that in my regular project screenshot tests are more wired to the screen setup (fragment, viewModel, etc), so I can't change the UI logic just for the test, because then the test would be pretty much useless.

you’ve commented out a solution that incorporates LaunchedEffect and setEntriesSuspending

I commented it out because with that approach the composable preview did not show data on the graph, and I figured it might be connected to the screenshot test issue. With regular setEntries the preview looks ok. When I use setEntriesSuspending (with LaunchedEffect) instead of setEntries I get the same result - the graph data on the screenshot is not visible.

I got a relatively cryptic error message about abc_vector_test

That is super strange. AFAICS abc_vector_test resource is used in getDrawable function, which I'm not using directly anywhere in my example 🤔 Can you paste some stacktrace of this issue?

Also, thank you for the hint about ChartEntryModel. I'm pretty new to compose, so I may not understand everything related to recompositions and stuff.

In your current setup, two ChartEntryModelProducers and two ChartEntryModels are created

Is this related to the fact that two pager pages are created or is it related to some compose stuff? (sorry for dumb questions, but I want to make sure if I understand the issue correctly)

takahirom commented 8 months ago

I think you should override the Default Dispatcher in the test to correct the issue. However, I haven't tried this myself. In the following file, the dispatcher is not specified: https://github.com/aleksandra-krzemien/nested-nav-graphs-example/blob/krzemien/compose-tests-with-animations/app/src/main/java/com/example/navgraphapplication/SecondFragment.kt#L136 Here is the default dispatcher: https://github.com/patrykandpatrick/vico/blob/v1/vico/core/src/main/java/com/patrykandpatrick/vico/core/entry/ChartEntryModelProducer.kt#L47

aleksandra-krzemien commented 8 months ago

I got the screenshot tests working using

LaunchedEffect(dataPoints) {
    entryProducer.setEntriesSuspending(entriesOf(*dataPoints)).await()
}

and

composeTestRule.waitForIdle()

instead of playing with composeTestRule.mainClock

but now I started to observe this issue again in my regular project (both in runtime and in screenshot tests) - but not in the example repo - so I'm not sure if this is the right approach, but I'm just hitting a bug, or if I still miss something 😅

patrickmichalik commented 8 months ago

I’m glad you’ve found a solution, @aleksandra-krzemien. FWIW, here’s the test stack trace:

android.content.res.Resources$NotFoundException: Drawable com.example.navgraphapplication:drawable/abc_vector_test with resource ID #0x7f070077
Caused by: android.content.res.Resources$NotFoundException: File res/drawable/abc_vector_test.xml from drawable resource ID #0x7f070077
    at android.content.res.ResourcesImpl.loadDrawableForCookie(ResourcesImpl.java:875)
    at android.content.res.ResourcesImpl.loadDrawable(ResourcesImpl.java:669)
    at java.base@17.0.9/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at java.base@17.0.9/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
    at java.base@17.0.9/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at org.robolectric.shadows.ShadowArscResourcesImpl$ResourcesImplReflector$$Reflector20.loadDrawable(Unknown Source)
    at org.robolectric.shadows.ShadowArscResourcesImpl.loadDrawable(ShadowArscResourcesImpl.java:169)
    at android.content.res.ResourcesImpl.loadDrawable(ResourcesImpl.java)
    at android.content.res.Resources.loadDrawable(Resources.java:1002)
    at android.content.res.Resources.getDrawableForDensity(Resources.java:992)
    at android.content.res.Resources.getDrawable(Resources.java:931)
    at android.content.Context.getDrawable(Context.java:810)
    at androidx.core.content.ContextCompat$Api21Impl.getDrawable(ContextCompat.java:1110)
    at androidx.core.content.ContextCompat.getDrawable(ContextCompat.java:526)
    at androidx.appcompat.widget.ResourceManagerInternal.getDrawable(ResourceManagerInternal.java:149)
    at androidx.appcompat.widget.ResourceManagerInternal.getDrawable(ResourceManagerInternal.java:137)
    at androidx.appcompat.widget.ResourceManagerInternal.checkVectorDrawableSetup(ResourceManagerInternal.java:506)
    at androidx.appcompat.widget.ResourceManagerInternal.getDrawable(ResourceManagerInternal.java:142)
    at androidx.appcompat.widget.AppCompatDrawableManager.getDrawable(AppCompatDrawableManager.java:480)
    at androidx.appcompat.widget.TintTypedArray.getDrawableIfKnown(TintTypedArray.java:94)
    at androidx.appcompat.app.AppCompatDelegateImpl.attachToWindow(AppCompatDelegateImpl.java:872)
    at androidx.appcompat.app.AppCompatDelegateImpl.ensureWindow(AppCompatDelegateImpl.java:848)
    at androidx.appcompat.app.AppCompatDelegateImpl.onCreate(AppCompatDelegateImpl.java:555)
    at androidx.appcompat.app.AppCompatActivity$2.onContextAvailable(AppCompatActivity.java:133)
    at androidx.activity.contextaware.ContextAwareHelper.dispatchOnContextAvailable(ContextAwareHelper.kt:84)
    at androidx.activity.ComponentActivity.onCreate(ComponentActivity.java:358)
    at androidx.fragment.app.FragmentActivity.onCreate(FragmentActivity.java:216)
    at android.app.Activity.performCreate(Activity.java:8290)
    at android.app.Activity.performCreate(Activity.java:8269)
    at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1384)
    at org.robolectric.android.internal.RoboMonitoringInstrumentation.callActivityOnCreate(RoboMonitoringInstrumentation.java:377)
    at org.robolectric.android.controller.ActivityController.lambda$create$0(ActivityController.java:138)
    at org.robolectric.shadows.ShadowPausedLooper.runPaused(ShadowPausedLooper.java:227)
    at org.robolectric.android.controller.ActivityController.create(ActivityController.java:136)
    at org.robolectric.android.controller.ActivityController.create(ActivityController.java:146)
    at org.robolectric.android.internal.RoboMonitoringInstrumentation.lambda$startActivitySyncInternal$0(RoboMonitoringInstrumentation.java:125)
    at org.robolectric.shadows.ShadowInstrumentation.runOnMainSyncNoIdle(ShadowInstrumentation.java:1201)
    at org.robolectric.android.internal.RoboMonitoringInstrumentation.startActivitySyncInternal(RoboMonitoringInstrumentation.java:120)
    at org.robolectric.android.internal.LocalActivityInvoker.startActivity(LocalActivityInvoker.java:36)
    at org.robolectric.android.internal.LocalActivityInvoker.startActivity(LocalActivityInvoker.java:41)
    at androidx.test.core.app.ActivityScenario.launchInternal(ActivityScenario.java:362)
    at androidx.test.core.app.ActivityScenario.launch(ActivityScenario.java:202)
    at androidx.test.ext.junit.rules.ActivityScenarioRule.lambda$new$0(ActivityScenarioRule.java:77)
    at androidx.test.ext.junit.rules.ActivityScenarioRule$$ExternalSyntheticLambda1.get(D8$$SyntheticClass)
    at androidx.test.ext.junit.rules.ActivityScenarioRule.before(ActivityScenarioRule.java:110)
    at app//org.junit.rules.ExternalResource$1.evaluate(ExternalResource.java:50)
    at androidx.compose.ui.test.junit4.AndroidComposeTestRule$apply$1$evaluate$1.invoke(AndroidComposeTestRule.android.kt:272)
    at androidx.compose.ui.test.junit4.AndroidComposeTestRule$apply$1$evaluate$1.invoke(AndroidComposeTestRule.android.kt:271)
    at androidx.compose.ui.test.AndroidComposeUiTestEnvironment$AndroidComposeUiTestImpl.withDisposableContent(ComposeUiTest.android.kt:491)
    at androidx.compose.ui.test.AndroidComposeUiTestEnvironment$runTest$1$1$1$1$1$1.invoke(ComposeUiTest.android.kt:323)
    at androidx.compose.ui.test.AndroidComposeUiTestEnvironment.withComposeIdlingResource(ComposeUiTest.android.kt:375)
    at androidx.compose.ui.test.AndroidComposeUiTestEnvironment.access$withComposeIdlingResource(ComposeUiTest.android.kt:228)
    at androidx.compose.ui.test.AndroidComposeUiTestEnvironment$runTest$1$1$1$1$1.invoke(ComposeUiTest.android.kt:322)
    at androidx.compose.ui.test.AndroidComposeUiTestEnvironment.withWindowRecomposer(ComposeUiTest.android.kt:349)
    at androidx.compose.ui.test.AndroidComposeUiTestEnvironment.access$withWindowRecomposer(ComposeUiTest.android.kt:228)
    at androidx.compose.ui.test.AndroidComposeUiTestEnvironment$runTest$1$1$1$1.invoke(ComposeUiTest.android.kt:321)
    at androidx.compose.ui.test.AndroidComposeUiTestEnvironment.withTestCoroutines(ComposeUiTest.android.kt:362)
    at androidx.compose.ui.test.AndroidComposeUiTestEnvironment.access$withTestCoroutines(ComposeUiTest.android.kt:228)
    at androidx.compose.ui.test.AndroidComposeUiTestEnvironment$runTest$1$1$1.invoke(ComposeUiTest.android.kt:320)
    at androidx.compose.ui.test.junit4.IdlingStrategy.withStrategy(IdlingStrategy.android.kt:52)
    at androidx.compose.ui.test.AndroidComposeUiTestEnvironment$runTest$1$1.invoke(ComposeUiTest.android.kt:319)
    at androidx.compose.ui.test.junit4.IdlingResourceRegistry.withRegistry(IdlingResourceRegistry.jvm.kt:157)
    at androidx.compose.ui.test.AndroidComposeUiTestEnvironment$runTest$1.invoke(ComposeUiTest.android.kt:318)
    at androidx.compose.ui.test.junit4.ComposeRootRegistry.withRegistry(ComposeRootRegistry.android.kt:146)
    at androidx.compose.ui.test.AndroidComposeUiTestEnvironment.runTest(ComposeUiTest.android.kt:317)
    at androidx.compose.ui.test.junit4.AndroidComposeTestRule$apply$1.evaluate(AndroidComposeTestRule.android.kt:271)
    at app//org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
    at app//org.robolectric.RobolectricTestRunner$HelperTestRunner$1.evaluate(RobolectricTestRunner.java:588)
    at app//org.robolectric.internal.SandboxTestRunner$2.lambda$evaluate$2(SandboxTestRunner.java:290)
    at app//org.robolectric.internal.bytecode.Sandbox.lambda$runOnMainThread$0(Sandbox.java:101)
    at java.base@17.0.9/java.util.concurrent.FutureTask.run(FutureTask.java:264)
    at java.base@17.0.9/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
    at java.base@17.0.9/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
    at java.base@17.0.9/java.lang.Thread.run(Thread.java:840)
Caused by: java.lang.IllegalArgumentException: resource native/windows/x86_64/robolectric-nativeruntime.dll not found.
    at com.google.common.base.Preconditions.checkArgument(Preconditions.java:220)
    at com.google.common.io.Resources.getResource(Resources.java:194)
    at org.robolectric.nativeruntime.DefaultNativeRuntimeLoader.loadLibrary(DefaultNativeRuntimeLoader.java:154)
    at org.robolectric.nativeruntime.DefaultNativeRuntimeLoader.lambda$ensureLoaded$0(DefaultNativeRuntimeLoader.java:84)
    at org.robolectric.util.PerfStatsCollector.measure(PerfStatsCollector.java:86)
    at org.robolectric.nativeruntime.DefaultNativeRuntimeLoader.ensureLoaded(DefaultNativeRuntimeLoader.java:74)
    at org.robolectric.nativeruntime.DefaultNativeRuntimeLoader.injectAndLoad(DefaultNativeRuntimeLoader.java:54)
    at org.robolectric.shadows.ShadowNativeVectorDrawable.nCreateGroup(ShadowNativeVectorDrawable.java:153)
    at android.graphics.drawable.VectorDrawable.nCreateGroup(VectorDrawable.java)
    at android.graphics.drawable.VectorDrawable$VGroup.__constructor__(VectorDrawable.java:1352)
    at android.graphics.drawable.VectorDrawable$VGroup.<init>(VectorDrawable.java)
    at android.graphics.drawable.VectorDrawable$VectorDrawableState.__constructor__(VectorDrawable.java:1008)
    at android.graphics.drawable.VectorDrawable$VectorDrawableState.<init>(VectorDrawable.java)
    at android.graphics.drawable.VectorDrawable.__constructor__(VectorDrawable.java:364)
    at android.graphics.drawable.VectorDrawable.<init>(VectorDrawable.java)
    at android.graphics.drawable.DrawableInflater.inflateFromTag(DrawableInflater.java:167)
    at android.graphics.drawable.DrawableInflater.inflateFromXmlForDensity(DrawableInflater.java:136)
    at android.graphics.drawable.Drawable.createFromXmlInnerForDensity(Drawable.java:1394)
    at android.graphics.drawable.Drawable.createFromXmlForDensity(Drawable.java:1355)
    at android.content.res.ResourcesImpl.loadXmlDrawable(ResourcesImpl.java:907)
    at android.content.res.ResourcesImpl.loadDrawableForCookie(ResourcesImpl.java:861)
    ... 73 more

About the ChartEntryModelProducer issue, entryModelOf takes the data you give it, creates a ChartEntry list, creates a ChartEntryModelProducer, and uses it to synchronously create a ChartEntryModel. In your example, the ChartEntry list from the generated ChartEntryModel is passed to another ChartEntryModelProducer (entryProducer), which creates another ChartEntryModel. The intermediate ChartEntryModel is redundant. All you need is a ChartEntry list, which you can create via entriesOf, as in the snippet from your latest comment. One concern remains: the mapping of data on the main thread on every recomposition. This can significantly degrade performance. As such, both entriesOf and page.dataPoints.map should be moved off the main thread. I’d suggest modifying your solution as follows. When paired with waitForIdle, this should continue to work with screenshot tests. (If it doesn’t, please let me know, and we’ll take a closer look.)

LaunchedEffect(page.dataPoints) {
    withContext(Dispatchers.Default) {
        entryProducer
            .setEntriesSuspending(entriesOf(*page.dataPoints.map { it.x to it.y }.toTypedArray()))
            .await()
    }
}

Regarding the IllegalStateException problem, it occurs only in specific circumstances, but it’s a Vico bug, and it doesn’t point to user error. As mentioned here, the problem has been resolved with Vico 2.0.0 Alpha 5. We’ll also be releasing a Vico 1 update that addresses it.

@takahirom, regarding your suggestion, I believe that would work, but since the setup here isn’t test-specific, it would be preferable not to have ChartEntryModelProducer block the main thread.

aleksandra-krzemien commented 8 months ago

@patrickmichalik as for the drawable exception, it seems that Robolectric's RNG doesn't work on Windows, so you'd have to use @GraphicsMode(GraphicsMode.Mode.LEGACY) in the RoborazziTest.

Thank you for the explanation. About proposed solution, I can confirm that for the example project I linked it works fine. In my regular project I hit IllegalStateException pretty much every time when using withContext(Dispatchers.Default) and from time to time without it, so probably there is an issue with my setup. There isn't any alpha version of Vico 1 with this issue fixed so I can check if I still get the exception, right?

patrickmichalik commented 8 months ago

Thanks for the tip, @aleksandra-krzemien! I work on both Windows and macOS, so I’ll keep in mind to use the latter with Roboelectric.

I’m glad the suggested solution works as expected. Regarding the IllegalStateException issue, even if the exception occurs frequently, it almost certainly isn’t a problem with your setup. This exception points to an internal synchronization issue, and we’ve identified the cause. There isn’t a Vico 1 alpha with a fix yet, but we’re working on porting the fix from Vico 2 to Vico 1 as we speak, and an alpha will be released as soon as this is done.

aleksandra-krzemien commented 8 months ago

Ok, so I guess there is nothing more to discuss in this issue for now, and you can close it if you prefer. I'll update to the new Vico version once it's available and check if everything works as expected.

patrickmichalik commented 8 months ago

Sounds good. I’ll let you know when the update is available. I’m using “Close as not planned” because the issue with screenshot tests turned out not to be a Vico bug.

patrickmichalik commented 8 months ago

The update, Vico 1.14.0 Alpha 2, is now available, @aleksandra-krzemien.

aleksandra-krzemien commented 8 months ago

I can confirm that with this version both my app and my screenshot tests work correctly without any exceptions 🎉

patrickmichalik commented 8 months ago

I’m glad to hear that!