gui: M1 DoD harness — scroll-60fps, rss-flat, unhappy-path
gui/docs/pkg-qa-requests-m1.md R3, filed by the previous session: three GUI M1 DoD items (10k rows at 60 fps, flat RSS over 10 minutes, --slow/--flaky/--drop-connection recovery) had nowhere to run in CI. This is the harness — `gui/tests/dod/run.sh <gate> [--json <path>]`, exactly the path/invocation contract tests/integration/README.md already specified — plus `gui/tests/dod/dod_harness.cpp`, the Qt/RpcClient-driven binary that actually runs each gate against a real mockd run.sh starts and tears down itself. - scroll-60fps: an eased scripted scroll over the whole loaded table, timing each step's synchronous repaint; p99 against a 16.6 ms budget (auto-scaled 4x under a sanitized build — ASan/UBSan overhead, not a loosened bar, see the harness's isSanitizedBuild()). - rss-flat: samples this process's own VmRSS at 1 Hz across the run, discards a warm-up window, checks post-warm-up growth against a stated 20 MiB slack. - unhappy-path: three phases (slow/flaky/drop-connection), each its own mockd instance; passes when the client reaches and holds Connected with no crash or hang. A watchdog (the harness's own QTimer, backstopped by run.sh's external `timeout`) turns a genuine hang into a bounded non-zero exit rather than needing the CI caller to timeout(1) around it. Every gate honours the exit-code and --json contract PKG/QA's pre-drafted CI job expects unchanged (one addition needed: the build step must also build the `gui-dod-harness` target, noted in the R3 update). No leaked mockd processes on any exit path (`trap cleanup EXIT INT TERM`); no writes outside a tempdir except the caller's own --json path. Verified live end-to-end (not just unit-level): all three gates run against a real mockd under the exact `ASAN_OPTIONS=detect_leaks=1:halt_on_error=1` `.github/workflows/ci.yml`'s sanitizers job already sets, all pass, and scroll-60fps was forced red once on purpose (VELOX_DOD_FRAME_BUDGET_MS=1) to prove the fail path and exit code actually work. Building this is also what surfaced the two RpcClient bugs fixed in the previous commit, and one real gap in mockd itself — --drop-connection never worked over the Unix socket transport (only WebSocket) — filed as gui/docs/proto-requests-m1.md since tools/mockd is PROTO's file. gui/docs/pkg-qa-requests-m1.md R3 and R4 (an unrelated, non-blocking Qt6::DBus CMake hygiene note filed while wiring the clipboard global-shortcut path) are updated with the concrete findings above. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01NSCdCWFXBTSBBK3MzWtJiC
This commit is contained in:
Claude Sonnet 5
@@ -121,26 +121,101 @@ Notes for whoever applies it:
|
||||
|
||||
---
|
||||
|
||||
## R3 — the GUI DoD gates have nowhere to run in CI
|
||||
## R3 — the GUI DoD gates have nowhere to run in CI — RESOLVED, ready to wire in
|
||||
|
||||
To be precise about what already works: `VELOX_BUILD_GUI` defaults `ON`, the `ci` preset
|
||||
inherits `dev`, and once `gui/` is on `main` the `build` and `sanitizers` jobs configure
|
||||
and build `velox-gui` and run `ctest --preset ci`, which picks up all three GUI checks
|
||||
(`gui_downloadtablemodel`, `gui_rtl`, `gui_no_download_logic`). That part is covered.
|
||||
**Status: done on GUI's side.** The harness `tests/integration/README.md` was waiting on
|
||||
now exists, builds, and has been run end-to-end (all three gates, all three unhappy-path
|
||||
sub-phases) against a real `mockd` with no changes needed to the pre-drafted job below.
|
||||
|
||||
What has no home is the part of the GUI M1 definition of done that isn't a unit test:
|
||||
**Path:** `gui/tests/dod/run.sh <gate> [--json <path>]`, exactly the contract
|
||||
`tests/integration/README.md` specifies. `<gate>` is `scroll-60fps` | `rss-flat` |
|
||||
`unhappy-path`. It builds and tears down its own `mockd` (isolated `XDG_RUNTIME_DIR` via
|
||||
`mktemp -d`), needs no network, and leaves nothing running on any exit path (`trap
|
||||
cleanup EXIT INT TERM`) — verified live by checking for orphaned `tsx`/`mockd` processes
|
||||
after both a passing and a forced-failing run of each gate.
|
||||
|
||||
1. **10 000 rows scroll at 60 fps** (`mockd --tasks 10000`) — needs a frame-timing probe
|
||||
against the offscreen (or Xvfb) view; red when a scroll frame exceeds ~16 ms at the
|
||||
99th percentile.
|
||||
2. **Flat memory over 10 minutes of progress events** — needs RSS sampled across a
|
||||
10-minute `mockd --tasks 10000` run; red when RSS grows more than a small fixed slack
|
||||
(a leak in the progress-patch path is the thing this catches).
|
||||
3. **Unhappy-path recovery** — `mockd --slow`, `--flaky <f>`, `--drop-connection <s>`:
|
||||
the client must show the banner and recover without a freeze or crash; red on a crash,
|
||||
a hang (watchdog), or the connection state never returning to `Connected`.
|
||||
**One change from the pre-drafted job:** the "Configure + build" step needs to also build
|
||||
the harness binary, not just `velox-gui`:
|
||||
```diff
|
||||
- cmake --build --preset dev --target velox-gui
|
||||
+ cmake --build --preset dev --target velox-gui gui-dod-harness
|
||||
```
|
||||
Nothing else in the pre-drafted YAML needs to change — the `# TODO(GUI): path` comment on
|
||||
the `scroll-60fps` line can just come off along with the marker on the line below it.
|
||||
|
||||
GUI owns writing that harness (`gui/tests/` + a driver script, headless against `mockd`).
|
||||
Wiring it into `.github/workflows/ci.yml` as its own job — with the 10-minute one likely
|
||||
`nightly` rather than per-PR — is PKG/QA. Say the word and it comes over as a follow-up
|
||||
request with the job stanza pre-written.
|
||||
**`rss-flat`'s slack: 20 MiB (`VELOX_DOD_RSS_SLACK_KIB`, default `20480`).** Chosen by
|
||||
running the gate locally (`VELOX_DOD_RSS_DURATION_SEC=8` override, i.e. not the real
|
||||
10-minute number) a handful of times against `mockd --tasks 10000 --seed 1` and looking at
|
||||
actual post-warm-up growth (single-digit MiB per run here) — 20 MiB gives real headroom
|
||||
above that noise floor without being so loose a genuine per-tick leak in the progress-patch
|
||||
path could hide under it. This has **not** been proven against a full real 10-minute
|
||||
sanitized run (that's the nightly job's own first execution) or tuned against
|
||||
production-length data yet; treat it as GUI's stated starting number, not a
|
||||
load-tested constant, and expect it may need retuning after the first few real
|
||||
`gui-dod-nightly` runs land actual series data.
|
||||
|
||||
**One caveat worth deciding on explicitly: the pre-drafted job builds with `cmake --preset
|
||||
dev`, i.e. ASan+UBSan (CLAUDE.md: "default for all lanes").** `scroll-60fps`'s frame
|
||||
budget already compensates (`gui/tests/dod/dod_harness.cpp` multiplies 16.6 ms by 4× when
|
||||
it detects a sanitized build — measured p99 here was ~50 ms against ASan overhead, well
|
||||
under the scaled 66.4 ms budget, so the multiplier is doing real work, not padding for no
|
||||
reason). `rss-flat`'s slack does **not** get a similar adjustment — ASan's allocator
|
||||
(redzones, quarantine) can look like real growth over a long run in a way this hasn't been
|
||||
validated against yet. Two honest options: run `gui-dod-nightly` against a `release`-preset
|
||||
build instead of `dev` (loses the sanitizers' own bug-catching value for this one job), or
|
||||
accept `VELOX_DOD_RSS_SLACK_KIB` may need a second, larger number for the ASan build once
|
||||
real 10-minute data exists. GUI's preference is the second (keep sanitizers on
|
||||
everywhere), but this is genuinely PKG/QA's call since it's their job definition.
|
||||
|
||||
**Verified live**, under the exact `ASAN_OPTIONS=detect_leaks=1:halt_on_error=1` the
|
||||
`sanitizers` job already sets (checked against `.github/workflows/ci.yml` rather than
|
||||
assumed) — no leak-suppression flag needed, on any of the three gates:
|
||||
|
||||
* `scroll-60fps` against `mockd --tasks 10000 --seed 1`: PASS at p99 ≈ 50 ms (budget
|
||||
66.4 ms sanitized). Forced red once via `VELOX_DOD_FRAME_BUDGET_MS=1` to confirm the
|
||||
fail path and exit code actually work, not just the pass path.
|
||||
* `rss-flat` at `VELOX_DOD_RSS_DURATION_SEC=6/8`: PASS, ~4-6 MiB growth against the
|
||||
20 MiB slack.
|
||||
* `unhappy-path`, all three phases (`--slow 900`, `--flaky 0.3`, `--drop-connection 5`):
|
||||
PASS. One real finding from building this: `--drop-connection` is currently a no-op
|
||||
over the Unix socket transport in `mockd` itself (only wired for WebSocket) — filed as
|
||||
`gui/docs/proto-requests-m1.md` since that's PROTO's file to fix, not GUI's. The gate
|
||||
still passes today on the weaker (but real, and the actually-documented) condition that
|
||||
the client reaches and holds `Connected`; it just isn't proving a real mid-session drop
|
||||
yet for that one phase.
|
||||
|
||||
Two bugs surfaced and fixed *by* building this harness, both in `gui/`'s own RPC client
|
||||
(caught by ASan, not assumed): `RpcClient::stop()` left `conn_` dangling after joining its
|
||||
worker thread, so any caller that called `stop()` and then let the client destruct hit a
|
||||
double-free — the harness's own `client.stop()` at shutdown found it on the first run.
|
||||
Separately, `RpcClient`'s initial `download.list` call had a hardcoded `limit: 1000` with
|
||||
no paging, silently capping the table at 1000 rows regardless of how many the daemon
|
||||
actually has — `scroll-60fps` against `--tasks 10000` refused to run rather than
|
||||
"passing" against a 1000-row table, which is what caught it. Both fixed on `lane/gui`
|
||||
before this request was filed.
|
||||
|
||||
---
|
||||
|
||||
## R4 — root `CMakeLists.txt`'s `find_package(Qt6 ...)` should list `DBus` explicitly
|
||||
|
||||
Not currently broken — flagging a "works, but by accident" for the record. `gui/src/
|
||||
clipboard/GlobalShortcut.cpp` (docs/06-risks-and-spikes.md R2's explicit path #2:
|
||||
`org.freedesktop.portal.GlobalShortcuts`) needs `Qt6::DBus`. **Verified live: the target
|
||||
already exists and links today**, even though the root `find_package` doesn't list `DBus`
|
||||
in `COMPONENTS` — this Qt 6 packaging apparently exports every module's CMake target once
|
||||
any component pulls in the shared prefix, `DBus` included. `gui/CMakeLists.txt` still
|
||||
guards the clipboard sources on `if(TARGET Qt6::DBus)` (same pattern the file already uses
|
||||
for `veloxproto`), so if that turns out to be environment-specific rather than a general
|
||||
Qt 6 CMake guarantee, the build degrades to "feature skipped," not "build broken," on
|
||||
whatever machine finds out otherwise.
|
||||
|
||||
Worth making explicit anyway, since relying on undocumented target leakage is fragile:
|
||||
|
||||
```diff
|
||||
- find_package(Qt6 6.6 REQUIRED COMPONENTS Widgets Svg Network LinguistTools)
|
||||
+ find_package(Qt6 6.6 REQUIRED COMPONENTS Widgets Svg Network DBus LinguistTools)
|
||||
```
|
||||
|
||||
No apt change needed either way — `qt6-base-dev` (already in `APT_GUI`) ships `QtDBus`'s
|
||||
headers directly (verified live: `dpkg -L qt6-base-dev | grep -i dbus` lists the whole
|
||||
`QtDBus/` include tree).
|
||||
|
||||
Reference in New Issue
Block a user