test(compose): prepare tests for re-landing CMP 1.12 (fixes 8 of 10 regressions) - #6666
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR adds ChangesAsynchronous test settling
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: 🔵 Low · up to This PR changes test synchronization for asynchronous resource loading and does not modify production code. A bounded concern remains because the new suspend test helper may swallow cancellation, so the change is mergeable with explicit owner follow-up to preserve cancellation semantics. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@feature/connections/src/commonTest/kotlin/org/meshtastic/feature/connections/ScannerViewModelTest.kt`:
- Around line 754-761: Update warmScanFailureStrings() to use
org.meshtastic.core.common.util.safeCatching instead of runCatching, preserving
the existing resource-warming operations while allowing CancellationException to
propagate.
Apply the same fix in
`@feature/connections/src/commonTest/kotlin/org/meshtastic/feature/connections/ScannerViewModelTest.kt`
around lines 743 - 749.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d570b1bb-c457-46d9-bb5c-bd3d93985e5d
📒 Files selected for processing (5)
core/service/src/androidHostTest/kotlin/org/meshtastic/core/service/MeshNotificationManagerImplConversationTest.ktcore/service/src/androidHostTest/kotlin/org/meshtastic/core/service/MeshNotificationManagerImplTest.ktcore/testing/src/commonMain/kotlin/org/meshtastic/core/testing/TestScopes.ktfeature/connections/src/androidHostTest/kotlin/org/meshtastic/feature/connections/AndroidScannerViewModelBondingTest.ktfeature/connections/src/commonTest/kotlin/org/meshtastic/feature/connections/ScannerViewModelTest.kt
This comment has been minimized.
This comment has been minimized.
❌ 1 Tests Failed:
View the full list of 1 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
Collaborator
Author
CI red is the known compass flake, not this PR
Parsing the The four classes this PR fixes, as executed in that same CI run, against their source
Counts match exactly with no repeats, so all 8 previously-failing tests passed on first attempt — none was skipped, and none passed only after a retry. That matters because this repo retries tests and caches a task that passes after a retry, which can hide a flip; |
…bump CMP 1.12.0-rc01 changed `AsyncCache.getOrLoad` in compose-resources to run each load on a private `Dispatchers.Default` scope instead of the caller's, so every string-resource read is now genuinely asynchronous. Tests that read state on the next line, or that pump only `advanceUntilIdle()`, observed it before it landed. Adds `TestScope.runUntilSettled` for tests that just need the value to arrive, and pre-loads the scan-failure strings in ScannerViewModelTest, which asserts on exact virtual-time BLE cooldowns and so cannot wait in real time. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
runCatching swallows CancellationException. Use safeCatchingAll, which the ViewModel already wraps these same lookups in: it re-throws CancellationException while still absorbing both the unmocked-Resources exception under androidHostTest and the Error skiko's initializer can raise on the JVM test classpath. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jamesarich
changed the title
jamesarich
force-pushed
the
claude/loving-lalande-bf538c
branch
from
August 13, 2026 02:06
5bb4b83 to
305a04a
Compare
This was referenced Aug 13, 2026
Merged
jamesarich
added a commit
that referenced
this pull request
Aug 13, 2026
Reverts d8361cc, restoring #6662's bump now that every regression it caused has a fix on main: - #6666 settles asynchronous resource loads in the 8 tests that failed deterministically (:core:service, :feature:connections) - #6669 keeps ViewModel coroutines inside the test that started them, covering ConnectionsViewModelTest and RadioConfigViewModelTest - #6668 removes the runBlocking getString shim from every Dispatchers.Default-reachable notification path, which is what made the bump a production hazard rather than only a test one Also drops the Renovate rule that blocked 1.12.0-rc01, since its lift condition is now met. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
#6664 reverted CMP 1.12.0-rc01 because it broke 10 tests. This PR fixes 8 of them, so that work does not have to be redone when the bump is re-landed.
This has no effect on current
main, which is on CMP 1.11.1. The changes are test-only and correct under both 1.11.1 and 1.12.x. Their value is as re-land groundwork.The cause is a behaviour change in
compose-resources, not a bug in our code and not a wrong resource lookup. Between 1.11.1 and 1.12.0-rc01,AsyncCache.getOrLoadmoved the load out of the caller's scope:Every string-resource read becomes genuinely asynchronous. Lookup is still correct — values come back
null, never wrong. Tests running onUnconfinedTestDispatcherthat read.valueon the next line, and tests that pump onlyadvanceUntilIdle(), observe the state before it lands.This also explains why only some tests in each class failed: the cache is process-wide, so whichever test touches a given string first pays the async cost and the rest hit it warm.
🛠️ Changes
Tests only. No production change.
core/testinggainsTestScope.runUntilSettled { }— pumpsrunCurrent()and waits in real time between passes. Used by the notification and bonding tests. On 1.11.1 the predicate is already satisfied on the first pass, so it returns immediately.ScannerViewModelTestinstead pre-loads its five scan-failure strings. It asserts on exact virtual-time BLE retry cooldowns and therefore cannot wait in real time: any real-time suspension insiderunTestcauses the virtual clock to advance to the next scheduled task, firing the 30s cooldown early. That constraint is documented in the helper's KDoc so the next person doesn't rediscover it.safeCatchingAll(the same wrapperScannerViewModeluses around these lookups), which absorbs both the unmocked-Resourcesexception and theErrorskiko's initializer can raise on the JVM test classpath, while still re-throwingCancellationException.ScannerViewModelTestis incommonTest, so it also runs undertestAndroidHostTest, where the stubs leaveResources.getSystem()unmocked; resources never resolve there and the ViewModel's untranslated fallback — byte-identical text, produced synchronously — is what the assertions match. That is why CI only ever showed[jvm]failures for this class.Testing Performed
:feature:connections:testAndroidHostTest(50 tests),--rerun-tasks:core:service:testAndroidHostTest(11 tests),--rerun-tasks:feature:connections:jvmTest --tests "*ScannerViewModelTest*"spotlessApply spotlessCheck detekt assembleDebug test allTestsreports-shard-featureartifact--rerun-tasks)entries == distinct, no hangsRetry verification, from the JUnit XML rather than from log markers — this repo retries tests and caches a task that passes after a retry, so a test that flipped would otherwise be invisible. From CI run 31657204256 (rc01):
@TestScannerViewModelTestAndroidScannerViewModelBondingTestMeshNotificationManagerImplTestMeshNotificationManagerImplConversationTestCounts match exactly with no repeats, so all 8 previously-failing tests passed on first attempt — none skipped, none passing only after a retry.
On 1.11.1 the per-test times are 0.002–0.134s, confirming
runUntilSettledreturns on its first pass rather than polling to its 10s timeout when the resource has already resolved inline. Passing alone would not have distinguished those two cases.The per-method isolation runs were done specifically to rule out tests passing only because an earlier test in the same JVM warmed the process-wide cache — the order-dependence this root cause predicts. All nine pass alone on a cold cache.
Re-land prerequisites — this PR is not sufficient on its own
Re-landing CMP 1.12 needs all of the following. This PR is only the first:
These test fixes (this PR) — covers 8 of the 10 failures.
core/uiConnectionsViewModelTestandfeature/settingsRadioConfigViewModelTest— the other 2. SameAsyncCachecause:ConnectionsViewModeldispatches its firmware notification viagetStringSuspend, includingtitle = getStringSuspend(Res.string.firmware_update_available), and the test asserts the dispatch happened afteradvanceUntilIdle(). They pass on 1.11.1 today only because the revert stepped off rc01; they will fail again the moment the bump returns.The blocking
runBlockingresource lookup atcore/resources/.../GetString.kt:26— 29 call sites across 6 files. This is the production-side face of the same finding, and on 1.12 it is a permanent, unrecoverable hang, not a slow call:ServiceScopeisCoroutineScope(dispatchers.default + SupervisorJob())(core/service/.../di/CoreServiceModule.kt:33), so the packet/notification pipeline runs onDispatchers.Default. On 1.12getString()blocks its calling thread while the load it awaits is dispatched toDispatchers.Default— so a blocking caller on a Default worker parks that worker awaiting work that needs a Default worker. Once enough callers do this concurrently, the pool is starved: every subsequentgetStringin the process hangs forever, main thread included, with no recovery short of a restart.jstackshows Default workers parked onBlockingCoroutine. A message burst is the trigger.The measured threshold was 16 concurrent blocking callers, but that number is the
Dispatchers.Defaultparallelism of the machine it was measured on (= CPU count). On an 8-core phone it wedges at 8, so real devices are easier to wedge than the dev machine, not harder.Separately, the first cold
getStringcosts ~197.7ms on the main thread, paid byinitChannels()duringMeshService.onCreate.Benign on 1.11.1 only because resolution was inline. A mitigation migrating Default-reachable sites to
getStringSuspendis staged onclaude/sad-gagarin-2f1ff1. CMP 1.12 must not be re-landed before this is resolved.Removing the Renovate rule revert(deps): back out CMP 1.12.0-rc01 until its test regressions are fixed #6664 added to
.github/renovate.jsonblocking1.12.0-rc01, which is now onmain.Out of scope
NodeDetailCompassLifecycleTest— genuinely not a CMP regression. Codecov reports a 66.67% failure rate onmainand it reproduces on 1.11.1. An earlier revision of this branch raised itswaitUntiltimeout; that was reverted once the flake data showed the timeout was treating a pre-existing flake as CMP fallout. Owned separately.