Studio CI: make a failing browser smoke say why, and stop it skipping the build gates - #8983
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Correction to my earlier comment. I merged #8980 in to get past a red |
…9026) * Repair the prompt-queue contract test against the queueing refactor Repo tests (CPU) is red on main. test_composer_only_queues_behind_the _current_chat greps thread.tsx for code that #8952 moved or replaced, so it fails on any branch regardless of what that branch changes. It is currently red on #8935, #8964, #8980 and #8983 for this reason alone. The behaviour it guards is intact, and in two of the three cases the code that replaced it is stronger than what the test still asserted, so the assertions are repointed rather than dropped: - Queueing moved out of handleSubmit into an extracted queueComposerText, so the Cmd/Ctrl+Enter path could share it. Assert the delegation in handleSubmit and the queueing inside queueComposerText, which keeps this a contract on behaviour rather than on where the code sits. - promptQueueStartPendingRef.current.has(reservationKey) became a .get(reservationKey) === identity comparison. A reservation can be replaced between the start and the callback, and acting on the successor would dispatch the wrong prompt; presence alone never caught that. - temporary: useChatRuntimeStore.getState().incognito became temporary: incognitoAtQueueStart, captured when the queue starts instead of read live at dispatch. A chat toggled out of temporary mid-queue must not have its queued prompts persisted. Each rewritten assertion was checked against a deliberately broken tree rather than assumed to discriminate. Removing the delegation, swapping the identity check back to presence, reading temporary live again, and stopping queueComposerText from queueing each fail the file. tests/studio 11 passed for this file, whole directory green. * Read the identity check out of the dispatch guard, not the whole file Reported on this PR and correct. The assertion searched thread.tsx for promptQueueStartPendingRef.current.get(reservationKey) === anywhere, and the abort and cleanup branches beside the dispatch carry the same comparison. The dispatch guard could regress to .has(reservationKey) on its own, which is exactly the bug this assertion exists to catch, and the test would still pass on its neighbours. It now slices the if condition that guards the startPromptQueue call and asserts the identity comparison there, with .has excluded from that guard. All three comparisons are still required by count, because the other two are load-bearing too: abort without it reports the successor's start as this one's failure, and cleanup without it deletes the successor's entry. 11 passed. Rewriting the dispatch guard to .has fails it; it passed before. --------- Co-authored-by: danielhanchen <unslothshared@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
for more information, see https://pre-commit.ci
… gate ahead of the smokes The failure dump iterated the live deque the drain thread is still appending to, while writing each line to stdout, so a vite server that was still talking during the dump raised deque mutated during iteration and dropped the tail in exactly the noisy failure the dump was added for. list() of a deque is atomic; take it first. Startup bundle budget still ran after the browser smokes, and every step carries an implicit success(), so a red smoke skipped it. It only needs dist/, so it moves up with the other build gates.
|
On the red check: The failing assertion is
|
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Cross-platform runReplicated onto a staging repo rather than adding to the org queue, branched from fresh upstream
The one red is the inherited
The staging PR is closed, never merged. |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
# Conflicts: # .github/workflows/studio-frontend-ci.yml # tests/studio/test_autoscroll_harness_contract.py
Merged
|
| index | step |
|---|---|
| 12 to 15 | Build, unstable_Provider, Bundle size budget, Startup bundle budget |
| 16 to 17 | Install Chromium, Dev-server lifecycle tests |
| 18 to 21 | ANSI, autoscroll, research freeze, chat stream pacing (continue-on-error) |
| 22 | Upload browser smoke artifacts (if: failure()) |
main's new tests/studio/playwright_stream_pacing.py path trigger is kept too. Nothing dropped from either side.
tests/studio/test_autoscroll_harness_contract.py, purely additive. This PR adds three test functions, main adds two, at the same place. I kept every one and checked it rather than eyeballing: the merged file has 11 test functions, and the set difference against each side is empty in both directions, so no assertion was dropped from either.
Re-proved after the merge, because a merge that quietly neuters a guard is the failure mode worth guarding against. pytest tests/studio/test_autoscroll_harness_contract.py tests/studio/test_playwright_suites_run_in_ci.py is 16 passed. Then, on a deliberately broken tree, I moved Startup bundle budget back behind the browser smokes and test_no_build_gate_sits_behind_a_browser_smoke failed as it should, and passed again once restored. The guard still bites, and it correctly does not fire on main's new smoke, which sits after the gates.
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
# Conflicts: # .github/workflows/studio-frontend-ci.yml
Merged
|
| index | step |
|---|---|
| 10 to 13 | Build, unstable_Provider, Bundle size budget, Startup bundle budget |
| 14 to 15 | Install Chromium, Dev-server lifecycle tests |
| 16 to 21 | ANSI, autoscroll, research freeze, settings tab panels, settings panel that cannot load, chat stream pacing |
| 22 | Upload browser smoke artifacts (if: failure()) |
Verified by set difference rather than by reading: no step name and no pull_request.paths entry from either side is missing. Nothing dropped.
Re-proved after the merge. pytest tests/studio/test_autoscroll_harness_contract.py tests/studio/test_playwright_suites_run_in_ci.py is 16 passed; the wider enumerating suites (test_no_test_shadows_another.py, test_playwright_server_lifecycle.py, test_smoke_workflows_share_one_script.py) bring it to 43 passed. Then on a deliberately broken tree I moved Startup bundle budget back behind the smokes and test_no_build_gate_sits_behind_a_browser_smoke failed, and passed again once restored. The guard still bites, and it correctly does not fire on any of main's three new smokes, which all sit after the gates.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The stream-pacing smoke is continue-on-error, which rewrites its CONCLUSION to success while leaving its OUTCOME as failure. The artifact upload was gated on a bare failure(), so on the runs where that smoke was the only thing that failed -- exactly the runs where its report is the whole point -- the upload was skipped and logs/playwright-stream-pacing went nowhere. Give the step an id and OR its raw outcome into the upload condition. Guarded: the new test walks every continue-on-error browser smoke and fails if it has no id, or if its outcome is not named in the upload condition. Shown red two independent ways first, reverting the condition to bare failure() and separately deleting the step id, each naming the step.
for more information, see https://pre-commit.ci
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
Merged current No conflicts. Nothing in the merge touches this PR's own change (the build-gate ordering in Re-proved one guard red on the merged tree before pushing: reverting the upload step's |
|
Simulation evidence for this PR. The claim is narrow, so I tried to break it in the ways a real CI failure actually arrives rather than only the one that motivated it. The original failure, reproduced then fixedFive ways a browser smoke can fail, each run twice: once on the merge base and once on this branch, same worktree layout, same Chromium, same injected fault. What CI would have printed:
The three modes that improve are exactly the three where the base output stopped at a Playwright locator timeout with no statement of cause. On this branch the same run additionally prints For the storm case it prints the 500 and the aborted module request that caused it, and for the health-timeout case the repeated The two modes that do not improve are honest negatives and I am reporting them as such: the SPA-fallback and dead-config cases already raised a specific Artifacts also go from 1 file to 3 on every failing mode that reaches the browser: base wrote only Cross engineThe green arm and the throw arm were both run on Chromium, Firefox and WebKit. Green arm: all three exit 0 and all five production panes render clean text with no ANSI escapes. Throw arm: all three exit 1 and all three print the Signals and cleanupSIGTERM and SIGINT were sent mid-run. Neither leaves a vite server or a browser behind; SIGINT surfaces the usual The reordering halfThe other half of this PR moves the four build gates ahead of the browser smokes. Verified on this branch: Assertions proven redBoth new assertions were shown to fail on a deliberately broken tree, on the merged tree at
Staging CIReplicated onto a staging repo rather than adding load to the org queue. That staging run was created at 05:03 UTC from PR head GPU matrixNot applicable rather than skipped. This PR changes a GitHub Actions workflow and two Python assertion files. There is no CUDA, dtype or model axis to take a cartesian product over. Old installsNothing here is installed, persisted or migrated. No change to any file an existing |
# Conflicts: # .github/workflows/studio-frontend-ci.yml
|
Resolved the conflict with #9016 at Nothing was dropped from either side. What I kept:
No assertion from either side was dropped, and nothing was weakened to make the merge go through. Verified on the merged tree, not assumed:
The step order now reads: Unit tests, Build, unstable_Provider, Bundle size budget, Startup bundle budget, Install Chromium, Dev-server lifecycle tests, then the six browser smokes, then the failure-artifact upload. |
1 participant
"Browser smoke for ANSI tool output" is intermittently red. Across the runs I have watched reach the step, it failed 8 times and passed 3, on
mainand on branches alike, and it carries nocontinue-on-error, so every step behind it is skipped whenever it goes.This does not fix the smoke. It makes the smoke able to tell us what is wrong, and it stops the smoke deciding whether the build gates run.
What the failure looks like today
Run 31935573269,
main, in full:That is everything. No console, no page error, no dev-server output, no screenshot kept. An entry module that threw and an entry module that was merely slow produce exactly that text, and the two need opposite fixes.
I could not reproduce it here: 7 clean runs including a cold vite dependency cache and a run pinned to two cores. So the next step has to be evidence from the runner, which the harness currently discards.
Change
Say why.
echo_browser_errors(page, info)in_playwright_robust.pyprintspageerror,console.error,requestfailedand main-frame navigations as they arrive. All three smokes get it; none of them listened for any of it before. On failure the ANSI smoke also calls the existingdump_diagnostics(screenshot, URL, body excerpt) and prints vite's own tail, which is the only place a transform error or a forced reload is reported.Verified by injecting
throw new Error(...)at module scope insmoke-ansi-main.tsx. Before, that is indistinguishable from the log above. After:Stop it gating the build. The three smokes move below
Build, theunstable_Providerassertion and the 75 MB budget. A job stops at its first failing step, so today one red smoke means the checks that decide whether the app ships never report at all. The smokes each start their own vite dev server and read nothing out ofdist/, so the order is free. The smokes stay blocking; they just fail on their own account now.Keep the evidence.
actions/upload-artifactonlogs/playwright-*,if: failure()only, 3 day retention. Small, unlike the 35 MB dist upload.Coverage
Three assertions in
tests/studio/test_autoscroll_harness_contract.py, the existing file for "a harness must read what it records":Testing
python3 -m pytest tests/studio/test_playwright_server_lifecycle.py tests/studio/test_autoscroll_harness_contract.py -q: 25 passedplaywright_strip_ansi_smoke.py: passes, and fails with a named cause under the injected crashplaywright_research_freeze.py: passesplaywright_chat_autoscroll.py: fails here with "the content did not grow while detached", identically with and without this branch (checked by stashing it), so it is pre-existing and untouchedruff check tests/studio: cleanWhat this does not do
It does not fix the smoke, because I still cannot say what is wrong with it. Seven clean local runs, including one with a cold vite dependency cache and one pinned to two cores, did not reproduce it. It is not deterministic in CI either: it passed today on #8980, a branch whose only difference from
mainis three test-file path fixes that the smoke does not touch. An intermittent failure with no console, no page error and no server output is not diagnosable, which is what this PR changes.A green run of this branch therefore proves nothing on its own. The value is the next red one.