Repository navigation
chore(system-test): put the HeadlessBench rig in git, where a git clean cannot reach it - #261
Conversation
Verbatim copy of tools/system-test/HeadlessBench from the worktree it lived in untracked. bin/ and obj/ deliberately excluded. Integration with CI, the lock-file refresh script and a README follow in this branch. Refs #254
Compile it in CI, refresh its lock file with the others, and document it. Refs #254
|
/agentic_review |
PR Summary by QodoPreserve HeadlessBench system-test rig with CI compile coverage
AI Description
Diagram
High-Level Assessment
Files changed (8)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
… green run means more than it does
Qodo round 1 was right on three counts that are documentation problems, not
rescue problems:
* The run example used a fixed --out path while results.jsonl is appended and
never truncated, so following the README literally mixes two runs' verdicts
with nothing marking the boundary. The example now stamps the directory.
* CONN-DISC calls ConnectionManager.Disconnect, but a user's click runs
DaqifiViewModel.DisconnectDeviceCommand, which also unsubscribes active
channels from LoggingManager, removes the firmware notification and clears
SelectedDevice. The row can pass while that cleanup is broken.
* LOG-SESSION asserts toggle state only; session finalization is fire-and-forget
and unverified.
Program.cs is deliberately untouched — this PR's review anchor is that the
rescued rig is a verbatim copy. The fixes are queued on #260.
Refs #254, #260
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit e0d32ed |
Shepherd review — blast radius, merge order, and two disclosure defectsThis PR changes Measured against 1.
|
| combination | merge | HeadlessBench | tests | verdict |
|---|---|---|---|---|
main + #261 |
fast-forward | builds | 530 / 530 | safe (+ iOS Release ✓) |
| #261 + #255 | clean | builds | 560 / 560 | safe |
| #261 + #256 | clean (build.yml auto-merged) |
builds | 538 / 538 | safe |
| #261 + #258 | clean | builds | 535 / 535 | safe |
| #261 + #259 | clean | builds | 535 / 535 | safe |
| all five | see note | builds | 578 / 578 | safe (+ iOS Release ✓) |
Test counts are exactly additive (530 + 30 + 8 + 5 + 5 = 578), so nothing interferes.
Desktop stayed at 191 warnings on the five-way, and no lock file moved in any combination.
main + #261 is a fast-forward, which is worth stating plainly: the merge result is
e0d32ed3, so the green CI on that SHA is not an approximation of the post-merge state —
it is the post-merge state.
No merge order is required. #261 is safe to land in any position relative to the other
four. The reason is structural, and the builds confirm it: #261 touches no file that any
existing project compiles — no .cs, .axaml, .csproj or MSBuild-visible property —
so it cannot invalidate the other four's greens in either direction. The one new coupling,
the HeadlessBench compile, is green against all four individually and together.
Two things for whoever lands second, neither of them #261's doing:
build.yml(fix(firmware): a running upload can be cancelled, and it says what it is doing #256 × chore(system-test): put the HeadlessBench rig in git, where a git clean cannot reach it #261) auto-merges and I verified both sides survive intact:
expected=25and the "25 screens" comments from fix(firmware): a running upload can be cancelled, and it says what it is doing #256, plus the HeadlessBench comment
block and build line from chore(system-test): put the HeadlessBench rig in git, where a git clean cannot reach it #261. Glance at it, but it is fine.tools/parity-audit/README.md(fix(firmware): a running upload can be cancelled, and it says what it is doing #256 × fix(notifications): the flyout names its empty state instead of rendering a blank panel #258) does conflict — my five-way merge
surfaced it. chore(system-test): put the HeadlessBench rig in git, where a git clean cannot reach it #261 does not touch that file and is not involved. Note that after both land
the prose also needs a human pass, not just a marker resolution: fix(firmware): a running upload can be cancelled, and it says what it is doing #256 moves the baseline
to 25 screens while fix(notifications): the flyout names its empty state instead of rendering a blank panel #258's paragraph still says 24. fix(logged-data): the empty plot region is a black void — the axes and the card were both there, both gated off #259 auto-merges once that is settled.
5. Two disclosure defects — filed on #260, not pushed
The 7-vs-16 row gap is disclosed accurately and I did not try to reconstruct the missing
rows. grep for all ten strings (DEV-TILE, CH-TILE, DEV-NAME, DEV-RATE, CH-DIO,
CH-PWM, SD-LIST, DEV-NET, DEV-DEBUG, DAQIFI_RESTORE_NAME) returns 0 — they
genuinely are not in the source. Deferring the five conceded Qodo findings out of a rescue
was the right discipline. But the deferral chain did not close:
- The README's pointer was false. It says the five gaps are "Tracked in HeadlessBench: the 9 extra matrix rows and DAQIFI_RESTORE_NAME did not survive the rescue, and --scripted is still a stub #260". HeadlessBench: the 9 extra matrix rows and DAQIFI_RESTORE_NAME did not survive the rescue, and --scripted is still a stub #260
carried one of them (--scripted). The other four — including the strongest,
CONN-DISC— were documented in the README and nowhere else. I have recorded all four
on HeadlessBench: the 9 extra matrix rows and DAQIFI_RESTORE_NAME did not survive the rescue, and --scripted is still a stub #260, so the pointer is now true. I verified theCONN-DISCentry against the source
myself:DisconnectDeviceunsubscribes active channels, calls
ConnectionManager.Disconnect, callsRemoveFirmwareNotification, and clears
SelectedDevice— the rig does only the second of the four. The README's description of
that is exact. - The README contradicts itself on that same gap. Line ~21, under "It goes through the
user's code path, not a convenient one", says disconnection is
ConnectionManager.Instance.Disconnect(device), "which is what the Devices pane calls" —
the opposite of the Known gaps entry 60 lines below. The same wording is inProgram.cs
at theCONN-DISCblock. A reader who stops at the selling-point section gets exactly
the false confidence the Known gaps section exists to prevent.
Defect 2 needs a commit, and I have deliberately not pushed it: this PR has the largest
blast radius of anything open, its review is settled and its CI is green on the current head,
and a one-paragraph rewording is not worth resetting either. Filed on #260 instead.
Not verified here
The Android head does not build on this Mac (workload absent) — CI covers it and is green
on e0d32ed3. I did not re-run the bench rig; the builder's non-destructive run stands,
and its one [FAIL] is the known fw-3.7.2 timebase discrepancy, correctly not filed.
Every other number in the body re-derived against the final diff and checks out: +1449/−7
across 8 files, 413 rescued lines (380 + 33), +26/−7 of integration, 530/530, 191 warnings,
6 bugs → 0 (6 Qodo threads, all resolved), 6/7 bench rows.
|
Qodo-clean, CI green — ready for review at |
The PR is clean from Qodo’s perspective: all six findings are resolved, with zero active findings. Based on the reported CI results for |
…-RATE CONN-DISC now drives DaqifiViewModel.DisconnectDeviceCommand rather than ConnectionManager.Instance.Disconnect, and asserts the unsubscribe and the SelectedDevice clearing that command also does. LOG-SESSION waits for the session's persisted SampleCount. RunHardwareSequence cleans up on the exception path. Setup runs inside the guard and truncates results.jsonl. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
closes #254
Not merging — for review.
This PR is net-additive, and that is the point. It does not make the codebase smaller or flatter. It buys durability for 413 lines of working system-test rig that existed only as untracked files in one worktree, where
git clean -fdxor removing that worktree would have destroyed it.Diff: +1449 / −7 across 8 files — 413 of those lines are the rescued source (
Program.cs380 +.csproj33), 885 are a machine-generated lock file, 125 are the new README, and +26 / −7 is the integration into four existing files.What is preserved
tools/system-test/HeadlessBench/—Program.cs(380 lines),HeadlessBench.csproj(33),packages.lock.json(885, machine-generated). Committed substantially as-is: no rewrite, no tidy-up. A rescue mixed with a refactor is unreviewable, and improvements are noted as follow-ups rather than smuggled in here.bin//obj/deliberately excluded (637 MB of build output).It is the only thing in the repo that exercises the device → Core → view-model → rendered-window path end to end. The 530-test unit suite covers none of it, and
AvaloniaCapturerenders screens but never connects to hardware.#254's Current state says the rig gained 9 more matrix rows (
DEV-TILE,CH-TILE,DEV-NAME,DEV-RATE,CH-DIO,CH-PWM,SD-LIST,DEV-NET,DEV-DEBUG) plus aDAQIFI_RESTORE_NAMErepair mode. None of them are in the source that reached me —grepfor all ten strings returns nothing, and the file's own header still says it covers the original seven. The line count agrees: 413 against the ticket's "~450".So the 9-row expansion was lost before this rescue could catch it, which is #254's own failure mode arriving one revision too late. Recorded in #260 rather than papered over, with the spec for re-adding them.
Preserving the 7-row version is still worth doing. It builds, and it runs (below).
The four decisions, and why
1. Not in
Daqifi.Avalonia.slnx. #254 asks for this, but it is also just the house convention: the slnx holds the five app/test projects and nothing else — notAvaloniaCapture, notWpfCapture, not the vendored OxyPlot. And slnx membership was never what gated anything, because CI builds by explicit csproj path, not by solution.2. But it IS in CI, as a compile-only gate. This is the half of "keep it out of the solution" that would otherwise leave it to bit-rot, and it is the decision with a real trade, so:
The worry — "CI will build it and it needs hardware" — does not apply. Compiling is not running. The rig only opens a serial port when given
--port; adotnet buildnever does. No runner needs a board.Why it earns its step. The rig reaches deep into internals —
DaqifiViewModel,ConnectionDialogViewModel.ConnectManualSerialCommand,LoggingManager.Subscribe,shell.Plotter.LoggedPoints— none of it public API, all of it moving. #245 and #246 both deletedDaqifiViewModelmembers after searching the repo for readers. A rig outside the repo is invisible to that search; it would have been broken silently and nobody would have known until the next bench session. Now it breaks the build instead. That is exactly the rationale already written into this workflow forAvaloniaCapture("a change that broke it landed green").It is added in three places, mirroring
AvaloniaCaptureline for line:Restore,Build, and the from-scratch lock-derivation check. Never run in CI — that needs a board, and the comment says so.3. The lock file is committed, and here is the proof rather than the assumption.
It is byte-identical to
tools/parity-audit/AvaloniaCapture/packages.lock.json—diffbetween them is empty. That is not a coincidence: identical TFM (net10.0), no RID, the same sixPackageReferences at the same versions, the same singleProjectReference. Identical restore inputs, identical output.That identity carries the ubuntu leg for free:
AvaloniaCapture's copy already passes CI's locked restore and the from-scratch-derivation check on ubuntu today, including the case-sensitivity trap from #106.Rehearsed locally on top of that:
dotnet msbuild ... -getProperty:RestoreLockedMode -p:ContinuousIntegrationBuild=truetrue— the gate arms for this projectobj/dotnet restore ... --force-evaluategit status --short -- '**/packages.lock.json'after every build, including the-r iossimulator-arm64oneNo
-rwas passed to any restore. The rig's csproj already carries theNU1004warning in its header; the new README repeats it.4. It was untracked, not ignored.
git check-ignore -von all three files exits 1 — nothing in any.gitignorematches. Nothing needed fixing, and nothing was changed;bin//obj/are already covered repo-wide.Documentation
New
tools/system-test/HeadlessBench/README.md— what it does, how to run it, the output layout, the two traps it encodes, and the two build constraints.tools/parity-audit/README.mdis untouched (it is a live merge-conflict hotspot between #256 and #258).Three consequential doc fixes my own change forced, not gold-plating:
Directory.Build.propssaid "Six of the seven projects carry lock files" — adding an eighth governed project makes that seven of eight..github/scripts/refresh_lock_files.shgains the project inPROJECTS, or its lock file is the one nothing ever refreshes — the exact staleness the script's own comment warns about..github/dependency-updates/README.mdenumerates the leaf projects twice; both now include it.It runs — against the real bench board
Board lock held; connect / stream 5 s / disconnect only. No firmware, WiFi, SD, reboot or power-cycle. The rig redirects
DAQIFI_DATA_DIRto<out>/appdataitself, and the run demonstrably built a whole app-data tree there (DB, config,Logs/,SentryCache/) — the real~/Library/Application Support/DAQiFiwas never opened.lsofon the port after exit: nothing.The one
[FAIL]is the board, not the app, and the rig is what proves it. Itsunexpectedprobe separates the layers:The board believes it sampled at 100 Hz; wall clock says it did not. That is the known fw-3.7.2 timebase behaviour — not filed, and not an app regression. Distinguishing "the device sampled slower" from "the app got the samples late" is the entire reason this rig exists, and it did it unprompted.
Also seen and deliberately not chased here — logged in #260: after a successful disconnect,
ConnectionStatusStringstill reads'Connected'while both device collections are empty.Verification
dotnet build Daqifi.Avalonia.Desktopdotnet build Daqifi.Avalonia.iOS -c Release -r iossimulator-arm64 --no-restoredotnet test Daqifi.Avalonia.Testspython3 .github/scripts/test_refresh_lock_files.pyBoth warning figures are MSBuild all-project totals, ~20 of which are
third_partyAVP1002.Warning delta on both gate heads: zero — by construction, not by luck. The only change to any file MSBuild reads is inside an XML comment in
Directory.Build.props. No.cs,.axaml,.csprojor property that Desktop, iOS or Tests compile is touched, and the new project is in neither head's graph, so their inputs are byte-identical tomain's.Disclosed: the new project does add one first-party warning to its own compile —
Program.cs(346,13) CS0618: 'Bitmap.Save(Stream, int?)' is obsolete.AvaloniaCapturealready emits that same warning twice from the identicalopaque.Save(stream)call, so the CI job compiling both goes from 2 to 3. Left alone on purpose: fixing it is an improvement to the rig, which this PR is not for; it should be fixed in both files together.The Android head is not buildable on this Mac (workload absent) — CI covers it.
Review outcome
Qodo round 1 raised 6 bugs, all against the rescued
Program.cs. Round 2 on the same head: 0. Every thread answered and resolved.One was rebutted — "rendered UI is never asserted". The PNG is evidence, not the assertion; each
Step's UI half is the bound view-model state (ai.IsActive+LoggingManager.SubscribedChannels,shell.Plotter.LoggedPoints,ConnectedDevices), none of which a failed render makes pass. Pixel assertions areAvaloniaCapture's job, and Qodo's own precedent search cited #16 rejecting the same thing as non-fatal.Five were conceded, and the strongest deserves naming:
CONN-DISCcallsConnectionManager.Instance.Disconnectwhile a user's click runsDaqifiViewModel.DisconnectDeviceCommand, which also unsubscribes active channels fromLoggingManager, removes the firmware notification and clearsSelectedDevice. The row can pass while that cleanup is broken — a genuine asymmetry, since the connect side does go through the dialog's command. Verified against the source, not conceded on trust.Program.csis still byte-for-byte what was recovered. That is the deliberate line: this PR's review anchor is that the rescued file is unmodified, and the original is no longer reachable to diff a changed version against. So the conceded fixes went to #260, and — because a silent gap is the real hazard — all five are now written into the rig's README under "Known gaps — read before trusting a green run". A green run can no longer be over-read.The one thing that was fixed here is the part I actually authored: the README's run example used a fixed
--outpath whileresults.jsonlis append-only, which is what made stale-result accumulation reproducible. The example now stamps the directory, with an explicit warning.Also corrected, prompted by chasing that disconnect finding: I had filed
ConnectionStatusStringstill reading'Connected'after disconnect as a possible app bug. It is not one —ConnectionManager.ConnectionStatusis documented as the status of the most recent Connect call, explicitly "Not a per-device result" (IsDisconnectedis the connectivity signal; misreading it was #212). Withdrawn on #260.Merge note
#256 also touches
.github/workflows/build.yml, but only a "24 screens" → "25 screens" comment at line 160; this PR inserts a comment block and two build lines below it. Different lines, so git should merge them, but whoever lands second should glance at it. Neither #255, #256 nor #258 touchestools/system-test.