diff --git a/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverBackend.kt b/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverBackend.kt index 94ee73b..634d0bf 100644 --- a/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverBackend.kt +++ b/sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverBackend.kt @@ -63,35 +63,97 @@ internal const val STABILITY_POLL_CAP_MILLIS = 2000L // missed. internal const val MIN_STABLE_STREAK_MILLIS = 750L +// TRANSITION_* bound the wait a snapshot pays when the tree it read shows two +// routes at once, which on Compose means a NavHost cross-fade is in flight. +// They are shorter than the constants above because this wait has one job, +// outlasting the fade, where the poll above must also catch an async effect +// that fires late. +// +// The streak matches the iOS companion's 300ms for the same reason: what this +// wait is for is the cross-fade, and the cross-fade is caught by the +// route-screen check rather than by streak length, so the streak only has to +// be long enough that the reads spanning it are not all inside one frame. +// MIN_STABLE_STREAK's 750ms is sized for a poll that must also catch an async +// effect firing late, and at ~160ms per read-and-interval it costs three or +// four more reads than this does. +internal const val TRANSITION_STABLE_STREAK_MILLIS = 300L + +// The interval is the iOS companion's rather than STABILITY_POLL_INTERVAL's +// 250ms: the wide interval exists to stop a per-step poll hammering +// UiAutomation, and this one runs only on the frames that show two routes, +// about a quarter of steps on folio. The read itself paces the loop. +internal const val TRANSITION_POLL_INTERVAL_MILLIS = 100L + +// The cap is the iOS companion's 1500ms, and it has to be at least this: the +// NavHost cross-fade is a 700ms tween (Compose navigation's default enter and +// exit), it starts when the action lands rather than when the snapshot begins, +// and the streak above has to fit after it. Measured on the emulator, a 600ms +// cap left about a third of fades unfinished. A layout that holds two routes at +// rest costs the full cap once per step and no more. +internal const val TRANSITION_POLL_CAP_MILLIS = 1500L + +// awaitSettledTree reads the hierarchy and, while the tree it gets back holds +// more than one route, keeps reading until the cross-fade lands or the cap +// expires. It returns the last tree read, so the caller gets the settled one +// rather than paying for another read. +// +// Two nodes carrying the SAME route id are one destination, not a transition; +// countRouteScreens counts distinct tags, so a screen that nests a repeat of +// its own id does not pay this wait at all. +internal fun awaitSettledTree(read: () -> String): String { + var json = read() + if (countRouteScreens(json) <= 1) return json + pollUntilStable( + TRANSITION_POLL_CAP_MILLIS, + TRANSITION_STABLE_STREAK_MILLIS, + TRANSITION_POLL_INTERVAL_MILLIS, + ) { + json = read() + stabilitySnapshot(json) + } + return json +} + // pollUntilStable returns when the snapshot has been non-null and equal to -// itself for an uninterrupted stretch of at least MIN_STABLE_STREAK_MILLIS, -// capped at timeoutMillis. snapshot must omit transient attributes (e.g. -// measure-pass bounds) so layout-only flicker doesn't extend the wait, and -// must return null when the snapshot looks transitional (e.g. mid NavHost -// cross-fade) so the streak resets and the loop keeps polling instead of -// declaring a partial state stable. -internal fun pollUntilStable(timeoutMillis: Long, snapshot: () -> String?) { +// itself for an uninterrupted stretch of at least streakMillis, capped at +// timeoutMillis. snapshot must omit transient attributes (e.g. measure-pass +// bounds) so layout-only flicker doesn't extend the wait, and must return null +// when the snapshot looks transitional (e.g. mid NavHost cross-fade) so the +// streak resets and the loop keeps polling instead of declaring a partial +// state stable. +// +// The streak is measured from the start of the read that opened the current +// run of identical snapshots, not from when that read returned: a hierarchy +// fetch is not instantaneous, and the UI changing mid-fetch would have changed +// the snapshot, so the fetch's own duration is evidence of stability. On +// Android a fetch costs more than the poll interval, so charging it to the +// streak is the difference between two reads and four. +internal fun pollUntilStable( + timeoutMillis: Long, + streakMillis: Long = MIN_STABLE_STREAK_MILLIS, + intervalMillis: Long = STABILITY_POLL_INTERVAL_MILLIS, + snapshot: () -> String?, +) { if (timeoutMillis <= 0) return val deadline = System.currentTimeMillis() + timeoutMillis + var runStart = System.currentTimeMillis() var prior = try { snapshot() } catch (_: Exception) { null } - var streakStart = 0L while (System.currentTimeMillis() < deadline) { - Thread.sleep(STABILITY_POLL_INTERVAL_MILLIS) + Thread.sleep(intervalMillis) + val currentStart = System.currentTimeMillis() val current = try { snapshot() } catch (_: Exception) { null } - val now = System.currentTimeMillis() if (prior != null && current != null && prior == current) { - if (streakStart == 0L) streakStart = now - if (now - streakStart >= MIN_STABLE_STREAK_MILLIS) return + if (System.currentTimeMillis() - runStart >= streakMillis) return } else { - streakStart = 0L + runStart = currentStart } prior = current } @@ -119,36 +181,43 @@ private val ROUTE_TAG_KEYS = setOf( private val jsonMapper = com.fasterxml.jackson.module.kotlin.jacksonObjectMapper() +// countRouteScreens counts DISTINCT route-level destination tags, not the nodes +// carrying them. A screen that nests a node repeating its own route id puts two +// tagged nodes in the tree while only one destination is on screen; counting +// nodes would read that as a cross-fade that never ends, and the caller would +// pay its full poll budget on every step of that screen and never settle. internal fun countRouteScreens(treeJson: String): Int { if (treeJson.isBlank()) return 0 return try { val root = jsonMapper.readTree(treeJson) - countRouteScreens(root) + val tags = mutableSetOf() + collectRouteScreens(root, tags) + tags.size } catch (_: Exception) { 0 } } -private fun countRouteScreens( +private fun collectRouteScreens( node: com.fasterxml.jackson.databind.JsonNode, -): Int { - var count = 0 + into: MutableSet, +) { val attributes = node.get("attributes") if (attributes != null && attributes.isObject) { for (key in ROUTE_TAG_KEYS) { val value = attributes.get(key) ?: continue if (value.isNull) continue - if (value.asText().endsWith("Screen")) { - count++ + val text = value.asText() + if (text.endsWith("Screen")) { + into.add(text) break } } } val children = node.get("children") if (children != null && children.isArray) { - for (child in children) count += countRouteScreens(child) + for (child in children) collectRouteScreens(child, into) } - return count } // structuralHash hashes a Maestro TreeNode-shaped JSON string by walking it @@ -873,13 +942,29 @@ class MaestroDriverBackend(private val serial: String?) : DriverBackend { override fun recentLogs(sinceUnixMillis: Long, minLevel: String) = readLogcat(serial, sinceUnixMillis, minLevel) + // snapshot waits out a NavHost cross-fade before it reads, so the runner is + // never handed a tree holding two routes at once. It belongs here rather + // than in waitForIdle: the runner gives waitForIdle a one-second deadline + // and abandons the RPC when it expires, which is not enough room for a + // 700ms fade that began before the settle did, and a wait that outlives the + // deadline just races the runner's own fetch on the device-side server. + // The snapshot RPC carries the step's deadline instead, so the wait can run + // to a bound that actually covers the animation. + // + // The predicate costs nothing on a settled frame: the read it needs is the + // read the snapshot was going to do anyway. That is what makes this + // affordable, where the structural poll that used to run in waitForIdle was + // not: it fetched the hierarchy ~4 more times on every mutating step. + override fun snapshot(): SnapshotSample = + SnapshotSample(awaitSettledTree { hierarchy() }, screenshot()) + override fun waitForIdle(durationMillis: Long) { // waitForAppToSettle blocks on the View-system animation and maestro's - // own structural settle, which is enough on its own. A follow-up - // structural-hash poll used to run here, but each hierarchy fetch is - // ~500ms on a physical device, so it cost ~2.8s per mutating step for - // marginal benefit; the runner already re-fetches while a frame still - // looks transitional. + // own structural settle. It cannot see a Compose cross-fade: the fade + // keeps both routes alive with the tree byte-identical, so a settle + // that watches for change returns in the middle of one. That is what + // snapshot above waits out; a structural poll here used to try, cost + // ~2.8s per mutating step, and was removed. driver.waitForAppToSettle(null, null, durationMillis.toInt()) } diff --git a/sidecar/src/test/kotlin/dev/sanderling/sidecar/RouteTransitionTest.kt b/sidecar/src/test/kotlin/dev/sanderling/sidecar/RouteTransitionTest.kt new file mode 100644 index 0000000..41e8e0f --- /dev/null +++ b/sidecar/src/test/kotlin/dev/sanderling/sidecar/RouteTransitionTest.kt @@ -0,0 +1,95 @@ +package dev.sanderling.sidecar + +import org.junit.Test +import kotlin.test.assertEquals +import kotlin.test.assertTrue + +// The Android backend's snapshot waits out a NavHost cross-fade before it +// reads. Without that wait the runner is handed a tree holding two routes at +// once, refuses to act on it, and the step applies nothing; how many steps land +// that way varies run to run, so the same seed walks a different trajectory +// each time. These cover the wait (awaitSettledTree) and what it costs. +class RouteTransitionTest { + private fun screen(id: String, child: String = "") = + """{"attributes":{"resource-id":"$id"},"children":[$child]}""" + + private fun tree(vararg children: String) = + """{"attributes":{"resource-id":"root"},"children":[${children.joinToString(",")}]}""" + + private val crossFade = tree(screen("LedgerScreen"), screen("AddTransactionScreen")) + private val landed = tree(screen("AddTransactionScreen")) + + @Test fun waitsForTheCrossFadeToLandAndReturnsTheLandedTree() { + // The first read caught both routes alive. The wait must keep reading + // until only the destination is left, and hand back that tree: the + // caller records what it returns, so returning the fade would put the + // frame in the trace whether or not the wait happened. + var reads = 0 + val settled = awaitSettledTree { + reads++ + if (reads <= 3) crossFade else landed + } + assertTrue(reads > 3, "must keep reading until the fade lands, reads=$reads") + assertEquals(1, countRouteScreens(settled), "must return a tree with one route") + } + + @Test fun settledFrameCostsExactlyOneRead() { + // A hierarchy read is the expensive part of a step, and this one is the + // read the snapshot was going to do anyway. A frame that is already on + // one route must not pay for a second: that cost, on every step, is why + // the unconditional structural poll was removed from waitForIdle. + var reads = 0 + val settled = awaitSettledTree { + reads++ + landed + } + assertEquals(1, reads, "a one-route frame must read once and return") + assertEquals(landed, settled) + } + + @Test fun repeatedRouteIdIsOneScreenNotATransition() { + // A screen nesting a node that repeats its own route id puts two tagged + // nodes in the tree while one destination is on screen. Counting nodes + // would read that as a fade that never ends: every step of that screen + // would burn the whole poll budget and still hand over a frame the + // runner refuses to act on. + val nested = tree(screen("HomeScreen", screen("HomeScreen"))) + assertEquals(1, countRouteScreens(nested), "the same id twice is one route") + var reads = 0 + awaitSettledTree { + reads++ + nested + } + assertEquals(1, reads, "a repeated route id must not be treated as a transition") + } + + @Test fun aLayoutThatKeepsTwoRoutesIsBoundedByTheCap() { + // Two routes alive at rest is a real layout, not a fade, and no amount + // of waiting will resolve it. The wait is bounded by wall clock, so + // such a screen costs the cap once per step and nothing more. + val start = System.currentTimeMillis() + val settled = awaitSettledTree { crossFade } + val elapsed = System.currentTimeMillis() - start + assertTrue( + elapsed >= TRANSITION_POLL_CAP_MILLIS, + "must actually wait out a two-route tree, elapsed=${elapsed}ms", + ) + assertTrue( + elapsed < TRANSITION_POLL_CAP_MILLIS + 1000L, + "must stop at the cap, elapsed=${elapsed}ms", + ) + assertEquals(crossFade, settled, "the caller still gets a tree to record") + } + + @Test fun capCoversTheNavHostFadePlusTheStreak() { + // Compose navigation's default enter and exit are a 700ms tween, and + // the fade starts when the action lands, not when the snapshot begins. + // A cap that does not clear the fade and the streak after it leaves the + // frame transitional, which is the whole defect. + assertTrue( + TRANSITION_POLL_CAP_MILLIS >= 700L + TRANSITION_STABLE_STREAK_MILLIS, + "cap ${TRANSITION_POLL_CAP_MILLIS}ms cannot cover a 700ms fade plus a " + + "${TRANSITION_STABLE_STREAK_MILLIS}ms streak", + ) + } +} diff --git a/sidecar/src/test/kotlin/dev/sanderling/sidecar/StabilityPollTest.kt b/sidecar/src/test/kotlin/dev/sanderling/sidecar/StabilityPollTest.kt index de9937e..f4e31af 100644 --- a/sidecar/src/test/kotlin/dev/sanderling/sidecar/StabilityPollTest.kt +++ b/sidecar/src/test/kotlin/dev/sanderling/sidecar/StabilityPollTest.kt @@ -21,23 +21,28 @@ class StabilityPollTest { // begun must reset the clock: the post-transition stable window has // to start over and meet MIN_STABLE_STREAK_MILLIS from scratch. var calls = 0 - val start = System.currentTimeMillis() - // First 4 samples are "calm", then 1 transient change, then "stable" - // forever - the calm prefix is meaningless because of the transition. + var transientAt = 0L + // A short "calm" prefix, one transient change, then "stable" forever. + // The prefix is meaningless because of the transition: only the streak + // that starts after it can end the wait. pollUntilStable(3000L) { calls++ when { - calls <= 4 -> "calm" - calls == 5 -> "transient" + calls <= 2 -> "calm" + calls == 3 -> { + transientAt = System.currentTimeMillis() + "transient" + } else -> "stable" } } - val elapsed = System.currentTimeMillis() - start + val sinceTransition = System.currentTimeMillis() - transientAt + assertTrue(calls > 3, "must keep sampling past the transition, got $calls") assertTrue( - elapsed >= MIN_STABLE_STREAK_MILLIS, - "post-transition streak must reach ${MIN_STABLE_STREAK_MILLIS}ms, elapsed=${elapsed}ms", + sinceTransition >= MIN_STABLE_STREAK_MILLIS, + "the calm prefix must not count: a full ${MIN_STABLE_STREAK_MILLIS}ms streak has to " + + "start over after the transition, returned ${sinceTransition}ms after it", ) - assertTrue(calls >= 10, "expected enough samples to span calm + transient + stable streak, got $calls") } @Test fun transitionalNullsForceLoopToKeepWaiting() {