Merge pull request #237 from fiddlerwoaroof/tooling/7fl8-exact-polls
tooling: pr-reviews timeout case asserts WAIT+1 polls, and a hang fails (7fl8)
This commit is contained in:
Ed Langley
GitHub
@@ -645,10 +645,12 @@ one place, with no per-invocation nix cost:
|
||||
calls it and the local gate calls it, so there is no second invocation to drift.
|
||||
It does three things a bare `./scripts/X-test.sh` does not:
|
||||
|
||||
1. **Pins a bash ≥ 5.** The ambient one is tried first, so a runner that already
|
||||
has bash 5.2 pays nothing; nix supplies one only when the ambient bash is
|
||||
older, which on macOS outside the dev shell it is. No bash ≥ 5 and no nix is
|
||||
**exit 3** — a refusal, never a quiet run under 3.2.
|
||||
1. **Pins a bash ≥ 5.1.** The ambient one is tried first, so a runner that
|
||||
already has bash 5.2 pays nothing; nix supplies one only when the ambient
|
||||
bash is older, which on macOS outside the dev shell it is. No bash ≥ 5.1 and
|
||||
no nix is **exit 3** — a refusal, never a quiet run under 3.2. The floor is
|
||||
5.1 and not 5 because `pr-reviews-test.sh` uses `wait -n -p`, and `-p`
|
||||
arrived in 5.1. Under 5.0 it is an invalid option (improvise-fd0j).
|
||||
2. **Scrubs the environment to an allowlist** (`env -i` plus `PATH`, `HOME`,
|
||||
`TMPDIR`, the `NIX_*` and `GITHUB_*` names, extensible through
|
||||
`CHECKERS_KEEP`). This is the part that closes the direnv hole above, and it
|
||||
|
||||
@@ -812,6 +812,63 @@ to corrupt a tracked file and call cargo, which needs the crates.io index, from
|
||||
`checkers.sh`, whose environment is scrubbed. The by-hand run above is the
|
||||
evidence.
|
||||
|
||||
### The timeout case asserts the exact poll count, and a hang fails — `scripts/pr-reviews-test.sh` (improvise-7fl8)
|
||||
|
||||
**What was unguarded.** "--wait keeps polling until its deadline" asserted *at
|
||||
least* 5 polls for `--wait 5`. On the one-tick-per-read clock the correct loop
|
||||
polls exactly 6 times: one initial poll, then re-polls until the post-poll clock
|
||||
read reaches the deadline. The 7epn entry above says the poll count tells a
|
||||
loop that stopped early from one that reached the deadline. That was true only
|
||||
for a loop stopping two or more ticks early. Both rivals from the #218 review
|
||||
passed the whole suite at 25/25, re-run here before the change:
|
||||
- a loop capped at 5 polls whatever the deadline;
|
||||
- `deadline = now + WAIT - 1`.
|
||||
|
||||
**The fix** asserts `polls = WAIT + 1`, which the fake clock makes exact. It
|
||||
also catches an over-polling `-le`. Against the new assertion both rivals give
|
||||
24 passed, 1 failed, each "exit 1 after 5 polls, wanted 1 after 6".
|
||||
|
||||
**The hang, which was wider than the ticket said.** 7epn left open that
|
||||
deleting the deadline check hangs the suite. It does not hang only at the
|
||||
fake-clock case. It hangs first at "an inline comment at an EARLIER head does
|
||||
not satisfy --wait", a real-clock `--wait 1` case, and at every other case that
|
||||
should time out. So the guard wraps *every* call. `"$REVIEWS"` is now a function
|
||||
that runs the script alongside a `/bin/sleep 20` watchdog and waits for
|
||||
whichever finishes first (`wait -n -p`, bash 5.1+). If the watchdog wins, it
|
||||
kills the script and returns 124. `/bin/sleep` is spelled out because the
|
||||
fake-clock cases put a no-op `sleep` first on `PATH`. A first version that
|
||||
killed a watchdog subshell left an orphaned `sleep`, and could signal a
|
||||
recycled pid.
|
||||
|
||||
**Rival: the deadline check deleted.** The suite now finishes in 93 s with 21
|
||||
passed and 4 failed. The four timeout cases fail with exit 124; the fake-clock
|
||||
one has made about 400 polls by then. Before the guard the same rival never
|
||||
finished, and an outer `timeout 30` killed it at exit 124.
|
||||
|
||||
### The bash floor is 5.1 — `scripts/checkers-test.sh` (improvise-fd0j)
|
||||
|
||||
**What broke.** `pr-reviews-test.sh` calls `wait -n -p`, and `-p` arrived in
|
||||
bash 5.1. `checkers.sh`'s `find_modern_bash` accepted any major version 5. So a
|
||||
bash 5.0 on `PATH` was pinned, and that suite then failed on an invalid option
|
||||
rather than on anything it tests.
|
||||
|
||||
**Fix.** The probe reads major and minor in one call and requires
|
||||
`major*100 + minor >= 501`. The exit-3 message and the header say 5.1.
|
||||
`context/code/tooling.md` states why the floor is 5.1.
|
||||
|
||||
**Cases** (section 5b). A fake bash answering `5 0` with no nix → exit 3. The
|
||||
control, a fake answering `5 1` → exit 0, proves that refusal comes from the
|
||||
version and not from the fake.
|
||||
|
||||
**Rivals**, run against the real suite in a copied sandbox:
|
||||
- **Floor 5.0** (`-ge 500`) fails "a bash 5.0 with no nix is exit 3, not 5.x"
|
||||
and nothing else.
|
||||
- **The pre-fix runner** fails only the 5.1 control. Its probe asked for the
|
||||
major version alone, so the fake's `5 1` was not a number and was refused.
|
||||
The 5.0 case passes against it for the same reason, so that case alone would
|
||||
not have distinguished the old code. The 5.0-floor rival is the one that
|
||||
does.
|
||||
|
||||
### The dead-ID audit compares positively — `scripts/bd-audit-ids-test.sh` (improvise-52eq)
|
||||
|
||||
**What broke.** The audit recipe in `AGENTS.md` ended
|
||||
|
||||
@@ -147,6 +147,19 @@ got=$?
|
||||
[ "$got" = 3 ] && ok "an old bash with no nix is exit 3" \
|
||||
|| bad "an old bash with no nix is exit 3" "exit $got, wanted 3"
|
||||
|
||||
# 5b. The floor is 5.1, not 5: `wait -n -p` is 5.1 (improvise-fd0j). The 5.1
|
||||
# control proves the 5.0 refusal is the version and not the fake itself.
|
||||
for v in "5 0:3:a bash 5.0 with no nix is exit 3, not 5.x" \
|
||||
"5 1:0:a bash 5.1 is accepted"; do
|
||||
printf '#!/bin/sh\ncase "$*" in *BASH_VERSINFO*) echo %s; exit 0 ;; esac\nexit 0\n' \
|
||||
"${v%%:*}" > "$work/fakebin/bash"
|
||||
want="${v#*:}"; name="${want#*:}"; want="${want%%:*}"
|
||||
env PATH="$work/fakebin:/usr/bin:/bin" HOME="$HOME" \
|
||||
"${BASH:-bash}" "$d/checkers.sh" any > "$work/out" 2>&1
|
||||
got=$?
|
||||
[ "$got" = "$want" ] && ok "$name" || bad "$name" "exit $got, wanted $want"
|
||||
done
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# 6. A named suite that does not exist is a usage error, not a silent no-op.
|
||||
d="$(sandbox missing)"
|
||||
|
||||
+10
-7
@@ -5,7 +5,7 @@
|
||||
#
|
||||
# It does three things a bare `./scripts/X-test.sh` does not:
|
||||
#
|
||||
# 1. pins a bash >= 5, taken from nix when the ambient one is older, so the
|
||||
# 1. pins a bash >= 5.1, taken from nix when the ambient one is older, so the
|
||||
# macOS system bash 3.2 can never be the shell a suite passes under;
|
||||
# 2. scrubs the environment down to an allowlist, because `nix develop`
|
||||
# exports `name`, `out`, `system`, `shell`, `builder` and two dozen more
|
||||
@@ -30,7 +30,7 @@ usage: checkers.sh [--list] [--self-test] [name ...]
|
||||
|
||||
exit 0 every suite passed
|
||||
exit 1 a suite failed, or a shebang is not `#!/usr/bin/env bash`
|
||||
exit 3 no bash >= 5 could be found and nix could not supply one
|
||||
exit 3 no bash >= 5.1 could be found and nix could not supply one
|
||||
exit 4 usage error
|
||||
|
||||
THIS SCRIPT'S `$?` MUST BE THE ONE YOU READ. Nothing may follow it in the same
|
||||
@@ -46,14 +46,17 @@ ALLOW="HOME TMPDIR TMP TEMP TERM LANG LC_ALL USER LOGNAME
|
||||
NIX_PATH NIX_REMOTE NIX_SSL_CERT_FILE SSL_CERT_FILE
|
||||
XDG_CACHE_HOME XDG_CONFIG_HOME XDG_DATA_HOME"
|
||||
|
||||
# Prints the absolute path of a bash >= 5, or nothing. The ambient one is tried
|
||||
# Prints the absolute path of a bash >= 5.1, or nothing. The ambient one is tried
|
||||
# first so a machine that already has a modern bash pays no nix startup at all.
|
||||
find_modern_bash() {
|
||||
local candidate major
|
||||
local candidate major minor
|
||||
candidate="$(command -v bash || true)"
|
||||
if [ -n "$candidate" ]; then
|
||||
major="$("$candidate" -c 'echo "${BASH_VERSINFO[0]}"' 2>/dev/null || echo 0)"
|
||||
if [ "$major" -ge 5 ] 2>/dev/null; then echo "$candidate"; return 0; fi
|
||||
read -r major minor < <("$candidate" -c 'echo "${BASH_VERSINFO[0]} ${BASH_VERSINFO[1]}"' \
|
||||
2>/dev/null || echo 0 0)
|
||||
if [ "$(( ${major:-0} * 100 + ${minor:-0} ))" -ge 501 ] 2>/dev/null; then
|
||||
echo "$candidate"; return 0
|
||||
fi
|
||||
fi
|
||||
command -v nix >/dev/null 2>&1 || return 0
|
||||
nix shell nixpkgs#bash --command bash -c 'command -v bash' 2>/dev/null || true
|
||||
@@ -122,7 +125,7 @@ main() {
|
||||
|
||||
local pinned; pinned="$(find_modern_bash)"
|
||||
if [ -z "$pinned" ]; then
|
||||
echo 'checkers: no bash >= 5 on PATH and nix cannot supply one.' >&2
|
||||
echo 'checkers: no bash >= 5.1 on PATH and nix cannot supply one.' >&2
|
||||
echo ' Enter the dev shell (direnv allow / nix develop) and retry.' >&2
|
||||
return 3
|
||||
fi
|
||||
|
||||
@@ -5,10 +5,25 @@
|
||||
set -uo pipefail
|
||||
|
||||
here="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
|
||||
REVIEWS="$here/pr-reviews"
|
||||
work="$(mktemp -d)"
|
||||
trap 'rm -rf "$work"' EXIT
|
||||
|
||||
# Every run is bounded, so a loop that never stops FAILS (exit 124) rather than
|
||||
# hanging the suite. /bin/sleep, because some cases put a no-op `sleep` first on PATH.
|
||||
BOUND=20
|
||||
bounded_reviews() {
|
||||
"$here/pr-reviews" "$@" &
|
||||
local pid=$! dog done rc
|
||||
/bin/sleep "$BOUND" &
|
||||
dog=$!
|
||||
wait -n -p done "$pid" "$dog"; rc=$?
|
||||
if [ "$done" = "$pid" ]; then kill "$dog" 2>/dev/null; wait "$dog" 2>/dev/null; return "$rc"; fi
|
||||
kill -TERM "$pid" 2>/dev/null; wait "$pid" 2>/dev/null
|
||||
echo "pr-reviews-test: killed after ${BOUND}s; the --wait loop never stopped" >&2
|
||||
return 124
|
||||
}
|
||||
REVIEWS=bounded_reviews
|
||||
|
||||
pass=0; fail=0
|
||||
ok() { printf ' ok %s\n' "$1"; pass=$((pass+1)); }
|
||||
bad() { printf ' FAIL %s (%s)\n' "$1" "$2"; fail=$((fail+1)); }
|
||||
@@ -240,15 +255,16 @@ polls=$(cat "$d3/.poll" 2>/dev/null || echo 0)
|
||||
[ "$got" = 0 ] && [ "$polls" = 3 ] && ok "a review arriving on the third poll is seen" \
|
||||
|| bad "a review arriving on the third poll is seen" "exit $got after $polls polls, wanted 0 after 3"
|
||||
|
||||
# The timeout path: nothing ever arrives, so --wait 5 on a clock that ticks
|
||||
# once per read must keep polling until the deadline, then report exit 1.
|
||||
# The timeout path: nothing arrives, and on a clock that ticks once per read the
|
||||
# count is exact: 1 poll, then one per tick until the deadline, so WAIT + 1.
|
||||
WAIT=5
|
||||
rm -f "$work/empty/.poll" "$work/ticks"
|
||||
CLOCK_TICKS="$work/ticks" PATH="$clock:$PATH" PR_REVIEWS_FIXTURE_DIR="$work/empty" \
|
||||
"$REVIEWS" 1 --wait 5 --interval 1 > "$work/out" 2>&1
|
||||
"$REVIEWS" 1 --wait "$WAIT" --interval 1 > "$work/out" 2>&1
|
||||
got=$?
|
||||
polls=$(cat "$work/empty/.poll" 2>/dev/null || echo 0)
|
||||
[ "$got" = 1 ] && [ "$polls" -ge 5 ] && ok "--wait keeps polling until its deadline" \
|
||||
|| bad "--wait keeps polling until its deadline" "exit $got after $polls polls, wanted 1 after 5 or more"
|
||||
[ "$got" = 1 ] && [ "$polls" = $((WAIT + 1)) ] && ok "--wait keeps polling until its deadline" \
|
||||
|| bad "--wait keeps polling until its deadline" "exit $got after $polls polls, wanted 1 after $((WAIT + 1))"
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# `gh api --paginate` emits one array PER PAGE, so a thread past the page size
|
||||
|
||||
Reference in New Issue
Block a user