UI Tests: fix crash when running multiple tests#5445
Conversation
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.
| if (isZendeskEnabled) { | ||
| if (PackageUtils.isUITesting()) return | ||
| else error("Zendesk shouldn't be initialized more than once!") |
There was a problem hiding this comment.
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.
|
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
| return EntryPoints.get( | ||
| applicationContext, | ||
| AndroidInjectorEntryPoint::class.java | ||
| ).injector() |
There was a problem hiding this comment.
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.
| companion object { | ||
| @Provides | ||
| @AppCoroutineScope | ||
| @Singleton |
There was a problem hiding this comment.
We need to make sure we use the same instance of the coroutineScope across all components, for cancelling to work.
| appContext.preferencesDataStoreFile("tracker") | ||
| } | ||
| }, | ||
| scope = appCoroutineScope |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
❓ 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?
There was a problem hiding this comment.
Nice catch, we can derive a scope that uses Dispatchers.IO from our app's CoroutineScope, done in cdaf2c7
pachlava
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
Thanks for the PR description, helped in understanding reasons here.
All tests passed, I'm leaving one concern.
| appContext.preferencesDataStoreFile("tracker") | ||
| } | ||
| }, | ||
| scope = appCoroutineScope |
There was a problem hiding this comment.
❓ 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
left a comment
There was a problem hiding this comment.
Thanks for addressing the comment, looks good!
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:
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 onZendeskHelperwhen running UI tests.Testing instructions
run
./gradlew connectedVanillaDebugAndroidTest -P android.testInstrumentationRunnerArguments.package=com.woocommerce.android.ui.mainand confirm tests run successfully.RELEASE-NOTES.txtif necessary.