fix(migration): wait for the Playground reply text, not for its bubble (#1143) - #1144
Merged
Merged
Conversation
…ble (#1143) Run #116 failed `nightly_ui/execute_flow_ui` with `assert ''` against a healthy product. `div-chat-message` is rendered the moment the machine message is created, so `to_be_visible()` resolved against a bubble still holding its loading dot, and `inner_text()` read an empty string ~1.5 s after send. The evidence that this is the test's fault and not the nightly's: - the run's own screenshot artifact shows the last AI bubble as a loading dot while the two earlier replies hold full text, and the stop button still visible in the composer — the stream was in flight; - `nightly_ui/execute_flow_api_post_update`, the next step, ran the same flow on the same nightly and got "Ahoy there, matey! The answer ye seek be 4..."; - `latest/execute_flow` and `nightly_api/execute_flow_api` both answered too. So the assertion waited for the reply's presence and then read its content, with nothing between the two. Introduced by #1139, which replaced a `to_be_hidden("button-stop")` check that could pass with no run at all; this is the symmetric mistake — reading a value before it exists rather than reading an absence as success. `to_have_text(re.compile(r"\S"))` waits for the text itself. Regex expectations are matched browser-side with `RegExp.test()`, so `\S` holds for any text with a non-whitespace character — verified against the installed `playwright._impl._helper.to_expected_text_values`. A bounded settle loop then lets the stream finish, so the step's recorded detail is the whole reply rather than its first tokens: a truncated reply in the report reads like a broken one, and a report that misleads its reader is the class of defect #1120 and #1141 were about. It cannot mask a failure — a stream that produced no text at all has already failed the assertion above it. `button-stop` is still deliberately not used as the completion signal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1143 — the
daily/dispatch migration run's own failure issue, opened by run #116.What failed
Run #116 failed
nightly_ui/execute_flow_uiwithassert ''against a healthy product:div-chat-messageis rendered the moment the machine message is created, soto_be_visible()resolved against a bubble still holding its loading dot, andinner_text()read an empty string ~1.5 s after send. The assertion waited for the reply's presence and then read its content, with nothing in between.Three independent pieces of evidence say the nightly was fine and the test was wrong:
nightly_ui/execute_flow_api_post_update, the very next step, ran the same flow on the same nightly and got"Ahoy there, matey! The answer ye seek be 4...";latest/execute_flowandnightly_api/execute_flow_apiboth answered too.Introduced by #1139, which replaced a
to_be_hidden("button-stop")check that could pass with no run at all. This is the symmetric mistake: reading a value before it exists, rather than reading an absence as success.The fix
Regex expectations are matched browser-side with
RegExp.test()— search, not full match — so\Sholds for any text containing a non-whitespace character. Verified against the installedplaywright._impl._helper.to_expected_text_values, which routes aPatternthroughexpected_regex(...); this was the one real correctness risk in the change and it is not left to assumption.A bounded settle loop (30 × 500 ms) then lets the stream finish, so the step's recorded
detailis the whole reply rather than its first tokens — a truncated reply in the report reads like a broken one, and a report that misleads its reader is the class of defect #1120 and #1141 addressed. It cannot mask a failure: a stream that produced no text at all has already failed the assertion above it.button-stopis still deliberately not the completion signal —to_be_hiddenis satisfied by an element that never existed.Validation — two real runs, same environment
Both
workflow_dispatchruns ofmigration-test.yml,1.11.1 → 1.12.0.dev10, same template, same runner image. Only the assertion differs.failuresuccessFAILEDPASSEDnightly_uiphasefailure, 35.8 ssuccess, 38.6 sexecute_flow_uiFAIL — the reply bubble rendered emptyPASS — Ahoy there, matey! The answer ye seek be 4, just like the number of cannons on me ship! Arrr!Force-fail: run #116 is the executed force-fail — the pre-fix code is the mutation, and it failed red on this exact assertion. Per
CONTRIBUTING.md, a pre-fix run that fails is valid force-fail evidence for the fix. The recordeddetailin run #117 also confirms the settle loop: the full sentence, not a truncated prefix.As a side benefit, run #117 exercises #1139's
PHASE_OUTCOME_*and #1141'sJOB_STATUSon the green path too (runner outcome: successper phase,**Job status (runner):** \success``); before this, both were only observed on a red run.Not validated locally, and why: the local OpenAI key is drained, so the migration flow does not answer on this machine and the streaming race cannot be reproduced here. What could be checked locally was: the file compiles, and the regex semantics read out of the installed Playwright source. The proof that matters is the pair of runs above, in the environment where the race actually occurs.
One thing this run exposed, filed separately
Run #117 closed #1143 by itself —
Close issue on successis guarded byif: success()with no ref check, so a green run of any branch closes the tracking issue for a bug still present onmain. Same family as the defects above: one signal (a green run) read as proof of something else (fixed on the default branch). Filed as its ownqa-infraissue rather than folded in here, since it is workflow scope and overlaps the blocks PR #793 is already editing. I reopened #1143 so this PR'sClosesis meaningful.No spec files are touched, so nothing here creates flows.
🤖 Generated with Claude Code