diff --git a/.github/scripts/shard-tests.sh b/.github/scripts/shard-tests.sh index 4abc1a79c..13db2f598 100755 --- a/.github/scripts/shard-tests.sh +++ b/.github/scripts/shard-tests.sh @@ -33,9 +33,76 @@ # # Whatever the weights, the property that matters is coverage, not balance: the # worst case of a bad weight is an unevenly filled shard, never a missing file. - +# +# --- Pinned-apart files (#847, #848) --------------------------------------- +# +# @test count is a loose proxy for runtime in general (above), but for a +# specific shape of file it is not loose, it is blind: a file whose cost is +# almost entirely waiting (background processes, poll loops with +# hundred-plus-iteration bounds) rather than how many @test blocks it +# contains can carry a tiny weight here while dominating its shard's actual +# wall clock. A file that is merely large -- many @test blocks, ordinary +# per-test cost -- is NOT this case; count already weights it correctly, and +# it is not pinned. +# +# Measured 2026-08-19 on a green main run (head 626a625b, run 32193147987) by +# correlating each `ok N ` line's own GitHub Actions timestamp against +# which file's `@test` block that description belongs to, then ranking every +# file by seconds-per-test rather than by raw duration (raw duration alone +# does not distinguish "slow because few tests wait a long time" from "slow +# because there are simply many tests", and only the former is what count +# weighting misses): +# +# tests/test_remote_engine_start_refusal.bats 722s / 9 tests = ~80s/test +# tests/test_remote_status_liveness.bats 380s / 31 tests = ~12s/test +# +# against a whole-suite per-test cost this script's own header already says +# runs ~0.0s-8s. Both are 1.5x-10x above that ceiling on a low test count, so +# both rank near the bottom of the count-weighted sort while carrying some of +# the largest absolute durations in the suite. (Files that are merely large in +# absolute terms -- e.g. a 179-test file at a very ordinary ~1.2s/test -- were +# checked and excluded: their weight already reflects their real cost.) +# +# #847's own trigger was exactly this class of file landing next to another +# heavy one purely because an unrelated 15-test addition elsewhere repacked +# the partition — the count weight cannot tell "heavy because slow" from +# "heavy because voluminous", so nothing stops two slow-but-few-tests files +# from drifting onto the same shard as the tree changes shape. Pinning these +# apart, in fixed shard slots decided before the ordinary weighted pass runs, +# means no future change to any OTHER file's test count can put two of them +# together again — that was the actual, demonstrated failure, not merely a +# theoretical one. +# +# This does not bound a shard's total duration: the heavier entry above +# (722s) is heavy enough on its own that no repacking of the rest of the +# suite moves its shard's floor by much. See tests.yml's bats-shard +# timeout-minutes for the ceiling this is paired with, sized to cover that +# floor plus a fair share of everything else with real margin. And a file NOT +# on this list can still turn out to be similarly disproportionate and land +# next to another one by chance — nothing here detects that case in general, +# only these two measured instances of it. Revisit alongside the counting +# scheme itself once the concurrent effort to shorten these files (tracked +# separately from #847/#848) lands and the numbers above are stale. +# +# FOLLOW-UP: whichever of that effort's PRs (#876 et al.) touches either file +# named below changes its real cost, possibly enough to make pinning it +# pointless or to make some other, currently-unremarkable file the next +# hidden outlier. Re-run this script's own measurement method (correlate a +# green run's `ok N` timestamps against each file, rank by seconds-per-test) +# on main once that work lands, and drop or replace entries here based on +# what it says then — this list is not meant to be permanent. The 30-minute +# job cap in tests.yml is a separate decision and does not depend on this one. +# +# Matched by basename, not by the `$dir`-relative path `files` below uses, so +# the pin still resolves when this script is invoked against a different +# tests-dir (as several of this file's own tests do). A pinned name that no +# longer exists in the tree (renamed, removed) is silently skipped rather +# than treated as an error: the partition's correctness (full coverage, +# asserted by tests/test_ci_sharding.bats) never depended on it. set -euo pipefail +PINNED_APART="test_remote_engine_start_refusal.bats test_remote_status_liveness.bats" + usage() { echo "usage: ${0##*/} [tests-dir]" >&2 echo " shard-index is 1-based and must be <= shard-total" >&2 @@ -62,12 +129,56 @@ files="$(find "$dir" -maxdepth 1 -name '*.bats' | LC_ALL=C sort)" # Weight each file by its number of test cases. `grep -c` exits 1 on no match # after printing 0, which set -e would otherwise treat as fatal. +file_weight() { + local n + n="$(grep -c '^[[:space:]]*@test' "$1" || true)" + [ -n "$n" ] || n=0 + printf '%s' "$n" +} + +# Seed the pinned files into distinct shards first, in PINNED_APART's own +# (measured-heaviest-first) order — not the order they happen to sort in +# below, which is by count and is exactly the metric these files defeat. Each +# consumes one shard slot (wrapping if there are more pinned files than +# shards); everything else is decided by the ordinary weighted pass afterward, +# which never reconsiders a file placed here. +i=0 +while [ "$i" -lt "$total" ]; do + load[i]=0 + i=$((i + 1)) +done + +pinned_paths=" " +slot=0 +for p in $PINNED_APART; do + match="" + while IFS= read -r f; do + [ -n "$f" ] || continue + if [ "$(basename "$f")" = "$p" ]; then + match="$f" + break + fi + done <= 2 (#847, #848)" { + # total=1 is deliberately not checked: with one shard both pins wrap into + # slot 0 and land together by construction, same as every other file — the + # pin has nothing to separate them FROM at total=1, so that is not a case + # this property claims to hold. + local pin1="test_remote_engine_start_refusal.bats" + local pin2="test_remote_status_liveness.bats" + local total i shard_files together + for total in 2 3 4 5 8; do + together=0 + for ((i = 1; i <= total; i++)); do + shard_files="$(cd "$REPO_ROOT" && bash "$SHARD" "$i" "$total")" + if grep -qF "$pin1" <<<"$shard_files" && grep -qF "$pin2" <<<"$shard_files"; then + together=1 + fi + done + [ "$together" -eq 0 ] + done +} + +@test "adding tests to an unrelated file does not reunite the pinned-apart files (#847 positive control)" { + # Reproduces #847's actual trigger, not a stand-in for it: that issue's + # collision was caused by appending @test cases to ONE file + # (test_roster_journal.bats) and having the repack land two OTHER, entirely + # untouched files in the same shard. This does the same append and checks + # the two files pinned above specifically, since a pin is exactly the part + # of the fix that is supposed to make this kind of drift unable to reunite + # them, whatever else in the tree changes shape. + local pin1="test_remote_engine_start_refusal.bats" + local pin2="test_remote_status_liveness.bats" + local dir="$BATS_TEST_TMPDIR/tests-plus" + mkdir -p "$dir" + cp "$REPO_ROOT"/tests/*.bats "$dir"/ + local i + for i in $(seq 1 20); do + printf '\n@test "synthetic case %d" {\n true\n}\n' "$i" >> "$dir/test_roster_journal.bats" + done + + local shard1="" shard2="" shard_files + for i in 1 2 3 4; do + shard_files="$(cd "$REPO_ROOT" && bash "$SHARD" "$i" 4 "$dir")" + grep -qF "$pin1" <<<"$shard_files" && shard1="$i" + grep -qF "$pin2" <<<"$shard_files" && shard2="$i" + done + [ -n "$shard1" ] + [ -n "$shard2" ] + [ "$shard1" != "$shard2" ] +} + @test "shard-tests.sh rejects out-of-range and non-numeric arguments" { run bash "$SHARD" 0 4 [ "$status" -eq 2 ]