tooling: pr-reviews polls past the second poll; stale harness lock names its fix
improvise-7epn: two cases on the one-tick-per-read clock. One resolves on poll 3, and one times out after polling up to the deadline. Replacing the post-poll deadline check with `break` now fails exactly those two (23 passed, 2 failed), where before it left the suite 23/23. improvise-lnsh: when `cargo metadata --locked` refuses the opfs-contract lock, build.sh now says not to remove --locked and names `cargo update -w --manifest-path harness/opfs-contract/Cargo.toml`. Checked by hand against a lock with pest pinned to 2.9.1. Evidence is in context/test-evidence/tooling.md. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
This commit is contained in:
@@ -763,3 +763,50 @@ to spare a harness directory. `opfs.spec.js` instead asserts `/opfs/index.html`
|
||||
is served, naming the fix. Injected by running `web/assemble.sh harness/serve`
|
||||
alone: 4 chromium failures, each carrying that message, where it used to fail
|
||||
with no hint of the cause.
|
||||
|
||||
### Polling past the second poll — `scripts/pr-reviews-test.sh` (improvise-7epn)
|
||||
|
||||
**What was unguarded.** Every multi-poll fixture resolved on poll 2, and the
|
||||
timeout case expects exit 1 whether the loop polls twice or twenty times. So
|
||||
replacing the post-poll deadline check with an unconditional `break` left the
|
||||
suite at 23 passed, 0 failed, and `--wait N` would have polled at most twice.
|
||||
|
||||
**Two cases, both on the one-tick-per-read clock.** "a review arriving on the
|
||||
third poll is seen" empties polls 1 and 2 and puts an inline comment at the head
|
||||
in `last`, with `--wait 10`, and asserts exit 0 *and* exactly 3 polls. "--wait
|
||||
keeps polling until its deadline" runs the empty fixture with `--wait 5` and
|
||||
asserts exit 1 after at least 5 polls. The second one is the timeout path. Its
|
||||
exit code alone cannot tell a loop that stopped early from one that reached the
|
||||
deadline, so it counts polls.
|
||||
|
||||
**Rival, injected and watched.** `break` in place of
|
||||
`[ "$(date +%s)" -lt "$deadline" ] || break`: 23 passed, 2 failed. The failures
|
||||
were exactly the two new cases, each "exit 1 after 2 polls". The first attempt
|
||||
to inject used `sed -i ''`, which the GNU sed on this PATH read as a filename.
|
||||
Nothing changed and the suite passed 25/25, which looks like a vacuous rival.
|
||||
The second attempt asserted the replacement landed before running.
|
||||
|
||||
**Still not covered.** Deleting the check outright makes the timeout case loop
|
||||
forever rather than fail. The suite has no wall-clock guard, so that fault shows
|
||||
as a hang, not a FAIL.
|
||||
|
||||
### A stale harness lock names its remedy — `harness/build.sh` (improvise-lnsh)
|
||||
|
||||
**What broke.** With a stale `harness/opfs-contract/Cargo.lock`, `build.sh`
|
||||
exited 101 and printed only cargo's text, whose `help:` line says to remove
|
||||
`--locked`. That is the one change that brings improvise-mkbp back. The real
|
||||
remedy was only in a source comment.
|
||||
|
||||
**Reproduced, then fixed, by hand.** Changing `pest`'s locked version from
|
||||
`2.9.2` to `2.9.1` in the lock reproduces the stale state. Before the fix:
|
||||
exit 101 and cargo's hint only. After: exit 1, and the script's own two lines
|
||||
follow cargo's, telling the reader not to remove `--locked` and naming
|
||||
`cargo update -w --manifest-path harness/opfs-contract/Cargo.toml`. The lock
|
||||
was untouched both times. Running that command on the stale lock restored it
|
||||
byte for byte (`cmp` against the committed file). With the committed lock,
|
||||
`build.sh` still runs to "harness/serve ready".
|
||||
|
||||
**Not automated.** There is no suite for `harness/build.sh`. A test would have
|
||||
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.
|
||||
|
||||
+5
-1
@@ -22,7 +22,11 @@ serve="$root/harness/serve"
|
||||
# re-resolves the lock itself before any `-- --locked` reaches cargo build.
|
||||
# Fix a failure with `cargo update -w` in harness/opfs-contract, and commit it.
|
||||
cargo metadata --locked --format-version 1 \
|
||||
--manifest-path "$root/harness/opfs-contract/Cargo.toml" >/dev/null
|
||||
--manifest-path "$root/harness/opfs-contract/Cargo.toml" >/dev/null || {
|
||||
echo "harness/build.sh: harness/opfs-contract/Cargo.lock is stale. Do not remove --locked." >&2
|
||||
echo " Run: cargo update -w --manifest-path harness/opfs-contract/Cargo.toml, and commit the lock." >&2
|
||||
exit 1
|
||||
}
|
||||
|
||||
"$root/web/build.sh" >/dev/null
|
||||
"$root/web/assemble.sh" "$serve" harness >/dev/null
|
||||
|
||||
@@ -217,6 +217,39 @@ got=$?
|
||||
[ "$got" = 0 ] && ok "a second passing before the first re-poll does not skip it" \
|
||||
|| bad "a second passing before the first re-poll does not skip it" "exit $got, wanted 0"
|
||||
|
||||
# Every fixture above resolves by poll 2, so a loop that stops after one
|
||||
# re-poll passed them all (improvise-7epn). This one resolves on poll 3.
|
||||
d3="$work/arrives-third"
|
||||
for n in 1 2; do
|
||||
mk "$d3" "$n.pr.json" <<EOF
|
||||
{"headRefOid":"$HEAD","reviews":[]}
|
||||
EOF
|
||||
mk "$d3" "$n.inline.json" <<'EOF'
|
||||
[]
|
||||
EOF
|
||||
mk "$d3" "$n.issue.json" <<'EOF'
|
||||
[]
|
||||
EOF
|
||||
done
|
||||
cp -f "$d/last.pr.json" "$d/last.inline.json" "$d/last.issue.json" "$d3/"
|
||||
rm -f "$d3/.poll" "$work/ticks"
|
||||
CLOCK_TICKS="$work/ticks" PATH="$clock:$PATH" PR_REVIEWS_FIXTURE_DIR="$d3" \
|
||||
"$REVIEWS" 1 --wait 10 --interval 1 > "$work/out" 2>&1
|
||||
got=$?
|
||||
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.
|
||||
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
|
||||
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"
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# `gh api --paginate` emits one array PER PAGE, so a thread past the page size
|
||||
# arrives as several concatenated documents. Passing that straight to
|
||||
|
||||
Reference in New Issue
Block a user