revert(deps): back out CMP 1.12.0-rc01 until its test regressions are fixed - #6664
Conversation
… fixed Reverts #6662 (bc4e9da). The bump broke 18 tests across :core:service, :feature:connections and :feature:node: compose-resources lookups return wrong/null values under test, and compose.uiTest idling times out. It reached main because its own test shards were cancelled rather than run, and the next PR's green shard replayed all six affected test tasks from the build cache, so nothing caught it until main was already red. Renovate is blocked from re-proposing 1.12.0-rc01, following the Gradle 9.7.0 precedent, so the revert holds until the regressions are addressed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesCompose version alignment
Estimated code review effort: 1 (Trivial) | ~5 minutes Mergeability Score: 🔵 Low · up to This change restores the prior Compose dependency set and prevents the known regression from being reintroduced automatically. It is mergeable with owner awareness, but the required release and full verification checks should be run to confirm build and packaging compatibility. Possibly related PRs
Suggested labels: 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 |
❌ 1 Tests Failed:
View the full list of 1 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
This comment has been minimized.
This comment has been minimized.
Why shard-feature is red here, and why no fix is pushed for itThe
Codecov reports a 66.67% failure rate on Correction to this PR's own description: I originally counted this test among #6662's regressions. That was wrong. The CMP regression count is 17, not 18, and I have updated the description accordingly. A single local pass I got on this branch was luck at that flake rate, not verification — my mistake, and worth stating plainly since I used it as evidence. What this PR does fix, confirmed by its own CI: shard-feature on this branch now fails only that one flaky test. All 17 CMP-caused failures are gone — Deliberately not pushing a fix for the flake here. It belongs to a separate, already-scoped effort covering the order-dependent/flaky family on Note for whoever merges: because the blocking test is flaky rather than deterministic, re-running the job may well turn it green. If this lands that way, it merged on a retry that flipped a known flake — not on a clean signal. Worth recording as such rather than as "green". 🤖 Addressed by Claude Code |
… rule The rule described the symptom as wrong/null resource lookups. The real cause is compose-resources AsyncCache.getOrLoad moving off the caller's dispatcher onto a private CoroutineScope(SupervisorJob()), so every cold resource read becomes a real cross-thread hop that advanceUntilIdle() cannot drain — which also explains why affected classes pass or fail depending on which test warms the process-wide cache first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Correction: two classes I called pre-existing are actually CMP casualtiesSuperseding my previous comment on this point, and the earlier revision of the description. I had reported This branch's own CI settles it. They also fit the root cause exactly: because Net effect on this PR: it fixes more than I claimed, not less. The description now reflects that, and the Renovate rule records the mechanism rather than my original mis-description of it.
🤖 Addressed by Claude Code |
An earlier count of 17/18 came from a regex over <testcase>...</testcase>, which mis-associates failures across self-closing <testcase/> elements. Parsed properly the bc4e9da run has 8 distinct CMP-caused failing test methods (22 entries, inflated by Develocity's two retries per test), plus one unrelated pre-existing flake. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Correction: the regression is 8 distinct test methods, not 17 or 18My failure counts in earlier revisions of this description were wrong, and so were several of the test names. Correcting publicly because those numbers and names were handed to other work. Cause of my error: I extracted failures with a regex over Correct figures for the
So 8 CMP-caused distinct methods. The description now lists them individually. Names I published that never failed: This sharpens the diagnosis rather than weakening it. All four real None of this changes what the revert does: 🤖 Addressed by Claude Code |
1 participant
Why
mainis red. CMP 1.12.0-rc01 (#6662) breaks 8 tests deterministically in:core:serviceand:feature:connections, and destabilises:core:uiand:feature:settingsintermittently. It is a release candidate we do not need to be on. Reverting restores a green baseline now; adapting to the new CMP behaviour can happen on its own schedule instead of blocking every other PR.It reached
mainthrough two independent gate failures, which is why nobody caught it:cancelled, not passed. Its PR head39fa5379shows all threetest-shardschecks ascancelled. The post-merge run atbc4e9da3aexecuted them for the first time and they failed.94269148979) reported success while serving every affected test taskFROM-CACHE—:core:service:jvmTest,:core:service:testAndroidHostTest,:feature:connections:jvmTest,:feature:connections:testAndroidHostTest,:feature:node:jvmTest,:feature:node:testAndroidHostTest. It executed none of them.Both gate holes are tracked separately; this PR only restores green.
Root cause
Not "wrong resource values" — the values are null, because resource reads stopped being synchronous.
compose-resourcesAsyncCache.getOrLoad:Every cold resource load now runs on a real
Dispatchers.Defaultthread while the caller awaits it.advanceUntilIdle()cannot drain a real-thread deferred, so:null;viewModelScopecoroutine still awaiting a load whenDispatchers.resetMain()runs surfaces asModule with the Main dispatcher is missing, blamed on the next test to start;This change is not in the CMP changelog.
Bisect
28dd35ef9(pre-bump)FROM-CACHEbc4e9da3a(#6662)d5848ad5e(#6661)FROM-CACHEReal green, real red, then silence.
What was failing
Distinct failing test methods in the
bc4e9da3arun. Parse the JUnit XML with an XML parser and count<testcase>elements that have a<failure>/<error>child — raw entry counts are ~3x higher because Develocity retries each test twice.:feature:connectionsScannerViewModelTest[jvm]bluetooth-disabled failure allows an immediate retry once the user re-enables itlocation-services-disabled failure allows an immediate retryscan quota failure honors retry-after cooldownscan startup failure clears scanning state disables auto-scan and surfaces error:core:serviceMeshNotificationManagerImplTestservice state rendering is deferred from the callerservice state seeds local stats before the local node row is available:core:serviceMeshNotificationManagerImplConversationTestnotification ids are namespaced per type so a node num cannot clobber the service notification:feature:connectionsAndroidScannerViewModelBondingTestsecurity exception does not arm the transport and surfaces an error8 distinct methods, 22 entries. Every one is a test that reads a string resource and asserts on it — consistent with the
AsyncCachechange and nothing else.Two further classes are broken by the same change, bringing the total to 10 distinct CMP-caused failures:
:core:uiConnectionsViewModelTestand:feature:settingsRadioConfigViewModelTest.They look intermittent in shard runs, but that is a shard artefact, not a property of the tests — the
AsyncCacheis process-wide, so whether a given test finds it warm depends on what ran before it. Under a class-filtered cold run the behaviour is fully deterministic.d5848ad5ealready pins rc01 andgit diff --stat d5848ad5e 0eb95531b -- core/ui/src feature/settings/srcis empty, so CMP is the only variable::core:ui:jvmTest --tests "*ConnectionsViewModelTest*" --rerun-tasksd5848ad5e)The failing method is
connected older known node exposes Android firmware update notice—expected: <1> but was: <0>atConnectionsViewModelTest.kt:201, which asserts a notification was dispatched.ConnectionsViewModelresolves that notification's title viagetStringSuspend(Res.string.firmware_update_available)(ConnectionsViewModel.kt:299), so the dispatch never lands before the assertion. Same defect as the 8 above.RadioConfigViewModelTestis theresetMainvariant: aviewModelScopecoroutine still awaiting a resource load whenDispatchers.resetMain()runs. It genuinely executed and passed on this branch (:feature:settings:jvmTest, five invocations, noFROM-CACHE).Relationship to #6666
#6666 ("test(compose): settle asynchronous resource loads after the CMP 1.12 bump") fixes 8 of the 10 by making the affected tests robust to asynchronous resource resolution. It does not touch
core/uiorfeature/settings, so it does not greenmainon its own while the bump is in place.Agreed plan is to land this revert first — it fixes all 10 in one diff and unblocks #6658 today, depending on no unpushed work — and then land #6666 on top. #6666's changes are test-only and correct under both 1.11.1 and 1.12.x, so they are not wasted: they become the groundwork that makes a future re-land safe. Whoever re-lands the bump removes the Renovate rule added here, and should also carry a fix for the two classes above, which #6666 does not cover.
🛠️ Changes
bc4e9da3a:compose-multiplatform1.12.0-rc01 → 1.11.1,compose-multiplatform-material31.12.0-alpha03 → 1.11.0-alpha07,androidx-compose-bom-aligned1.12.0 → 1.11.4..github/renovate.json— without it the next Renovate pass walks the bump straight back in. The rule records theAsyncCachemechanism and the lift condition.Testing Performed
CI on this branch is the primary evidence:
:core:ui:allTests, soConnectionsViewModelTestpasses.NodeDetailCompassLifecycleTest(see below) — all 8 CMP-caused failures are gone, including:feature:settings'RadioConfigViewModelTest.Local, with
--rerun-tasksso nothing came from the build cache::feature:connections:jvmTest --tests "*ScannerViewModelTest*":core:service:testAndroidHostTest --tests "*MeshNotificationManagerImpl*"Not covered here
:feature:nodeNodeDetailCompassLifecycleTestis a pre-existing flake and it fails on this PR too. Codecov reports a 66.67% failure rate onmain(passed 2, failed 4) forcompassSelectionFollowsScreenLifecycleAndDismissal, withComposeTimeoutException: Condition still not satisfied after 1000 msatNodeDetailCompassLifecycleTest.kt:140. It fails on 1.11.1 and 1.12.0-rc01 alike, so it is independent of the version reverted here, and it is owned by a separate effort. No fix is pushed for it here: folding an unrelated flake fix into a one-line dependency revert would make the revert harder to reason about or roll back.Because that test is flaky rather than deterministic, re-running the job may turn it green. If this merges that way it merged on a retry that flipped a known flake, not on a clean signal — worth recording as such.
Corrections to earlier revisions of this description
Stated openly since earlier versions were used as evidence:
NodeDetailCompassLifecycleTestamong chore(deps): bump the Compose stack to CMP 1.12.0-rc01 in lockstep #6662's regressions. Wrong — it is the pre-existing flake above. A single local pass I got on this branch was luck at a 66.67% failure rate, not verification.<testcase ...>(.*?)</testcase>, which mis-associates failures across self-closing<testcase/>elements and so credited passing tests with a later test's<failure>. Names I published that never failed includeinitChannels removes legacy categories and creates canonical channels,startBleScan succeeds while Connecting,bleDevicesForUi shows bonded devices only once they are visible via scan,startBleScan cancels active network scan,startBleAutoScan skips when device already selectedandselecting BLE stops active network scan. Corrected withxml.etree; a peer session caught it.ConnectionsViewModelTestandRadioConfigViewModelTestas pre-existing failures unrelated to chore(deps): bump the Compose stack to CMP 1.12.0-rc01 in lockstep #6662, on the basis of runs against a "clean checkout" ofd5848ad5e. That baseline was invalid:bc4e9da3ais an ancestor ofd5848ad5e, so that checkout still contained CMP 1.12.0-rc01. Both classes are in fact casualties of theAsyncCachechange and both pass on this branch.🤖 Generated with Claude Code