From 6b8765560f047b98bc661b6e2681bafcf7f823f1 Mon Sep 17 00:00:00 2001 From: vishk23 <119831996+vishk23@users.noreply.github.com> Date: Sun, 26 Jul 2026 23:09:47 -0400 Subject: [PATCH] fix(android): make the sync chip's state decision resolvable without an Application MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `SyncChipStateTest.lastSyncedAt_takesPriorityOverHistorySyncExperimental` fails on main — the only failure in the suite (3060 tests, 1 failure): java.lang.IllegalStateException: NoopApplication is not attached at com.noop.NoopApplication$Companion.localizedString(NoopApplication.kt:134) at com.noop.ui.UiStringsKt.uiString(UiStrings.kt:16) at com.noop.ui.TodayScoringKt.shortSyncAgo(TodayScoring.kt:412) at com.noop.ui.SyncChipState$Companion.resolve(TodayScoring.kt:399) `SyncChipState.resolve` is documented "Pure + unit-tested" but reached for two ambient dependencies: the process-wide Application's resources (for the translated "now" word) and the system clock. These are plain JVM tests with no Robolectric, so no Application is ever attached and the resource lookup throws. Only the `< 60s` branch of `shortSyncAgo` needs a word — every other bucket is digits plus a unit letter — which is why five of the six cases passed and the sixth, the one with a 5-second-old sync, did not. Same reason `lastSyncedAt_isSyncedWith- AgeText` survives at 65 seconds. Hoist both dependencies to parameters. `SyncStatusChip` — a composable that already depends on the clock and the string catalog — resolves them and hands them down, so the decision itself becomes a genuinely pure function. Same injected-clock style as `recordingStateFor` directly above it, which already takes its own `nowSec`. No behaviour change: the clock is still read at composition time, exactly as `shortSyncAgo` did on every recomposition, and the chip renders identical text. Tests are now deterministic (fixed `nowSec` rather than `System.currentTimeMillis()`) and gain a case for the sub-minute branch, which was previously impossible to reach from a unit test — it is the regression guard against the lookup moving back inside. The Swift twin is deliberately unchanged. Its `shortAgo` resolves its own clock and `String(localized:)` and is fine doing so: XCTest runs against a real bundle, and `SyncChipStateTests` passes. The two signatures now differ on purpose; the priority order they encode does not. Noted in the KDoc so it does not get "fixed" back. Verified locally: 3061 tests, 0 failures (was 3060 / 1), 381 test classes both before and after. i18n-coverage and source-hygiene gates both green. android.yml is disabled by default, which is why CI never caught this. --- .../src/main/java/com/noop/ui/TodayScoring.kt | 29 +++++++++--- .../src/main/java/com/noop/ui/TodayScreen.kt | 14 +++++- .../java/com/noop/ui/SyncChipStateTest.kt | 45 ++++++++++++++++--- 3 files changed, 74 insertions(+), 14 deletions(-) diff --git a/android/app/src/main/java/com/noop/ui/TodayScoring.kt b/android/app/src/main/java/com/noop/ui/TodayScoring.kt index afd0a85da6..b638e07bf5 100644 --- a/android/app/src/main/java/com/noop/ui/TodayScoring.kt +++ b/android/app/src/main/java/com/noop/ui/TodayScoring.kt @@ -6,7 +6,6 @@ import com.noop.analytics.ChargeDriver import com.noop.analytics.RecoveryDrivers import com.noop.analytics.RestScorer import com.noop.analytics.ScoreConfidence -import com.noop.R import com.noop.data.DailyMetric import java.time.LocalDate import java.time.Instant @@ -388,15 +387,29 @@ sealed class SyncChipState { companion object { /** Pure + unit-tested. Mirrors Swift `SyncChipState.resolve` exactly: backfilling wins over a - * known last-sync, which wins over the 5/MG experimental fallback. */ + * known last-sync, which wins over the 5/MG experimental fallback. + * + * [nowSec] (unix seconds) and [nowLabel] (the already-translated "now" word) are PARAMETERS + * rather than things this function reaches for, which is what keeps "pure" true. Resolving the + * word in here instead cost the twin its own test: it goes through `NoopApplication`, which + * throws `IllegalStateException: NoopApplication is not attached` under a plain JVM unit test — + * these run without Robolectric, so no Application is ever attached. Only the `< 60s` branch of + * [shortSyncAgo] wants a word, so the failure was invisible until a case landed in it. Same + * injected-clock style as [recordingStateFor] just above. + * + * Swift's twin keeps both inside its own `shortAgo` and is fine there — XCTest runs against a + * real bundle, so `String(localized:)` resolves. The two SIGNATURES therefore differ on purpose; + * the decision they encode does not. Don't "restore parity" by moving the lookup back in here. */ fun resolve( backfilling: Boolean, chunks: Int, lastSyncAtSec: Long?, historySyncExperimental: Boolean, + nowSec: Long, + nowLabel: String, ): SyncChipState = when { backfilling -> Syncing(chunks) - lastSyncAtSec != null -> Synced(shortSyncAgo(lastSyncAtSec)) + lastSyncAtSec != null -> Synced(shortSyncAgo(lastSyncAtSec, nowSec, nowLabel)) historySyncExperimental -> ExperimentalLive else -> Hidden } @@ -405,11 +418,13 @@ sealed class SyncChipState { /** Compact relative age for the header chip ("now" / "Nm" / "Nh" / "Nd") from a unix-SECONDS timestamp — * deliberately terse. "now" is the only word in here (the rest is digits + a unit letter), so it's the - * only piece that needs a catalog entry to translate. Mirrors the iOS `SyncChipState.shortAgo`. */ -internal fun shortSyncAgo(unixSec: Long): String { - val secs = (System.currentTimeMillis() / 1000L - unixSec).coerceAtLeast(0) + * only piece that needs a catalog entry to translate; it arrives as [nowLabel], resolved by the + * composable that owns the chip, so the bucketing stays framework-free. [nowSec] is unix seconds, + * injected for the same reason. Mirrors the iOS `SyncChipState.shortAgo`. */ +internal fun shortSyncAgo(unixSec: Long, nowSec: Long, nowLabel: String): String { + val secs = (nowSec - unixSec).coerceAtLeast(0) return when { - secs < 60 -> uiString(R.string.l10n_today_screen_sync_chip_now_c9bc849a) + secs < 60 -> nowLabel secs < 3600 -> "${secs / 60}m" secs < 86_400 -> "${secs / 3600}h" else -> "${secs / 86_400}d" diff --git a/android/app/src/main/java/com/noop/ui/TodayScreen.kt b/android/app/src/main/java/com/noop/ui/TodayScreen.kt index 9e14e2c345..deb0490d58 100644 --- a/android/app/src/main/java/com/noop/ui/TodayScreen.kt +++ b/android/app/src/main/java/com/noop/ui/TodayScreen.kt @@ -2132,7 +2132,19 @@ private fun SyncStatusChip( lastSyncAt: Long?, historySyncExperimental: Boolean, ) { - when (val state = SyncChipState.resolve(backfilling, chunks, lastSyncAt, historySyncExperimental)) { + // The clock and the translated "now" word are resolved HERE, in the composable that already depends + // on both, and handed down — so `SyncChipState.resolve` stays a genuinely pure decision that a plain + // JVM unit test can call with no attached Application. Reading the clock at composition time (rather + // than snapshotting it) is unchanged behaviour: `shortSyncAgo` did exactly this on every recomposition. + val state = SyncChipState.resolve( + backfilling = backfilling, + chunks = chunks, + lastSyncAtSec = lastSyncAt, + historySyncExperimental = historySyncExperimental, + nowSec = System.currentTimeMillis() / 1000L, + nowLabel = uiString(R.string.l10n_today_screen_sync_chip_now_c9bc849a), + ) + when (state) { is SyncChipState.Syncing -> ChipCapsule( Icons.Filled.Autorenew, "${state.chunks}", Palette.accent, uiString(R.string.l10n_today_screen_sync_chip_syncing_desc_bfc290e7, state.chunks)) diff --git a/android/app/src/test/java/com/noop/ui/SyncChipStateTest.kt b/android/app/src/test/java/com/noop/ui/SyncChipStateTest.kt index aef8fd9622..b623951bfc 100644 --- a/android/app/src/test/java/com/noop/ui/SyncChipStateTest.kt +++ b/android/app/src/test/java/com/noop/ui/SyncChipStateTest.kt @@ -8,22 +8,37 @@ import org.junit.Test * #245: `SyncChipState.resolve` is the one place the Today top bar's `SyncStatusChip` decides which of * the four sync states to show. Mirrors the iOS `SyncChipStateTests` 1:1 — same priority order (backfilling * wins over a known last-sync, which wins over the 5/MG experimental fallback), same cold-start `Hidden` case. + * + * Every case pins [NOW] instead of reading the system clock, and passes the "now" word in as [NOW_LABEL] + * instead of letting `resolve` resolve it: these are plain JVM tests with no Robolectric, so there is no + * attached `NoopApplication` to read the string catalog from. That is what made the `< 60s` cases below + * throw `IllegalStateException: NoopApplication is not attached` while every other case passed — only the + * sub-minute branch of `shortSyncAgo` needs a translated word. */ class SyncChipStateTest { + private companion object { + /** Any fixed instant works now that `resolve` takes its clock as an argument. */ + const val NOW = 1_700_000_000L + + /** Stand-in for the `l10n_today_screen_sync_chip_now_*` catalog entry the composable passes in. */ + const val NOW_LABEL = "now" + } + @Test fun backfilling_isSyncingWithChunkCount() { val state = SyncChipState.resolve( backfilling = true, chunks = 7, lastSyncAtSec = null, historySyncExperimental = false, + nowSec = NOW, nowLabel = NOW_LABEL, ) assertEquals(SyncChipState.Syncing(7), state) } @Test fun lastSyncedAt_isSyncedWithAgeText() { - val now = System.currentTimeMillis() / 1000L val state = SyncChipState.resolve( - backfilling = false, chunks = 0, lastSyncAtSec = now - 65, historySyncExperimental = false, + backfilling = false, chunks = 0, lastSyncAtSec = NOW - 65, historySyncExperimental = false, + nowSec = NOW, nowLabel = NOW_LABEL, ) assertEquals(SyncChipState.Synced("1m"), state) } @@ -32,6 +47,7 @@ class SyncChipStateTest { fun historySyncExperimental_withNoLastSync_isExperimentalLive() { val state = SyncChipState.resolve( backfilling = false, chunks = 0, lastSyncAtSec = null, historySyncExperimental = true, + nowSec = NOW, nowLabel = NOW_LABEL, ) assertEquals(SyncChipState.ExperimentalLive, state) } @@ -40,25 +56,42 @@ class SyncChipStateTest { fun coldStart_noBackfillNoSyncNoExperimental_isHidden() { val state = SyncChipState.resolve( backfilling = false, chunks = 0, lastSyncAtSec = null, historySyncExperimental = false, + nowSec = NOW, nowLabel = NOW_LABEL, ) assertEquals(SyncChipState.Hidden, state) } @Test fun backfilling_takesPriorityOverLastSyncedAt() { - val now = System.currentTimeMillis() / 1000L val state = SyncChipState.resolve( - backfilling = true, chunks = 2, lastSyncAtSec = now - 5, historySyncExperimental = false, + backfilling = true, chunks = 2, lastSyncAtSec = NOW - 5, historySyncExperimental = false, + nowSec = NOW, nowLabel = NOW_LABEL, ) assertEquals(SyncChipState.Syncing(2), state) } @Test fun lastSyncedAt_takesPriorityOverHistorySyncExperimental() { - val now = System.currentTimeMillis() / 1000L val state = SyncChipState.resolve( - backfilling = false, chunks = 0, lastSyncAtSec = now - 5, historySyncExperimental = true, + backfilling = false, chunks = 0, lastSyncAtSec = NOW - 5, historySyncExperimental = true, + nowSec = NOW, nowLabel = NOW_LABEL, ) assertTrue("A known last-sync should win over the experimental fallback", state is SyncChipState.Synced) } + + /** + * Regression guard for the branch that used to be unreachable from a unit test: a sub-minute sync must + * render the caller-supplied word verbatim. This fails to even compile — let alone pass — if the string + * lookup ever moves back inside `shortSyncAgo`, which is the whole point of keeping it out. No iOS twin: + * Swift's `shortAgo` resolves its own clock and `String(localized:)`, and can afford to because XCTest + * runs against a real bundle. + */ + @Test + fun lastSyncedUnderAMinute_isSyncedWithTheInjectedNowLabel() { + val state = SyncChipState.resolve( + backfilling = false, chunks = 0, lastSyncAtSec = NOW - 5, historySyncExperimental = false, + nowSec = NOW, nowLabel = NOW_LABEL, + ) + assertEquals(SyncChipState.Synced("now"), state) + } }