Repository navigation
test(runner): record portable suite observations - #46
Open
johnny4young wants to merge 3 commits into
Open
johnny4young wants to merge 3 commits into
johnny4young wants to merge 3 commits into
Conversation
johnny4young
marked this pull request as ready for review
October 6, 2026 03:50
The runner changes into the repository root before writing the summary, so a relative --summary path landed inside the checkout (leaving an untracked file) instead of beside the caller. Anchor it to the caller's directory first, keeping absolute POSIX and Windows drive paths as is. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ExmnfUngrDYjAheGvYwMnn
johnny4young
commented
Oct 6, 2026
johnny4young
left a comment
Owner
Author
There was a problem hiding this comment.
Review: runner records portable suite observations
What it does: scripts/run-tests.bash --summary PATH writes an opt-in JSON report. Per suite it records status, whole-second duration, exit status and reason, plus host/target OS, Bash version and HEAD/dirty. Quoting is pure Bash. The file is written atomically via mktemp + mv, and missing or corrupt status/duration/log evidence fails closed. Discovery switches to git ls-files -z. CI writes one summary per interpreter and uploads them with if: always().
Verdict: ready after the fix I pushed.
Fixed (pushed 386ddb2)
- 🟠
scripts/run-tests.bash: the runnercds to the repository root before writing the summary. A relative--summarypath was therefore resolved against the repo, not the caller. Repro:cd /tmp/x && …/scripts/run-tests.bash --summary out.json changelogwrote<repo>/out.jsonand left an untracked file in the checkout (which then also flips"dirty"only if tracked, so it silently pollutesgit status). The fix anchors a relative path to$PWDbefore thecdand leaves/…and WindowsC:/…/C:\…paths alone. A regression test was added intests/runner.bash; it fails without the fix.- Validation: full
scripts/run-tests.bash --jobs 4 --summary …as a non-root user gave 31/31 passed. ShellCheck 0.11.0, shfmt 3.13.1 (-i 2 -ci -bn),bash -nandtests/workflows.bashwere clean. #47 still merges cleanly on top, and itstests/runner.bashpasses.
- Validation: full
Checked and fine
json_string: verified with UTF-8 (café,日本),",\, TAB and\x01under bothCandC.UTF-8. Python'sjson.loadsround-trips it exactly.local LC_ALL=Cmakes the walk byte-wise, so multibyte sequences are re-emitted intact.record_suiteruns only in the parent shell (skip loop over<<<"$selected", andreport_suiteafterwait), so the arrays aren't lost in subshells.SECONDSworks in the backgroundedrun_suitesubshells."jobs":%sis always a validated integer by the time it is printed.git ls-files -z+read -d ''is bash 3.2-safe and fixes quoted non-ASCII paths. Theprintf '%s\0' tests/*.bashfallback keeps exported tarballs working.
Nits
- 🟡
write_summary:dirtyusesgit diff --quiet HEAD --, which ignores untracked files. Untracked suites can't run (discovery isls-files), but untracked fixtures or helpers can still change results.git status --porcelain --untracked-files=normalwould be stricter if this field is meant for comparing runs. - 🟡 Each
json_stringcall forks nothing but loops per character in Bash, which is fine at the current size. Just don't reuse it for log bodies. - 🟡 This PR and #45 both append to the same spots in
CHANGELOG.md/CONTRIBUTING.md, so the second to merge needs a trivial conflict resolution.
Generated by Claude Code
This was referenced Oct 6, 2026
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.
Summary
The portable runner executes fixed, Bash 3.2-compatible waves, but it records no per-suite timing evidence. This PR adds an opt-in
--summary PATHJSON report and has CI keep one per interpreter. The scheduler and the pass criteria are unchanged.What changed
scripts/run-tests.bash --summary PATHwrites:passed/failed/skipped(OS rule), whole-second duration, child exit status and reason;$BASH_VERSION, source HEAD (nullfor exported trees),dirtyandjobs.mktemp+mv) even when a child fails. Missing or corrupt status, duration or log evidence fails closed and is recorded with a reason.LC_ALL=C.git ls-files -z, so quoted non-ASCII paths are handled. Exported tarballs fall back toprintf '%s\0' tests/*.bash.test-summary-current.jsonand, on macOS,test-summary-bash32.json, and uploads them withif: always()assuite-observations-<os>.CONTRIBUTING.md,CHANGELOG.md, and new invariants intests/runner.bashandtests/workflows.bash.Why
Before proposing any change to the wave scheduler, we need matching per-suite observations, from the same source, fixtures, runner and interpreter class. These are coarse measurements, not a claimed saving.
How it was tested
json_stringround-trips UTF-8 (café,日本),",\, TAB and\x01exactly through Python'sjson.loads, under bothCandC.UTF-8;386ddb2, as a non-root user, with--summary: 31/31 passed. The generated JSON parsed, with head386ddb2,dirty: falseand 31 suites.-i 2 -ci -bn),bash -nandtests/workflows.bashare clean.386ddb2: all 10 jobs and all 11 checks green, including native macOS Bash 3.2 and Windows Bash/PowerShell. Previous qualification: run 37375586671 on4f03846.Review follow-ups
386ddb2fix(runner): resolve a relative--summarypath against the caller. The runnercds to the repository root before writing, so--summary out.jsonused to land in the checkout (as an untracked file) instead of next to the caller. A relative path is now anchored to$PWDbefore thecd;/…and WindowsC:/…/C:\…paths are kept as given. The new regression intests/runner.bashfails without the fix.Decisions taken
SECONDSresolution, to stay portable to Bash 3.2. No sub-second clock dependency was added.Known limitations & follow-ups
dirtyusesgit diff --quiet HEAD --, which ignores untracked files. Untracked suites can't run anyway (discovery usesls-files), but untracked fixtures could still change a result.Merge order
#46 → #47, then #45. #47 is stacked on this branch: merge this first, then retarget #47 to
main.CHANGELOG.mdandCONTRIBUTING.md(and touchesci.ymlandtests/workflows.bashin other hunks).git merge-treeshows only text conflicts in those two docs. Whichever lands later resolves them at merge time by keeping both entries.