Repository navigation
test(e2e): resize the emulated terminal before the PTY so a redraw is never parsed at the old size - #955
Open
jussielo-amd wants to merge 1 commit into
Open
test(e2e): resize the emulated terminal before the PTY so a redraw is never parsed at the old size#955jussielo-amd wants to merge 1 commit into
jussielo-amd wants to merge 1 commit into
Conversation
… never parsed at the old size The TUI driver resized the real PTY and then the emulated screen. The application redraws for the new size the moment it is signalled, and the reader thread parses that frame as soon as it arrives, so whenever the harness thread was descheduled between the two calls the frame, laid out for 120 columns, was parsed by a screen still 80 wide. Its lines wrapped and scrolled the top rows, the header with its warnings badge among them, off the screen, and because the dashboard only repaints the cells that changed the damage was never repaired. The dash-09 step that waits for the header badge to grow then timed out on a frozen, garbled screen, which is the intermittent failure seen on loaded runners; dash-08 and every other scenario that resizes shared the exposure. Resize the emulator first and the PTY second, under one lock, through a new helper in the library crate so a plain unit test can cover the ordering (the driver itself only builds into the cucumber binary). A failed PTY resize puts the emulated screen back, as before. Signed-off-by: Jussi Elo <jussi.elo@amd.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues are recorded for the terminal resize synchronization change.
No open issues 🎉
What changed in this PR
This PR addresses an intermittent dashboard end-to-end test failure by resizing the emulated terminal before the real PTY, preventing resize-triggered redraws from being parsed at the old dimensions. Production behavior is unchanged.
Changes:
- Adds a shared resize helper that holds the parser lock across both operations and restores the previous dimensions if the PTY resize fails.
- Routes
TuiSession::resize_tothrough the helper. - Adds four unit tests covering redraw handling, failure rollback, invalid dimensions, and return-value propagation.
| File | Description |
|---|---|
tests/e2e-cucumber/tests/e2e/tui_driver.rs |
Uses the shared helper for synchronized terminal resizing. |
tests/e2e-cucumber/src/terminal_size.rs |
Implements emulator-first resizing and adds regression tests. |
Review effort: Balanced
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 ROCMAI-907.
Summary
The Linux mock-lane scenario
@id:dash-gen-tps-expiry-boundary(dash-09) failed intermittently with "the failed scrape never reached the screen, so no frame here is known to postdate the failure: timed out after 30s waiting until the header warnings badge grows past 0", on pull requests that do not touch the dashboard. The failing screen in both CI logs I read is the same garbled frame: the Home summary cards drawn over the Observe tab, no header row at all, and the held "10.0 tok/s*" still visible. It is not the validity-window clock, which has been held by the harness since the earlier fix for the dash-08/09 timing and is not involved here. The screen the harness reads is corrupt, so the header's warnings badge can never appear in it.The cause is the order in which the TUI driver resized its terminal. The Observe step enlarges the pseudo-terminal to 120x40 and the driver resized the real PTY first and its emulated screen second. The dashboard redraws for the new size the moment it is signalled, and the reader thread parses that frame as soon as it arrives. If the harness thread is descheduled between the two calls, a frame laid out for 120 columns is parsed by a screen that is still 80 wide: its lines wrap, scroll the top rows (the header) off the screen, and leave cells in the wrong places. The dashboard only repaints cells that changed, so nothing ever repairs the damage and every later wait runs against a frozen, wrong screen. A loaded runner makes that stall likely enough to show up as a flake; dash-08 and every other scenario that resizes shared the exposure.
The fix resizes the emulated screen first and the PTY second, under one lock, through a new helper
resize_screen_then_ptyin the library crate (tests/e2e-cucumber/src/terminal_size.rs), andTuiSession::resize_tonow goes through it. Whatever the dashboard writes in answer to the resize is therefore parsed at the new size, and a frame it drew for the old size is only repainted over, because it clears and redraws on a resize. A failed PTY resize puts the emulated screen back, as before. No production code changes.Test plan
What I ran, on a 16-core WSL host with a debug
rocm/rocmdbuilt withrocm/e2e-test-hooks, running the builte2etest binary directly with-n dash-09:yesCPU burners, 0 failures; 96 more runs (12 loops of 8) with 32 burners, 0 failures. I could not reproduce the flake by load alone in 192 runs: the window is two consecutive calls, so it needs a scheduling stall exactly between them.a_frame_the_child_draws_in_answer_to_the_resize_lands_on_the_new_sizered (row 0 comes back empty: the header scrolled off) anda_failed_pty_resize_puts_the_emulated_screen_back_and_reports_the_errorred; restored by targeted edit.open_observe_view, the overflow, wide and detail steps,grow_rows). I ran the whole dash feature once (-n dash-): 48 of 49 scenarios passed; the one failure isdash-reports-wsl-gpu-telemetry(@requires-wsl @requires-gpu), which needs a real GPU under WSL and this host has none. I did not run it againstorigin/main.cargo test -p e2e-cucumber --lib(229 passed, 4 new),cargo test -p e2e-cucumber --test feature_naming,cargo fmt --all --check,cargo clippy --locked -p e2e-cucumber --test e2e -- -D warningsand--all-targetsfor the lib,cargo xtask manifest --check, and the commit hooks all pass.Not run: the full workspace gate,
cargo xtask e2ein release mode,scripts/smoke_local.py, and a Windows-target clippy (the helper is platform-neutral and nocfg(windows)code changed); CI carries them.User-observable behaviour is unchanged (this is test-harness code only), so no scenario is added; the existing dash-08 and dash-09 scenarios are the ones this keeps from flaking.