feat(ctest): add compact output filter - #2250
Conversation
3d3e3d5 to
3964bea
Compare
|
@KuSh @aeppling That benchmark calls Here is a local example showing two different response shapes: ❯ rtk curl -s https://mockhttp.org/json
{"message":"Hello from MockHTTP","timestamp":"2026-07-03T02:05:44.002Z","status":"success","code":200}
❯ curl -s https://mockhttp.org/json
{"metrics":{"cpu":45.2,"memory":78.5,"disk":62.3,"network":{"inbound":"1.2MB/s","outbound":"0.8MB/s"},"uptime":"15 days","services":[{"name":"api","status":"healthy"},{"name":"database","status":"healthy"},{"name":"cache","status":"warning"}]}}
A deterministic local HTTP fixture, or capturing one response and feeding the same body to both commands, should make this benchmark stable. |
3964bea to
6e2a53a
Compare
KuSh
left a comment
There was a problem hiding this comment.
Thanks for the update — I re-checked all five threads from my last review and they're genuinely addressed:
- the
.claude/changes are gone pipe_cmddetection is now gated onlooks_like_ctest_output()(first non-empty lineTest projectand a strict result-line match), with a negative test for keyword-only output- regexes are compiled lazily
collect_failure_blockno longer re-emits the raw dotted-progress line, and there's a fullassert_eq!covering the whole output- Timeout / Not Run / Exception / Not Run (Disabled) all have coverage now
Please rebase onto current develop before anything else. The branch's merge base is 66e09cb; develop has moved on since your 2026-07-21 push. GitHub still reports this mergeable with all 11 checks green, but both are measuring the July snapshot — I merged current develop into the head locally and cargo check --all-targets fails with 8 errors. Anchored inline below.
Sorry about the lazy_static! one in particular: my earlier request was correct when I made it, but develop migrated to LazyLock in 5269df7 a week after you pushed. That one's on the repo, not on you.
After the rebase there are two behavioural bugs worth fixing. I ran the filter against real ctest 4.3.4 output from a scratch CMake project rather than against the fixtures, and both reproduce — also inline.
KuSh
left a comment
There was a problem hiding this comment.
Marking this as changes-requested to reflect the blocking items in my review just above — the two rebase-breakers (lazy_static removed from develop in 5269df7, and the new pipeline_final_safe field on RtkRule) and the two behavioural bugs (***Failed result lines wrapped across a newline are never parsed, and ***Skipped is counted as a failure).
No new findings here; details and code anchors are in the inline comments. Happy to re-review as soon as it's rebased onto develop.
6e2a53a to
caaa674
Compare
KuSh
left a comment
There was a problem hiding this comment.
Round 2 is a real improvement — thanks for the thorough turnaround. I verified every one of my six previous points by building the rebased branch and running the filter, not just reading the diff: fmt and clippy --all-targets clean, 2673 tests pass, and the wrapped-Regex=[…], ***Skipped, and discovery-failure cases all now produce correct output. All six threads are resolved. Adopting ..RtkRule::DEFAULT and reusing CAP_WARNINGS/CAP_LIST was the right call.
I then did a deeper pass against real ctest 4.3.4 output — a scratch CMake project exercising serial, -j4, --repeat until-pass:2, green, no-tests, -N and --version runs. Seven new findings, all reproduced against real output rather than fixtures.
Blocking (1 high + 3 medium): the --repeat until-pass misreport, the -j block-attribution bug, unlabeled detail blocks, and the uncapped failed: list.
Non-blocking (3 low): uncapped skipped: list, tail-only truncation of pre-result diagnostics, and fixture realism. Happy for these to land as follow-ups — flagging them so they're on record, not to hold the PR.
The common thread in the blocking four: parallel and retry modes reshape CTest's line stream in ways the hand-written fixtures don't reproduce. Capturing a couple of real -j4 and --repeat runs as fixtures would likely catch all of them at once.
KuSh
left a comment
There was a problem hiding this comment.
Round 3 verified — thanks for the thorough turnaround, and for the rebase.
I checked the seven open items against the new head rather than the resolved flags, and all seven are genuinely fixed; every thread is now resolved. The branch rebases onto current develop (322db4d) and cargo check --all-targets is clean, so the two build-blockers from round 1 are actually gone rather than just invisible to CI. cargo fmt, cargo clippy --all-targets and 2678 tests all pass.
I did not take the new unit tests on trust — ctest 4.3.4 is installed here, so I generated real output and ran the built binary against it:
--repeat until-pass— a real flaky-then-passing run now yieldsctest: 2/2 passedwith nofailed:list. You fixed this at both layers (summary-based routing at:148and dedup-by-test-number inparse_tests), which is belt-and-braces and appreciated.-jattribution — a realctest -j4with three failures and no interveningStartlines attributes every block correctly.- caps, labels, head+tail truncation, fixture ordering — all confirmed.
Special credit for ctest_repeat_until_pass_raw.txt: it is byte-faithful to real ctest, including the retry line's missing N/M prefix and its four-space indent. That is exactly the detail that broke the old regex, and capturing it precisely is what made this round verifiable.
I also probed two things I thought might still be wrong and they turned out not to be defects — noting them so they don't get "fixed" later: a Start number that differs from the completing test's number is correct attribution (ctest prints Start on slot acquisition and output on completion), and the widened TEST_RE does not false-positive on test stdout because it stays ^-anchored.
Two items left, both in the failure-detail path.
One blocking (:479): a test that prints a CTest-shaped line truncates its own failure block and deletes the assertion evidence, with no marker of any kind.
One non-blocking but strongly wanted (:500): the per-test tee write is the wrong shape, and consolidating it fixes two defects at once while bringing the filter in line with what every other rtk filter does.
On the second one — while investigating it I found a pre-existing rtk bug in core/tee.rs (the {epoch_seconds}_{slug}.log filename collides with itself), which is not yours to fix. It is tracked at #3431 and I have added the single-process reproduction there. I mention it only because it is the visible symptom that shows why the per-test write shape is worth changing, and the consolidation sidesteps it without waiting on that issue.
- migrate regex statics to std::sync::LazyLock (lazy_static left develop in 5269df7) - use ..RtkRule::DEFAULT for the ctest rule (new pipeline_final_safe field) - join wrapped `***Failed <reason>` result lines before parsing and surface the reason - give ***Skipped tests their own bucket instead of reporting them as failures - report "no tests found" only for the literal `No tests were found!!!` line when nothing parsed - cap per-failure output at CAP_WARNINGS (tail) and the FAILED list at CAP_LIST, with tee hints - add real ctest 3.25.2 fixtures with exact-output and >=60% savings tests Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- summary line is authoritative for pass/fail routing and counts; a parsed failure no longer overrides "0 tests failed" (--repeat until-pass) - recognise retry result lines without the N/M prefix; the last result per test number replaces the earlier one in place - pre-result diagnostics are only claimed when the nearest boundary is a Start line, so -j runs no longer absorb other tests' result lines - failed entries are labeled "#N name (status, duration)" with their details indented beneath; empty blocks can no longer shift attribution - failed: and skipped: lists are capped at CAP_LIST with tee recovery; the duplicated CTest FAILED name list is dropped, its trailer lines are kept - failure detail keeps 2 head lines + 8 tail lines so pre-result diagnostics such as "Could not find executable" survive truncation - fixtures re-captured as stdout followed by stderr, matching runner.rs; add real -j4 and --repeat until-pass:2 captures Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review round 4 of rtk-ai#2250. - Failure blocks are bounded by validated CTest framing instead of any line shaped like one. `Start N: name` counts only when a parsed test has that number and name; the summary, total-time and "The following tests FAILED:" lines count only at their last occurrence. A test that prints ` Start 3: inner phase` or a nested summary keeps its assertion evidence, and the outer summary stays authoritative. Real capture: tests/fixtures/ctest_spoofed_framing_raw.txt. - Collection and rendering of failure blocks are split. Blocks are collected untruncated for every failed test, the section is rendered with the existing entry and head+tail caps, and one section-level `ctest-failed` tee holds the complete untruncated section whenever a block or the list was cut. The per-test `ctest-failure` writes are gone, so the hint no longer points at a colliding file (rtk-ai#3431) and the `[full output:]` file contains no `... +N more lines` markers. - The `***Failed <reason>` line stays outside the per-block cap.
f9f7b38 to
6cf56a9
Compare
KuSh
left a comment
There was a problem hiding this comment.
Round 4 verified — both previous items are genuinely fixed, and the approach you took on the framing one is better than what I suggested.
Rather than patching the boundary check, collect_framing_line_indices validates framing up front: a Start N: name counts only when a parsed test has that number and name, and the summary / total-time / FAILED: markers are taken with rposition so only the last occurrence frames. My original real-ctest repro now keeps its assertion lines, and ctest_spoofed_framing_raw.txt covers both vectors including a spoofed 100% tests passed inside a test's output.
The tee consolidation is exactly the shape we discussed — collect_failure_block is pure, render_failure_block truncates only at render time, and build_failed_section collects every block untruncated so one tee holds the whole section. Measured against the same fixtures I used last round:
| round 3 | round 4 | |
|---|---|---|
| tee files, 2 noisy failures | 2 writes, 1 colliding file | 1 file |
| its content | 40 ZZZ, 0 AAA | 40 AAA + 40 ZZZ |
| hidden failures' evidence (25-failure run) | 10 of 30 each | 30 of 30 each |
... more lines markers inside "full output" |
25 | 0 |
No regressions: real --repeat until-pass still reports 2/2 passed, real ctest -j4 attribution is still correct, green/skipped runs unchanged. Rebased cleanly onto current develop (d9a5893), cargo check --all-targets clean, 2681 tests + clippy + fmt green. Both threads resolved.
Two items this round, both found by probing the areas the refactor touched.
Blocking (:241) — result lines are now the one framing/parsing category left unvalidated, and that single gap produces three distinct symptoms, one of which erases a failure entirely.
Non-blocking (:542) — folded multi-line results render flush-left and evade the line cap.
A note on process, since this is the fourth round: the blocking item is a consequence of the round-3 dedup fix meeting the round-4 framing work, not a regression you introduced carelessly. Each round has been correct about the case it targeted. The suggestion at the end of the blocking comment is a two-line change I verified against real ctest, so this should be a short round.
…odes Review round 5 of rtk-ai#2250. - Result lines are trusted only when their `N/M` total matches the run. The run total is the final summary total, reconciled for disabled tests (ctest counts them in `N/M` but not in the summary), or the most frequent counter when no summary exists. A test that forwards a nested ctest run can no longer replace a real record, truncate its own block or inflate the totals; dedup keys on number and name so `--repeat` retries still collapse. Framing uses the retained result lines only. Real capture: tests/fixtures/ctest_nested_result_raw.txt. - Dashboard client modes (`-D`, `-D<x>`, `-M`, `-T`, `-S`, `-SP`, `--build-and-test`) bypass the filter like `-V` does, so diagnostics such as a missing DartConfiguration.tcl are shown verbatim. - Folded result-shaped logical lines are split back into physical lines inside failure blocks, so each line is indented and counted against the block cap.
KuSh
left a comment
There was a problem hiding this comment.
Round 5 verified — all four items from the last round are genuinely fixed, and the N/M validation went further than I sketched.
Confirmed against real ctest 4.3.4, not just the fixtures: the nested-result spoof now keeps #1 wrapper (Failed) and its ASSERT FAILED line, with the forwarded line correctly demoted from framing to content; --test-action's short form -T passes through; and the folded multi-line block is indented per physical line and counted per physical line. --repeat until-pass, -j4 attribution, the tee consolidation (1 file, 40+40, 0 truncation markers) and the parallel fixture's counter-vs-summary disagreement all still behave. 2766 tests, clippy and fmt clean on current develop. Both threads resolved.
counter_total_with_disabled_tests deserves a specific mention: reconciling CTest's counter (which includes disabled tests) against the summary total (which excludes them) is a subtlety I had only noticed in passing, and handling it explicitly is what keeps the parallel fixture correct.
That same reconciliation is where this round's blocking item comes from, and I want to be straight about the shape of it: --stop-on-failure worked in round 4 and does not work now. It is a genuine regression from the fix, not something that was always broken — I rebuilt the round-4 binary to confirm. The second blocking item is older than this round; the third is the one that would have contained both.
Five items: three blocking, two not.
:325—--stop-on-failureerases every failure detail (regression).:218— a result line wrapped past 8 physical lines drops a test and corrupts the previous one (pre-existing).:492— no fallback when the summary reports failures but nothing parsed. This is the structural one.:112— only the short dashboard flags pass through (non-blocking).:301— a killed run plus forwarded results reports green (non-blocking).
A note on the arc, since this is round five. Rounds 3, 4 and 5 each fixed their target correctly and each opened a new interaction, and that is not carelessness — it is the design. Output is reconstructed from parsed records and everything unparsed is discarded, so every gap in parsing becomes silent, total loss of the actionable content rather than degraded output. :492 is the one change here that breaks that pattern instead of patching another instance of it, and I would take it even if you disagree with my read of the other two.
…parsed failures Review round 6 of rtk-ai#2250. - The run total used to validate result lines is the counter of the first result line that carries one. ctest prints its own result line before any output a test could forward, so that total identifies the run under `--stop-on-failure` (scheduled > ran), with disabled tests, under `-j`, and in a killed run whose test forwards a larger nested suite. The summary-based reconciliation and the majority vote are gone; `--stop-on-failure` details are back. Real capture: tests/fixtures/ctest_stop_on_failure_raw.txt. - Wrapped result lines are folded until their terminator instead of giving up after eight physical lines, so a long `PASS_REGULAR_EXPRESSION` list no longer drops the test and swallows its neighbour's block. Real capture: tests/fixtures/ctest_long_regex_list_raw.txt. - When the summary reports more failures than were parsed, the raw `The following tests FAILED:` entries are rendered under `failed (unparsed, raw):` (capped at CAP_LIST with a tee tail hint), so a parsing gap degrades the output instead of erasing the names. - Dashboard long flags (`--dashboard`, `--test-model`, `--test-action`, `--script`, `--script-new-process`) bypass the filter like the short forms.
…ize the caps
Seven limits, each reached with real ctest 4.3.4 output.
Result lines are validated against the run total, but the summary was not, and
`has_failures` trusts a summary over the parsed records. A test that forwards a
nested ctest run echoes that run's summary too, so a run killed before printing
its own reported the nested one: a SIGKILLed suite of 6 whose first test had
already failed rendered "ctest: 3/3 passed" off the forwarded 3-test summary.
CTest prints its summary once every test has finished, so a validated Start
behind a summary places that summary inside a test's output -- that identifies a
forwarded suite whatever its size, where a size bound alone only catches a
nested suite larger than the run. Two bounds cover a kill landing between the
forwarded summary and the next Start: the run total caps a genuine total, and a
summary cannot report fewer failures than the result lines already validated.
split_status_reason undid the fold's newline before a closing bracket with a
global replace of " ]", which also rewrites brackets belonging to the reason.
A real PASS_REGULAR_EXPRESSION of "zzz[0-9 ]end" rendered as Regex=[zzz[0-9]end]:
a character class silently loses the space it matches on, and the reader debugs
a pattern ctest never ran. Only the fold's own trailing " ]" is undone now.
--no-tests=error exits 8 and says "Errors while running CTest", but the empty
run rendered as a bare "ctest: no tests found" -- the one line that reads as a
benign outcome for a run that failed. The lines behind the no-tests marker are
kept, as they already are behind a failure.
-T Test is the canonical CI invocation and prints ordinary test output, yet it
bypassed the filter along with every other dashboard mode, so the most common
form saved nothing. The action decides now: Test stays filtered, every other
action still bypasses (-T Coverage opens with "Performing coverage"), and a
test model with no action to pair it with is left alone rather than guessed at.
looks_like_ctest_output required "Test project " to be the first non-empty line,
tolerating one specific preamble. `ctest -T Test` emits a repeated "Cannot find
file: .../DartConfiguration.tcl" ahead of the banner, so `ctest -T Test 2>&1 |
rtk pipe` fell through to identity. The banner is looked up within a short
leading window; a result or no-tests line must still follow it, which is what
keeps unrelated output out.
The failed list capped entries at the flat CAP_LIST while each entry renders a
header plus up to MAX_FAILURE_LINES detail lines, which is the multi-line case
core/README.md ("Deviating from a cap") reserves reduced() for. A 25-failure run
with 30 diagnostic lines apiece rendered 247 lines, 67.9% off the raw output --
above the floor but not compact. At reduced(CAP_LIST, 5) it renders 187 lines,
75.8% off, and the section tee still recovers every entry. The skipped and
raw-trailer lists keep the full cap: their entries are one line each.
Tracking bucketed ctest under "system" beside ls and grep, while discover
classifies it as a test runner; every other runner is bucketed by ecosystem.
KuSh
left a comment
There was a problem hiding this comment.
Approving.
Round 6 landed findings 1–4, and I checked each against real ctest 4.3.4 rather than the fixtures: --stop-on-failure details are back, a 15-alternative PASS_REGULAR_EXPRESSION no longer drops its test at eight physical lines or swallows the next block, the raw FAILED: trailer contains a parsing gap instead of erasing it, and --test-action matches -T.
Rather than send a seventh round for what was left, I pushed 143dc8c to this branch — please git pull before you touch it again.
Finding 5, the one still open
Result lines are validated against the run total; the summary is not, and has_failures trusts a summary over the parsed records. A test that forwards a nested ctest run echoes that run's summary too, so a run killed before printing its own reports the nested one. A SIGKILLed 6-test suite whose first test had already failed rendered ctest: 3/3 passed. first_counter_total_rejects_forwarded_suite_in_killed_run passes because it omits the nested summary — real forwarded output always ends with one.
The rule that holds is ordering, not size: ctest prints its summary once every test has finished, so a validated Start behind a summary places that summary inside a test's output. I first tried bounding the total by run_total, and that only catches nested suites larger than the run; tests/fixtures/ctest_killed_smaller_forwarded_suite_raw.txt is the real capture that defeats it, kept as a fixture so the weaker version can't come back. The two numeric bounds are still there as a backstop for a kill landing between the forwarded summary and the next Start.
Four more, each reproduced with real ctest
split_status_reason's global.replace(" ]", "]")rewrote brackets belonging to the reason.PASS_REGULAR_EXPRESSION "zzz[0-9 ]end"rendered asRegex=[zzz[0-9]end]— the character class quietly loses the space it matches on, and you debug a pattern ctest never ran. Only the fold's own trailing" ]"is undone now.--no-tests=errorexits 8 and saysErrors while running CTest, but the run rendered as a barectest: no tests found: the one line that reads benign for a run that failed. The trailer is kept, as it already is behind a failure.-T Testis the canonical CI invocation and prints ordinary test output, yet it bypassed the filter with every other dashboard mode, so the most common form saved nothing. The action decides now —Teststays filtered, everything else still bypasses (-T Coverageopens withPerforming coverage), and a-Mwith no action to pair it with is left alone.looks_like_ctest_outputrequiredTest projectto be the first non-empty line, but-T Testemits a repeatedCannot find file: …/DartConfiguration.tclahead of it, soctest -T Test 2>&1 | rtk pipefell through to identity.
Two housekeeping items came along: the failed list now deviates to reduced(CAP_LIST, 5) because its entries are multi-line, which is the case core/README.md reserves reduced() for (25 noisy failures went 247 → 187 lines, 67.9% → 75.8% off the raw output), and tracking moved ctest off system so rtk gain stops filing C/C++ test runs beside ls and grep.
cargo fmt --all && cargo clippy --all-targets && cargo test --all is clean, 2701 tests pass.
Thanks for staying with this through six rounds. The framing validation from round 4 and the fold-until-terminator plus raw-trailer fallback from round 6 were the hard parts, and they're what made the rest tractable — every one of the remaining items was a single unvalidated input, not a structural problem.
Summary
rtk ctestwith compact CTest output for green runs and preserved failing-test details.ctestin hook rewrite/discover, pipe filtering, tracking, and TOML shadow warnings.Closes #2237
Test plan
cargo fmt -- --checkcargo test ctestcargo testcargo clippy -- -D warningscargo run --quiet -- rewrite "ctest -R smoke --output-on-failure"cargo run --quiet -- pipe --filter ctestwith sample CTest output