Fix unit test timeouts and other failures.#2903
Conversation
| { | ||
| "name": "ui", | ||
| "comment": "Tests that exercise Activities/UI", | ||
| "comment": "Compose-rendering tests must NOT share a shard with classes that call mockk<...>(relaxed = true) on Activity-derived types (LoginActivity, FragmentActivity, etc.). mockk's Android dispatcher bytecode-instruments those class hierarchies for the JVM process lifetime, which slows ContextThemeWrapper.getResources() to seconds per call — hanging Compose rendering. The current polluters are LoginActivityTest, NativeLoginManagerTest, and SalesforceSDKManagerTests, all kept in the `remaining` shard.", |
There was a problem hiding this comment.
I will admit that using shards to separate tests that mock activities and compose tests feels more like a workaround than a real solution, but I think it is the best we can do today. Completely re-writing many tests to not mock LoginActivity seems like it will be an enormous amount of work and will likely reduce our overall coverage.
This leaves us with the (existing) issue that there isn't a trivial way to run all tests locally. I have a script I created to test these changes that I could add to the PR. This would let us easily run tests like CI does. Let me know how you feel about that.
There was a problem hiding this comment.
I agree with your analysis short and long term. This looks like a practical way to improve our current test inventory and document future needs as well.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #2903 +/- ##
=============================================
+ Coverage 55.17% 66.45% +11.27%
- Complexity 2509 3100 +591
=============================================
Files 226 226
Lines 17781 17774 -7
Branches 2328 2327 -1
=============================================
+ Hits 9810 11811 +2001
+ Misses 6966 4886 -2080
- Partials 1005 1077 +72
🚀 New features to boost your workflow:
|
|
All SalesforceSDK lib test pass! (which was the goal) |
JohnsonEricAtSalesforce
left a comment
There was a problem hiding this comment.
👍🏻 Great research on this one, @brandonpage ⭐️
Summary
Fix
screenLockActivity_onAuthError_sendsAccessibilityEventand several other instrumentation tests that were timing out or failing on Firebase Test Lab.Root cause
mockk's Android dispatcher uses dexmaker to bytecode-instrument any class whose instances are mocked, including the entire ancestor chain. Once a test calls
mockk<LoginActivity>(relaxed = true)(or any other Activity-derived type) in a given JVM process, every subsequentContextThemeWrapper.getResources()call in that process is routed throughio.mockk.proxy.android.advice.Advice.findMethod— even afterunmockkAll().Compose calls
getResources()thousands of times per frame. With mockk's reflection on the hot path, single Compose frames take 8+ seconds. Tests likescreenLockActivity_onAuthError_sendsAccessibilityEventthat launch a Compose Activity then ANR onAuthenticatorServicebind, while others (tokenMigrationView_*,screenLockView_*) silently slow from ~2 s to ~30 s per test until the shard hits its timeout.Confirmed via JVM main-thread stack from a timed-out run:
io.mockk.proxy.android.advice.Advice$Companion.findMethod(Advice.kt:171)
io.mockk.proxy.android.AndroidMockKDispatcher.getOrigin(...)
android.view.ContextThemeWrapper.getResources(Unknown Source:14)
androidx.core.content.ContextCompat.getColor(...)
com.salesforce.androidsdk.ui.theme.SFColors.primaryColor(SFColors.kt:72)
androidx.compose.ui.platform.ComposeView.Content(...)
The pollution is irreversible within a JVM lifetime, so the fix has to keep Activity-mocking tests and Compose-rendering tests in separate test processes (separate Firebase shards).
Changes
Test isolation
BiometricAuthenticationCallbackTest.kt(new) — extracted threemockk<ScreenLockActivity>(relaxed = true)tests out ofScreenLockActivityScenarioTest. They tested an inner class via the outer Activity mock and were poisoning the rest of the class once they ran.ScreenLockActivityScenarioTest.kt— removed those three tests and unused imports. The remaining 29 Compose-driven cases now run unaffected..github/test-shards/SalesforceSDK.json:ScreenLockActivityScenarioTest,ScreenLockViewTest,TokenMigrationViewActivityTest) added to theuishard.TokenMigrationWebViewTestmoved to a newwebviewshard (slow on the emulator for unrelated WebView reasons; isolating it keeps it from eating budget elsewhere).LoginActivityTest,NativeLoginManagerTest,SalesforceSDKManagerTests,BiometricAuthenticationCallbackTest) stay inremaining.Test fixes
TokenMigrationWebViewTest.kt— deletedshouldOverrideUrlLoading_returnsTrueForCallbackUrl_userAgentFlow. PR Fix Token Migration silent failure. #2892 collapsed theuseWebServerFlowbranch in the production code; the test was verifying a path that no longer exists.AuthConfigUtilTest.java— broadcast tests (testBroadcastSucceeds,testBroadcastFails) now register theirBroadcastReceiveron aHandlerThreadinstead of the implicit main thread. Other tests in the same shard launch Activities, and the cumulative main-looper backlog could prevent the receiver'sonReceivefrom running before the test timeout, even though the broadcast had already been dispatched. Also widened the JUnit@Test(timeout=...)from 5 s to 30 s and added an explicitFuture.get(20, SECONDS)for a clearer failure mode.Test plan
screenLockActivity_onAuthError_sendsAccessibilityEventpassesScreenLockActivityScenarioTestcases pass (~2 s each)ScreenLockViewTestcases pass (~2.5 s each, down from 27 s)BiometricAuthenticationCallbackTestcases passtestBroadcastSucceeds/testBroadcastFailspass (~2.5 s each)CLASSES == NOT_CLASSES) passesCouple of usage notes for the PR description: