Skip to content

fix(e2e_ui): restore the route pump a revert removed from the optimistic-title test - #7899

Closed
yaoharry wants to merge 1 commit into
mainfrom
fix/e2e-ui-optimistic-title-route-pump
Closed

yaoharry wants to merge 1 commit into
mainfrom
fix/e2e-ui-optimistic-title-route-pump

Conversation

@yaoharry

Copy link
Copy Markdown
Collaborator

Related issue

Summary

tests/e2e_ui/sessions/test_new_session_optimistic_title.py::test_new_session_shows_first_prompt_optimistically
is failing on main right now, including on unrelated PRs:

E  AssertionError: the initial prompt was never POSTed to the session's /events
   (observed: []) — the auto-send path did not run
E  assert 'e2e sentinel optimistic label 9f4c2d' in []

observed: [] is the tell — not one event was recorded, rather than the wrong one.

Playwright's sync API dispatches route handlers only inside Playwright calls. The
sanity-check loop waits on event_texts, which is populated by an intercepted route
handler, but spins on time.sleep(0.05) — so handle_events never runs and a POST that
lands after the loop begins is never observed. The loop then burns its full 15s and asserts
on an empty list.

This is a regression from a revert, not a new bug. The wait_for_timeout pump and its
explanatory comment were added by PR #6362, and 821fa974c ("revert: restore the silent
omni update alias") reverted that whole commit to restore the omni update alias — taking
this unrelated Playwright fix with it as collateral damage. This change restores exactly
those three lines; nothing about the omni update alias is touched.

+    # Pump the driver with wait_for_timeout, not time.sleep: the sync API
+    # dispatches route handlers only inside Playwright calls, so a bare
+    # sleep loop never runs handle_events for a late-landing POST.
     deadline = time.monotonic() + 15
     while time.monotonic() < deadline and _PROMPT not in event_texts:
-        time.sleep(0.05)
+        page.wait_for_timeout(50)

Test Plan

  • Failure observed live on CI at E2E UI Tests (shard 3/10): the test RERUN then FAILED,
    i.e. it failed both the first attempt and the lane's automatic --reruns=1 retry —
    consistent with a deterministic never-dispatched handler rather than a timing blip.
  • Confirmed by git show 821fa974c -- tests/e2e_ui/sessions/test_new_session_optimistic_title.py
    that the revert is what removed the pump, and that the restored text matches the
    pre-revert version exactly.
  • Verified time is still imported and used elsewhere in the file, so the
    time.monotonic() deadline is unaffected.
  • pre-commit run --files tests/e2e_ui/sessions/test_new_session_optimistic_title.py → all hooks pass
    (including this repo's no unconditional @pytest.mark.skip rule).
  • This PR's own E2E UI Tests shards exercise the test directly; that is the real check.

Demo

  • Visual demo attached below
  • Non-visual evidence provided below or in Test Plan
  • Not applicable — no behavioral change

Type of change

  • Bug fix
  • Feature
  • UI / frontend change
  • Refactor / chore
  • Docs
  • Test / CI
  • Breaking change

Test coverage

  • Unit tests added / updated
  • Integration tests added / updated
  • E2E tests added / updated
  • Manual verification completed
  • Existing tests cover this change
  • Not applicable

Coverage notes

The change is to the E2E test itself, restoring a wait that makes its own sanity assertion
observable. The test is its own coverage: it fails deterministically without the pump on the
lane's retry as well as its first attempt, and this PR's E2E UI Tests shards run it.

This pull request and its description were written by Isaac.

Playwright's sync API dispatches route handlers only inside Playwright
calls, so a bare time.sleep loop never runs handle_events and a POST
that lands late is never recorded — the wait expires with no events
observed at all. Restore the wait_for_timeout pump, which a revert of an
unrelated CLI change removed along with it.

Co-authored-by: Isaac <no-reply@databricks.com>
Signed-off-by: harry-yao_data <harry.yao@databricks.com>
@omnigent-ci

omnigent-ci Bot commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

Polly AI Review

Blocking issues

None found. The change addresses the stated failure without unrelated changes or weakened assertions.

Security vulnerabilities

None found. No security boundaries, dependencies, or credential handling are affected.

Non-blocking notes

Runtime validation remains outstanding: this environment lacks Playwright, browsers, and the application server. Confirm the targeted test passes in the PR’s E2E UI lane. In a configured E2E environment, run:

python -m pytest tests/e2e_ui/sessions/test_new_session_optimistic_title.py::test_new_session_shows_first_prompt_optimistically

Approach

The approach is sound and consistent with existing tests. Yielding through Playwright allows intercepted requests to be handled while preserving the monotonic deadline and required prompt assertion. No materially simpler alternative is warranted.

Summary

The saved diff was checksum-verified. Static review found no correctness, security, or scope issues. The change addresses callback starvation without masking a missing initial prompt. The reported revert history could not be independently confirmed from the shallow checkout; successful E2E execution remains the final validation.


Automated review by Polly · workflow run

@yaoharry

Copy link
Copy Markdown
Collaborator Author

Suggest prioritising this one for review — it is currently the sole red check on four separate PRs.

test_new_session_optimistic_title is failing on main, so every open PR that runs E2E UI Tests (shard 3/10) inherits it. Observed so far on #7896, #7900, #7902, and this shard's sibling runs, always with the identical signature:

AssertionError: the initial prompt was never POSTed to the session's /events
(observed: []) — the auto-send path did not run

It fails on the first attempt and on the lane's automatic --reruns=1 retry, so it is deterministic rather than flaky-green — which is why it reddens every PR rather than some.

This PR is green across all 10 E2E UI shards (44 checks, no failures), and the change is a three-line restoration of a pump that 821fa974c removed as collateral while reverting an unrelated CLI alias change. Merging it should clear that shard for everyone.

This comment was written by Isaac.

@github-actions

Copy link
Copy Markdown
Contributor

Closed. If you want to pick this back up, comment /reopen. GitHub only lets maintainers press the Reopen button, so this command does it for you. It needs the source branch to still exist.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XS Pull request size: XS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant