UI Tests: fix crash when running multiple tests #5445

hichamboushaba · 2021-12-13T14:19:29Z

Description

When running multiple tests during the same sessions, Hilt will reset the state after running the test, and will create new copies for all components for subsequent tests.
This PR makes the following changes to make UI tests work across multiple tests:

since Zendesk is using a static singleton Zendesk.INSTANCE, its state is being kept across tests, which causes a crash when we try to initialize the app, this PR relaxes the check we do on ZendeskHelper when running UI tests.
We use DataStore in the app, but without providing our own CoroutineScope, it will stay active when the test is finished, this PR makes sure we pass our CoroutineScope, and we cancel it properly after finishing the test.
The login library is not migrated to Hilt yet, and instead it still uses AndroidInjector, in our Application for tests, we were caching the AndroidInjector in a field, but this meant that after a second test is launched, we'll keep track of the past graph, which meant using nonconsistent components (different instances of Stores), and failure during login.
The FCMMessageService is called sometimes on app launch if a token is received, which leads to a race condition where the injection of the service happens before Hilt tests are initialized (check this for more details), since we don't need this service for tests, I added a dummy implementation that does nothing.

Testing instructions

run ./gradlew connectedVanillaDebugAndroidTest -P android.testInstrumentationRunnerArguments.package=com.woocommerce.android.ui.main and confirm tests run successfully.

I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

When running multiple tests, Hilt will give each test its own copy of components, but since Zendesk is using a static singleton, it will be using the same instance, so it will be already initialized for all subsequent tests. With this change, we will continue gracefully on this case.

hichamboushaba · 2021-12-13T14:27:15Z

WooCommerce/src/main/kotlin/com/woocommerce/android/support/ZendeskHelper.kt

+        if (isZendeskEnabled) {
+            if (PackageUtils.isUITesting()) return
+            else error("Zendesk shouldn't be initialized more than once!")


Honestly I don't know if this error here is useful for non-testing scenarios or not, but I decided to leave it as is.

peril-woocommerce · 2021-12-13T14:27:23Z

You can test the changes on this Pull Request by downloading the APK here.

This ensures that it's cleared when our scope is cancelled, which happens in UI tests

hichamboushaba · 2021-12-13T18:13:31Z

WooCommerce/src/androidTest/kotlin/com/woocommerce/android/WooCommerceTest.kt

+        return EntryPoints.get(
+            applicationContext,
+            AndroidInjectorEntryPoint::class.java
+        ).injector()


Caching the AndroidInjector here meant having a different graph than the active one provided to its consumers when any subsequent test ran, this happened mainly in LoginWpcomService which is part of the login library, and still uses AndroidInjector.

hichamboushaba · 2021-12-13T18:14:24Z

WooCommerce/src/main/kotlin/com/woocommerce/android/di/ApplicationModule.kt

@@ -39,6 +40,7 @@ abstract class ApplicationModule {
    companion object {
        @Provides
        @AppCoroutineScope
+        @Singleton


We need to make sure we use the same instance of the coroutineScope across all components, for cancelling to work.

hichamboushaba · 2021-12-13T18:15:11Z

WooCommerce/src/main/kotlin/com/woocommerce/android/datastore/DataStoreModule.kt

        produceFile = {
            appContext.preferencesDataStoreFile("tracker")
-        }
+        },
+        scope = appCoroutineScope


Passing the scope here fixes the issue of having multiple DataStores active when a second test is launched, since the first DataStore will be closed when the scope is canceled.

❓ The default scope here CoroutineScope(Dispatchers.IO + SupervisorJob()), the new one is CoroutineScope(SupervisorJob() + dispatcher) where dispatcher is a Dispatchers.Default.

Are we sure we don't want DataStores to operate on Dispatchers.IO?

The IO dispatcher citing docs:

is designed for offloading blocking IO tasks to a shared pool of threads.

I'm afraid that DataStore designers didn't expect it to run on CoroutineDispatcher different than IO and that might lead to some unexpected behaviours. WDYT? Maybe we can change scope only for UI tests?

Nice catch, we can derive a scope that uses Dispatchers.IO from our app's CoroutineScope, done in cdaf2c7

pachlava

Thanks a lot for spending your time on this @hichamboushaba !

I tried both command line execution and running a package from Android Studio, and it worked (I used Pixel 2 API 28, since it will likely be used on CI):

I will refrain from reviewing the code itself, since I don't have enough context and knowledge to do this effectively, but your explanations and changes made sense to me. 👍

wzieba

Thanks for the PR description, helped in understanding reasons here.

All tests passed, I'm leaving one concern.

wzieba · 2021-12-14T11:45:41Z

WooCommerce/src/main/kotlin/com/woocommerce/android/datastore/DataStoreModule.kt

        produceFile = {
            appContext.preferencesDataStoreFile("tracker")
-        }
+        },
+        scope = appCoroutineScope


❓ The default scope here CoroutineScope(Dispatchers.IO + SupervisorJob()), the new one is CoroutineScope(SupervisorJob() + dispatcher) where dispatcher is a Dispatchers.Default.

Are we sure we don't want DataStores to operate on Dispatchers.IO?

The IO dispatcher citing docs:

is designed for offloading blocking IO tasks to a shared pool of threads.

I'm afraid that DataStore designers didn't expect it to run on CoroutineDispatcher different than IO and that might lead to some unexpected behaviours. WDYT? Maybe we can change scope only for UI tests?

wzieba

Thanks for addressing the comment, looks good!

hichamboushaba added 2 commits December 13, 2021 15:10

Add a function for checking if we are running UI tests

f3b2da5

hichamboushaba added the category: ui tests Related to UI testing. label Dec 13, 2021

hichamboushaba requested review from pachlava, a team and kidinov and removed request for a team December 13, 2021 14:26

hichamboushaba marked this pull request as ready for review December 13, 2021 14:26

hichamboushaba commented Dec 13, 2021

View reviewed changes

hichamboushaba removed the request for review from kidinov December 13, 2021 14:45

hichamboushaba marked this pull request as draft December 13, 2021 14:45

hichamboushaba added 5 commits December 13, 2021 19:06

Make AppCoroutineScope singleton

950ca9d

Pass the scope to the created datastore

41b6dd2

This ensures that it's cleared when our scope is cancelled, which happens in UI tests

Make sure

95e4ec3

Make sure our AppCoroutineScope is canceled after running tests

f22a41f

Avoid caching the androidInjector in tests

37f9469

hichamboushaba commented Dec 13, 2021

View reviewed changes

hichamboushaba marked this pull request as ready for review December 13, 2021 18:19

hichamboushaba requested a review from wzieba December 13, 2021 18:19

Add a dummy FCM service implementation to avoid crashing the app

2aa8f74

hichamboushaba modified the milestones: 8.2, 8.3 Dec 13, 2021

pachlava approved these changes Dec 13, 2021

View reviewed changes

pachlava mentioned this pull request Dec 13, 2021

Adds UI tests execution to CircleCI #5423

Merged

1 task

wzieba reviewed Dec 14, 2021

View reviewed changes

Use Dispatchers IO for the datastore's scope

cdaf2c7

wzieba approved these changes Dec 14, 2021

View reviewed changes

wzieba merged commit a955722 into develop Dec 14, 2021

wzieba deleted the ui-tests-hilt branch December 14, 2021 12:23

pachlava mentioned this pull request Dec 20, 2021

Selected all UI tests for CI execution #5473

Closed

1 task

hichamboushaba mentioned this pull request Dec 21, 2021

Hilt migration experiment wordpress-mobile/WordPress-Android#15653

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI Tests: fix crash when running multiple tests #5445

UI Tests: fix crash when running multiple tests #5445

hichamboushaba commented Dec 13, 2021 •

edited

Loading

hichamboushaba Dec 13, 2021

peril-woocommerce bot commented Dec 13, 2021 •

edited

Loading

hichamboushaba Dec 13, 2021

hichamboushaba Dec 13, 2021

hichamboushaba Dec 13, 2021

wzieba Dec 14, 2021

hichamboushaba Dec 14, 2021

pachlava left a comment

wzieba left a comment

wzieba Dec 14, 2021

wzieba left a comment

UI Tests: fix crash when running multiple tests #5445

UI Tests: fix crash when running multiple tests #5445

Conversation

hichamboushaba commented Dec 13, 2021 • edited Loading

Description

Testing instructions

hichamboushaba Dec 13, 2021

Choose a reason for hiding this comment

peril-woocommerce bot commented Dec 13, 2021 • edited Loading

hichamboushaba Dec 13, 2021

Choose a reason for hiding this comment

hichamboushaba Dec 13, 2021

Choose a reason for hiding this comment

hichamboushaba Dec 13, 2021

Choose a reason for hiding this comment

wzieba Dec 14, 2021

Choose a reason for hiding this comment

hichamboushaba Dec 14, 2021

Choose a reason for hiding this comment

pachlava left a comment

Choose a reason for hiding this comment

wzieba left a comment

Choose a reason for hiding this comment

wzieba Dec 14, 2021

Choose a reason for hiding this comment

wzieba left a comment

Choose a reason for hiding this comment

hichamboushaba commented Dec 13, 2021 •

edited

Loading

peril-woocommerce bot commented Dec 13, 2021 •

edited

Loading