Group CI logs and surface failures on the default (non-bucketed) test run#14715
Open
cderv wants to merge 12 commits into
Open
Group CI logs and surface failures on the default (non-bucketed) test run#14715cderv wants to merge 12 commits into
cderv wants to merge 12 commits into
Conversation
Plans GitHub Actions log grouping for the default (non-bucketed) full test run, plus ::error annotations and GITHUB_STEP_SUMMARY failure tables. Documents runner constraints (no nested groups, annotation caps, summary size limits), spike-verified marker placement in Deno's test output framing, phased implementation, invariants, and a verification plan. No behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WFvZcK71eJ3gmmDT6RcFDd
…ping design The gate itself is applied on the built-version testing branch; this only retains the design doc's pointer to the follow-up plan that documents how the gate is eventually removed.
Review findings addressed: - Annotation cap was defined per process but GitHub's limit is per step; in bucket mode many run-tests.sh processes share one step. The harness now emits annotations only when it owns the step (default path), gated by QUARTO_TESTS_GHA_ORCHESTRATED (replacing QUARTO_TESTS_NO_LOG_GROUP, which also covers grouping); bucket mode keeps the YAML per-file errors, and the step summary is documented as the complete record in all modes. - Step-summary size budget is now stat-based on the shared summary file so it holds across processes within a step. - The lazy-vs-eager group closure choice is now explicit (lazy chosen), invariant 3 is scoped to harness-registered tests, and the eager fallback is defined next to the policy instead of as an aside. - The julia-engine env-strip-list sync gains a mechanical drift check: a quarto-cli unit test comparing kStripEnvVars with the subtree helper after the pull, added to the follow-up procedure and acceptance. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WFvZcK71eJ3gmmDT6RcFDd
The workflow-command toolkit in github.ts (error/escapeData/escapeProperty, group markers) was dead code for the test harness. Phase 1 of the CI test-log design (dev-docs/ci-test-log-grouping-design.md) needs a few more primitives so the harness can surface test failures as annotations and a step-summary table: - stripAnsi: annotation and step-summary text do not render ANSI, so color codes from captured output must be removed first. - harnessOwnsStep: the harness emits per-test annotations only when no outer orchestrator (the bucket-loop YAML) has claimed the step; parameterized so unit tests need not mutate process-global env. - AnnotationBudget: GitHub silently drops past 10 errors per step, so cap at 9 per-test annotations plus one aggregate. - stepSummary/stepSummarySize + a 512 KiB budget: content near the 1 MiB per-step limit can be silently dropped, and bucket mode has many processes appending to one file, so the file size is the cross-process coordinator. - summary table/detail formatters: <pre> with HTML-escaped bodies so hostile test output cannot break out of the markdown table or a code fence. All gate on GITHUB_ACTIONS or take an injectable path, so nothing changes off CI. Unit tests cover the cap/aggregate counter, the ownership gate, ANSI stripping, the stat-based budget including the degrade path, and escaping of hostile test names.
Phase 1 of dev-docs/ci-test-log-grouping-design.md. On the default (non-bucketed) full run, one deno test process produces thousands of flat log lines with no ::error annotations and no step summary, so a failure is only findable by scrolling. The harness now, in its existing failure path and only when GITHUB_ACTIONS=true: - emits one ::error annotation per failed test (file + title + repro command and a 20-line ANSI-stripped excerpt), pointing at the smoke-all document when the failure is a doc test (the file a developer opens) rather than the harness .test.ts file; - appends a step-summary table row per failure — the complete failure record, emitted in every mode — with an expandable output block flushed after the rows (GFM ends a table at the first non-row line, so details cannot sit between rows) and degrading to name-only once the shared summary file crosses the size budget. Since one process is the whole step on this path, the module-level annotation budget is exactly a per-step budget. Annotations are gated on harness step ownership: the two bucket-loop steps in test-smokes.yml already emit their own per-file ::group and ::error, so they set QUARTO_TESTS_GHA_ORCHESTRATED and the harness suppresses annotations there (summary rows still emit). No ::group/::endgroup is emitted by the harness — that is Phase 2. Local output is byte-identical.
GFM ends a table at the first non-row line, so per-failure detail blocks cannot interleave with table rows; the implementation streams rows into one contiguous table and flushes buffered detail blocks plus the aggregate ::error at an unload listener. Update Phase 1 to describe the implemented shape and its accepted hard-crash caveat (rows are written at failure time, so the table stays the complete record). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WFvZcK71eJ3gmmDT6RcFDd
The bucketed CI runs got navigable logs by wrapping each file in a ::group:: at the workflow level (#13787/#13807), but the default non-bucketed run is one giant deno test whose thousands of flat lines have no structure. Have the harness itself open a group per test file so that path collapses the same per-test noise the bucket loop does. Grouping lives in a small state machine (gha-grouping.ts) so the single-open-group invariant — the GitHub runner cannot nest groups — is enforced and unit-testable in isolation. The harness opens/transitions the group at the start of each test's fn, closes it on the failure path before fail() throws (so the FAILED line and ::error land outside the collapsed group), and closes any open group at unload before the Phase 1 aggregate error and Deno's terminal ERRORS/FAILURES sections. Every emission is gated on harnessOwnsStep() (on CI, no outer orchestrator), the same ownership switch Phase 1 uses: local runs and orchestrated bucket runs stay byte-identical, so the #13807 bucket log format is untouched. Closure is lazy per file; direct Deno.test files are a documented exemption. Design and invariants: dev-docs/ci-test-log-grouping-design.md.
The per-file grouping depends on how Deno's test reporter frames output (markers replayed at column 0, unload flushing before the terminal sections) — behavior that can shift across Deno upgrades. This dev-only script reads a captured run log and mechanically asserts the invariants: no nested groups, markers at column 0, and no harness FAILED line or ERRORS/FAILURES header trapped inside a collapsed group. Run it after any Deno bump to catch reporter drift before it ships a broken log.
Status header now reflects the implemented state (Phase 3 optional, fork trial matrix still pending). The checker section documents that run-tests.ps1 must be invoked via pwsh -File with shell-level redirection: pwsh -Command with *> feeds the redirect into the script's arg parsing, blanking the file arguments and silently running the full suite. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WFvZcK71eJ3gmmDT6RcFDd
… annotation budget Review #1991: initDenoDom/prereq/chdir/setup ran after the per-file group opened but outside the guarded failure path, and teardown could throw after it — any of those failures left the FAILED result line inside a collapsed group. The whole test body is now wrapped in an outer catch that closes the group (idempotently) before rethrowing. Review #1986 (confirmed empirically on the pinned Deno 2.7.14): Deno instantiates each test file's module graph separately — module state resets per file and unload fires once per file — so the module-level annotation counter was a per-FILE budget, not per-step, and failures spanning >10 files would silently lose annotations. The budget now coordinates through a sidecar counter file derived from GITHUB_STEP_SUMMARY (unique per step; no locking needed as files run sequentially without --parallel), and the aggregate ::error is emitted inline by the failure that crosses the cap (a per-file unload cannot know it is last). The same finding corrects the spike interpretation: the inter-file ::endgroup:: was the previous file's own unload, not a cross-file transition — grouping output is unchanged, and foreign (non-harness) test files now provably run with no group open. Design doc updated throughout; unit tests cover the new decision API and cross-instance coordination through a shared counter file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WFvZcK71eJ3gmmDT6RcFDd
…tinel) The trial run (cderv/quarto-cli run 29767179626) failed 3 reporting unit tests that had passed locally: passing an explicit undefined for the injectable counter/summary/orchestrated parameters triggers the JS default parameter, which on CI resolves to the REAL per-step sidecar counter (already exhausted by earlier failures), the REAL GITHUB_STEP_SUMMARY file (the test appended to the run's summary), and the REAL QUARTO_TESTS_GHA_ORCHESTRATED variable (set in bucket steps, so the gate tests would flip once these files run inside a CI bucket). AnnotationBudget, stepSummary, stepSummarySize, and harnessOwnsStep now take an explicit null sentinel meaning 'no file / treat as unset', resolved with === undefined checks so null survives; unit tests pass null. Everything else the trial exercised behaved as designed: 9+1 annotations from the step-wide budget across 12 failing files, zero harness annotations in orchestrated bucket runs, grouping invariants checker green on real CI logs from both OSes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WFvZcK71eJ3gmmDT6RcFDd
Collaborator
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
The doc claimed every test-smokes-built.yml leg reaches the flat default path; only the smoke leg does (empty buckets). The playwright and ff-matrix legs pass non-empty buckets and take the already-grouped orchestrated loop, where the harness stays silent by design. Also flip the stale trial-has-not-run status, correct the file-origin mechanism to context.origin (not a captured stack), and fix two line/step-name citations (run-tests.sh deno test invocation, the Restore R packages grouping step).
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
3 participants
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.
Problem
The default (non-bucketed) test path runs one giant
deno testover every discovered file and emits no workflow commands, so the CI log is thousands of flat lines — finding a failure means scrolling or text search. This is the path the smoke leg oftest-smokes-built.ymlruns by default (emptybuckets); the playwright and ff-matrix legs pass a non-emptybuckets, so they already go through the per-file::group::bucket loop added by #13787 and #13807. Those PRs grouped only the bucket loops (one file perrun-tests.shinvocation) — the default path was left flat, and no per-test::errorannotation or step-summary table existed on either path.What it does
Per-test-file
::group::markers plus per-test::errorannotations on the default path, andGITHUB_STEP_SUMMARYfailure tables in every mode:QUARTO_TESTS_GHA_ORCHESTRATEDenv contract (set by the bucket loops) keeps the GHA TESTS - Improve logging to group in the right place #13807 bucket-log format byte-identical and doubles as a no-revert kill switch. Everything else gates onGITHUB_ACTIONS, so local output is byte-identical.tests/tools/check-gha-log.ts, a mechanical invariant checker to re-run on Deno version bumps (reporter framing is version-sensitive).Design of record (constraints, spike results, invariants, verification plan) lives in the branch at
dev-docs/ci-test-log-grouping-design.md.Evidence
Fork trial matrix with seeded failures, Linux + Windows:
Two real bugs the trial caught and fixed before this PR: Deno's per-file module instances invalidating a per-process budget, and explicit
undefinedargs triggering default parameters so unit tests touched the real CI counter/summary/env (null-sentinel fix).Notes
Note
Stacked on #14706; GitHub retargets this to
mainwhen it merges.Related but independent finding from the trial: #14711 (cold-cache renv deadlock).
A built-mode (source=build) confirmation run on this branch is linked in a follow-up comment.
AI-assisted PR