review round 1: stop installing rust-analyzer on macOS, and paste the base
P1 --- the workflow contradicted this lane's own claim. `components:
rust-analyzer` rode the shared `dtolnay/rust-toolchain` step, which
matrixes over ubuntu AND macos, so the binary would have been present
on both. PRESENCE, not PMACS_REQUIRE_LSP, is what decides whether a
gated test body runs --- the unset variables on macOS only meant
absence would be tolerated there, not that the tests stay skipped. Two
rust-analyzer tests would therefore have executed on macOS for the
first time ever, on the legs that are simultaneously the CI critical
path and the documented flake surface, one of them doing real indexing,
none of it covered by the Linux-only local runs behind this lane. Moved
into the Linux-gated step so the text and the workflow agree.
P2 --- the ledger promised a base and pasted nothing. Worse than the
review knew: a script was run to substitute it, reported success,
matched no text, and the result was never re-read. The claim shipped on
the strength of a tool's exit status. The entry now carries the pasted
base and a recovery command, and records the lesson in the terms this
ledger keeps relearning --- asserting a measurement is not making one,
and a tool reporting success is not the measurement either.
P3 --- tool versions pinned (gopls v0.16.2,
vscode-langservers-extracted 4.10.0, yaml-language-server 1.15.0), so
CI no longer drifts with upstream publishes and a break has a commit
here to bisect against. `tests/support/` now states why it exists
beside `tests/common/`: the latter re-exports daemon and PTY
machinery, and pulling that into three binaries that spawn neither to
reach a six-line environment check is the wrong trade. Recorded as a
cost, with the rule that a third such directory means consolidating
rather than continuing.
Also recorded, because §1.2 is NOT fully closed by this lane: the
guards arm the ENTRY skip only, and m4_acceptance's mid-test
"workspace likely still indexing; skipping" survives --- so even armed,
that test's one assertion can still vanish under load, which is exactly
when a regression would show. Mid-test skips want their own pass. And
the follow-up this lane creates: removing the documentation lane
removes the only pointer to githubsucks/handoff-2026-07-20, so that
branch needs deleting after merge or nothing will ever mention it
again.
P4 --- the consolidated lua skip message names PMACS_TEST_LUAJIT again;
the `or_else(locate_shell("luajit"))` path still honours it and the
escape hatch had become undiscoverable. Double blank line before the
parked lane collapsed.
One gate note worth carrying rather than burying. A `PMACS_REQUIRE_GPU=1
cargo test -p pmacs-gpu` run failed once during this round, naming the
`--bin pmacs-gpu` target, and my gate chain still printed a clean
summary because `cmd | tail -2` in an `&&` chain returns TAIL's exit
status, not cargo's. Four subsequent runs pass 202/202 and the failure
has not reproduced, so it is recorded as observed-and-unreproduced
rather than explained. The masking is the durable part: a gate summary
assembled through a pipe can report success over a failure.
Gates re-run after the fix: fmt; clippy -D warnings; --lib 1863; --lib
--features crdt 2048; m4_acceptance 121; m6_5 11; m6_8 8; required GPU
202 (x4); git diff --check clean.
This commit is contained in:
parent
35b119700f
commit
a81ff917d8
|
|
@ -84,8 +84,6 @@ jobs:
|
|||
steps:
|
||||
- uses: actions/checkout@v4
|
||||
- uses: dtolnay/rust-toolchain@stable
|
||||
with:
|
||||
components: rust-analyzer
|
||||
- uses: Swatinem/rust-cache@v2
|
||||
# Arm the external-tool-gated tests (see `TEST_IMPROVEMENT.md`
|
||||
# §1.2). Before this step nothing installed these tools, so every
|
||||
|
|
@ -110,9 +108,23 @@ jobs:
|
|||
# `locate_lua` looks for `lua` or `luajit` by name; the
|
||||
# distro package installs `lua5.4` only.
|
||||
sudo ln -sf "$(command -v lua5.4)" /usr/local/bin/lua
|
||||
go install golang.org/x/tools/gopls@latest
|
||||
# rust-analyzer belongs HERE, not on the shared toolchain
|
||||
# step. `components:` there applies to every matrix leg, so it
|
||||
# would install the binary on macOS too — and *presence*, not
|
||||
# PMACS_REQUIRE_LSP, is what decides whether a gated test body
|
||||
# runs. That would have executed the rust-analyzer tests on
|
||||
# macOS for the first time ever, on the legs that are both the
|
||||
# CI critical path and the documented flake surface, while
|
||||
# this lane's text claimed Linux only.
|
||||
rustup component add rust-analyzer
|
||||
# Versions are PINNED. `@latest` and bare `npm install -g`
|
||||
# make CI behaviour drift with upstream releases: a bad gopls
|
||||
# or yaml-language-server publish then breaks CI with no
|
||||
# commit in this repository to bisect against.
|
||||
go install golang.org/x/tools/gopls@v0.16.2
|
||||
echo "$(go env GOPATH)/bin" >> "$GITHUB_PATH"
|
||||
npm install -g vscode-langservers-extracted yaml-language-server
|
||||
npm install -g vscode-langservers-extracted@4.10.0 \
|
||||
yaml-language-server@1.15.0
|
||||
- run: cargo build --all-targets --no-default-features --features ${{ matrix.lua }}
|
||||
# Several acceptance binaries spawn real daemon / PTY child
|
||||
# processes. Keep the harness serial so macOS runners do not
|
||||
|
|
|
|||
|
|
@ -524,8 +524,24 @@ has **no branch and no framing yet**.
|
|||
## Test-improvement arc, lane 2 — silent-skip arming
|
||||
|
||||
- Portable branch: `githubsucks/silent-skip-arming`, worktree
|
||||
`../pmacs-skiparm`. Base measured at write time, pasted below.
|
||||
Implements `TEST_IMPROVEMENT.md` §1.2 and §5.4.
|
||||
`../pmacs-skiparm`. Implements `TEST_IMPROVEMENT.md` §1.2 and §5.4.
|
||||
- **Base, measured at write time rather than quoted:**
|
||||
|
||||
```
|
||||
$ git log --oneline -1 githubsucks/main
|
||||
5e186c7 Merge pull request #193 from levineuwirth/test-improvement-audit
|
||||
```
|
||||
|
||||
The previous revision of this entry said "base measured at write
|
||||
time, pasted below" and then pasted nothing: the script meant to
|
||||
substitute it reported success and silently matched no text, and the
|
||||
claim was not re-read. Recorded because it is the same defect this
|
||||
ledger keeps catching one level up — **asserting a measurement is not
|
||||
making one, and a tool reporting success is not the measurement
|
||||
either.**
|
||||
- Recovery from a clean checkout:
|
||||
`git fetch githubsucks && git worktree add ../pmacs-skiparm
|
||||
-b silent-skip-arming githubsucks/silent-skip-arming`.
|
||||
- **The defect:** `let Ok(_) = which_binary(x) else { eprintln!(..);
|
||||
return; }` reports GREEN when the tool is absent, and CI installed
|
||||
none of the tools. A block of real-language-server and multi-shell
|
||||
|
|
@ -563,6 +579,32 @@ has **no branch and no framing yet**.
|
|||
question, since none of them had. Armed locally: 11 `m6_5` + 8 `m6_8`
|
||||
REPL tests green, and all six real-LSP tests (clangd x2, gopls x2,
|
||||
rust-analyzer x2) green individually.
|
||||
- **rust-analyzer is installed in the Linux-gated step, not via the
|
||||
toolchain action's `components:`.** The first revision put it there,
|
||||
which applies to *every* matrix leg — and **presence, not
|
||||
`PMACS_REQUIRE_LSP`, is what decides whether a gated test body
|
||||
runs**. That would have executed the two rust-analyzer tests on macOS
|
||||
for the first time ever, on the legs that are simultaneously the CI
|
||||
critical path and the documented flake surface, while this entry
|
||||
claimed Linux only. The variables not being set there would only have
|
||||
meant absence was tolerated; it would not have kept the tests
|
||||
skipped. Text and workflow now agree.
|
||||
- **Tool versions are pinned** (`gopls@v0.16.2`,
|
||||
`vscode-langservers-extracted@4.10.0`,
|
||||
`yaml-language-server@1.15.0`). `@latest` and bare `npm install -g`
|
||||
make CI drift with upstream releases, so a bad publish breaks CI with
|
||||
no commit here to bisect against. Caching the built `gopls` on the
|
||||
pinned version is a follow-up, not done here.
|
||||
- **§1.2 is NOT fully closed by this lane.** The guards arm the
|
||||
*entry* skip only. `tests/m4_acceptance.rs`'s mid-test rust-analyzer
|
||||
bail ("workspace likely still indexing; skipping") survives, so even
|
||||
armed, that test's only assertion can still vanish under load —
|
||||
precisely when a regression would show. Mid-test skips are their own
|
||||
shape and want their own pass.
|
||||
- **Follow-up owed after this merges:** delete
|
||||
`githubsucks/handoff-2026-07-20`. Removing the documentation lane
|
||||
removes the only pointer to that branch, so nothing will otherwise
|
||||
remind anyone it still exists on the remote.
|
||||
- Linux only for now, deliberately: macOS needs the brew equivalents
|
||||
and roughly doubles install cost on the slowest matrix leg. The
|
||||
variables stay unset there, so those tests skip cleanly.
|
||||
|
|
@ -574,7 +616,6 @@ has **no branch and no framing yet**.
|
|||
current documentation. The section said "whoever confirms the branch
|
||||
carries nothing unique removes the section"; this is that.
|
||||
|
||||
|
||||
## Parked lane: kill-ring browser + persistence
|
||||
|
||||
- Portable branch: `githubsucks/kill-ring-browser`
|
||||
|
|
|
|||
|
|
@ -461,7 +461,11 @@ fn m6_5_repl_spawns_fish() {
|
|||
#[test]
|
||||
fn m6_5_repl_spawns_lua() {
|
||||
let Some(lua) = locate_shell("lua").or_else(|| locate_shell("luajit")) else {
|
||||
support::skip_or_fail_overridable("lua/luajit", "PMACS_REQUIRE_LUA", "PMACS_TEST_LUA");
|
||||
support::skip_or_fail_overridable(
|
||||
"lua/luajit",
|
||||
"PMACS_REQUIRE_LUA",
|
||||
"PMACS_TEST_LUA or PMACS_TEST_LUAJIT",
|
||||
);
|
||||
return;
|
||||
};
|
||||
let setup = format!(
|
||||
|
|
|
|||
|
|
@ -97,7 +97,11 @@ fn locate_lua() -> Option<PathBuf> {
|
|||
}
|
||||
}
|
||||
}
|
||||
support::skip_or_fail_overridable("lua/luajit", "PMACS_REQUIRE_LUA", "PMACS_TEST_LUA");
|
||||
support::skip_or_fail_overridable(
|
||||
"lua/luajit",
|
||||
"PMACS_REQUIRE_LUA",
|
||||
"PMACS_TEST_LUA or PMACS_TEST_LUAJIT",
|
||||
);
|
||||
None
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -1,7 +1,17 @@
|
|||
//! Shared test-support helpers.
|
||||
//!
|
||||
//! Included by `#[path = "support/mod.rs"] mod support;` rather than
|
||||
//! copied. Files under `tests/` subdirectories are not compiled as
|
||||
//! copied.
|
||||
//!
|
||||
//! **Why this is separate from `tests/common/`, which also exists.**
|
||||
//! `tests/common/mod.rs` re-exports `daemon` and `pty` — real daemon
|
||||
//! spawning and PTY plumbing. Including it to reach a six-line
|
||||
//! environment check would compile that machinery into three test
|
||||
//! binaries that spawn neither, for no benefit. `support` is the
|
||||
//! dependency-free half: helpers any test binary can take without
|
||||
//! taking a subsystem with them. Two directories is a cost worth
|
||||
//! naming rather than leaving to be rediscovered; if a third appears,
|
||||
//! consolidate instead of continuing the pattern. Files under `tests/` subdirectories are not compiled as
|
||||
//! their own test binaries, so this costs nothing — and
|
||||
//! `m6_8_multi_repl_acceptance.rs` previously carried a comment saying
|
||||
//! cross-test-binary sharing "would need a fixture crate", which is not
|
||||
|
|
|
|||
Loading…
Reference in New Issue