Skip to content

test: observe the service restart and the phone calendar tap deterministically - #567

Merged
mrgoonie merged 5 commits into
mainfrom
test/543-flaky-transient-states
Oct 7, 2026
Merged

mrgoonie merged 5 commits into
mainfrom
test/543-flaky-transient-states

Conversation

@mrgoonie

@mrgoonie mrgoonie commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

Refs #543

Makes two of the flaky tests listed in #543 watch their transient state reliably. Each cause was shown from code and a reproduction before the fix. No retries, timeouts or assertions were loosened.

1. Service simulator: "timed out waiting for restart disables the add binding"

packages/widget-cli/test/service-simulator-real-browser.e2e.spec.ts (CI run 37560419435, job 112596164161)

Cause. A simulated service restart set the services to loading, and a fixed 2 s timer then set them ready again. The browser has to do several steps inside that window before the widget shows the disabled state:

  • the shell reloads 500 ms after the POST;
  • the widget frame loads;
  • the bridge handshake runs;
  • availability arrives, and only then are the buttons disabled.

On a slow runner those steps outlast the window, so the disabled state is never drawn. A MutationObserver recorder at 8x CPU throttle confirmed this: the widget never rendered the disabled state before the timer fired. Polling harder cannot see a state that never exists.

Fix. startDevHost takes a new option, serviceRestart: "held", which keeps the restarted services loading until DevHost.finishServiceRestart() is called. The option is test-only and defaults to the existing 2 s timed restart, so dev-host behaviour is unchanged. The real-browser test holds the restart, waits for both bindings to be disabled, finishes the restart, and then waits for recovery. A unit test in dev-host.spec.ts covers the held restart.

Evidence (local Windows, with Chromium CPU throttled through CDP Emulation.setCPUThrottlingRate):

Run Result
Before, 4x throttle failed 3/3 (and 1/2 in a second batch)
Before, 8x throttle failed 10/10: 8 at "restart disables the add binding", 2 at "restart recovery"
After, 8x throttle passed 10/10
After, unthrottled, 8 in parallel passed 40/40

Load alone does not reproduce the failure: the old test also passed 40/40 unthrottled.

2. Calendar phone tap: the tap selected the wrong event

apps/web/e2e/calendar-views.spec.ts, "the week view is usable at phone width…" (was line 422)

Cause. The test is from the CI trace of run 37495565659 (artifact e2e-test-results). The tap went to (213, 383.78), the centre of the offsite event's box. Screencast frames show the content moving about 39 px up during the tap. The touch then selected evt_deploy, the next event down, and the widget action POST carried that event too. The scroller went 216 → 316 → 503, and Playwright logged "element is not stable" several times before "stable". Two motions can still be running after the test centres the event:

  • The follow-bottom effect smooth-scrolls .cc-scroll (scroll-behavior: smooth) toward the bottom, and can keep going or start again after the instant scrollIntoView. One local run saw 55 px of extra scroll.
  • The calendar's .cc-row and .cc-card play the cc-enter entrance with a bounce easing. It can pause at its turning point long enough for Playwright's stability check to pass, and then move the event again.

Locally the old test passed, but in 12 instrumented runs the event still moved 4–25 px after it was positioned. A scroll-still check alone left 17–25 px of movement, all of it from the entrance animation.

Fix. Before tapping, the test now:

  1. waits for every finite animation on the event's ancestors to finish (animation.finished);
  2. re-centres the event until the transcript is still across two frames and sits further from its bottom than BOTTOM_FOLLOW_SLACK_PX, so the client no longer follows it;
  3. runs the existing elementFromPoint hit test, then taps.

Evidence:

Run Event movement after positioning Result
Before, 12 instrumented runs 4–25 px passed locally; the CI failure is above
After, 15 instrumented runs 0 px in every run 15/15 passed
After, whole spec, --repeat-each=5 — 45/45 passed

Not covered here

  • Service artifact input probe (item 1): addressed by test(runtime): write the probe's report aside and rename it into place #547.
  • Lazy-media note: the issue body and its comments contain no lazy-media note, so there was nothing to reproduce.
  • map-view, audio-document, model-pool, the widget.spec gallery, image-generator:558: not addressed in this PR. Their causes were not proven.

Verification

  • pnpm typecheck passes.
  • pnpm invariants passes all 13 checks.
  • eslint is clean on all changed files.
  • vitest.widget-dev-host.config.ts: the held-restart test passes.

…as seen it

The service simulator's restart reported ready again two seconds after it began,
while the shell reloads half a second in and the widget then needs its frame,
bridge handshake and availability before it disables a button. On a slow runner
that pipeline outlasts the remaining window, so the disabled state was never
drawn and "restart disables the add binding" timed out.

The dev host now takes serviceRestart: "held", which keeps the services loading
until finishServiceRestart() is called. The real-browser test holds the restart,
waits for both bindings to be disabled, then finishes it and waits for recovery.
The default stays a two-second restart.
…settled

The phone week-view test tapped the offsite event right after centring it,
while the transcript could still be moving: the follow-bottom smooth scroll
can continue or restart after the instant scroll, and the calendar's row and
card enter with a bounce that can look stable at its turn and then move again.
In the failing CI trace the content moved about 39px under the tap point and
the touch selected the next event down (evt_deploy).

The test now waits for the entrance animations around the event to finish,
then re-centres the event until the transcript stays still and sits beyond
the follow-bottom slack, and only then hit-tests and taps.
@mrgoonie

mrgoonie commented Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

Review attestation: ready to merge at adf7e473355296ed7222d8148d4dc179a82b9f9a, reviewed by agent:code-reviewer.

A push to this PR makes this attestation stale; the new head needs its own review.

@mrgoonie
mrgoonie enabled auto-merge (squash) October 7, 2026 14:33

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Code review skipped — your organization has no extra usage available to pay for this review.

If your organization's extra usage balance is empty, an organization admin can add extra usage credits at claude.ai/admin-settings/usage. If its monthly spend limit was reached, an admin can raise it on the same page. If neither applies, contact Anthropic support.

Once extra usage is available, someone with write access to this repository can comment @claude review on this pull request to trigger a review.

@mrgoonie

mrgoonie commented Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

Review attestation: ready to merge at a7b5a5dc7ce9edc17e3605560614002106db3dfa, reviewed by agent:code-reviewer.

A push to this PR makes this attestation stale; the new head needs its own review.

@mrgoonie

mrgoonie commented Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

Review attestation: ready to merge at dc93682a76202f38cd4823f6960d2a9b3638fc19, reviewed by agent:code-reviewer.

A push to this PR makes this attestation stale; the new head needs its own review.

@mrgoonie
mrgoonie merged commit e836f55 into main Oct 7, 2026
33 checks passed
@mrgoonie
mrgoonie deleted the test/543-flaky-transient-states branch October 7, 2026 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant