diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8fbad57..ce7e0a0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -84,13 +84,58 @@ 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 + # test guarded on them returned early and reported GREEN without + # executing its body — a whole block of real-language-server and + # multi-shell coverage that had never once run in CI. Installing + # them is only half the fix; the `PMACS_REQUIRE_*` variables below + # are what turn a future missing tool back into a failure instead + # of silently restoring the vacuum. + # + # Linux only for now, deliberately. macOS would need the brew + # equivalents and roughly doubles the install cost on the slowest + # leg of the matrix; arming one platform already converts these + # from never-executed to executed, and the second is incremental. + # The tests still skip cleanly on macOS because the variables are + # unset there. + - name: Install external tools that gate acceptance tests (Linux) + if: runner.os == 'Linux' + run: | + sudo apt-get update + sudo apt-get install -y clangd zsh fish lua5.4 + # `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 + echo "$(go env GOPATH)/bin" >> "$GITHUB_PATH" + npm install -g vscode-langservers-extracted yaml-language-server - 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 # expose cross-test process lifecycle races that are unrelated # to the behavior under test. + # + # PMACS_REQUIRE_* make a missing tool fatal rather than a silent + # skip, exactly as PMACS_REQUIRE_GPU already does for the headless + # render job. Set only where the install step ran. + # + # PMACS_REQUIRE_PYRIGHT is deliberately NOT set and basedpyright + # is deliberately NOT installed: that test has no timeout and + # hangs forever (root cause is the non-interruptible reader-thread + # join in `RuntimeHandles::drop`, already a named deferral in + # `src/process.rs`). This job has no `timeout-minutes`, so arming + # it today would trade a vacuous green for a six-hour hang on four + # legs. It gets armed after the hang fix and the CI timeouts land, + # and its own variable exists so that flip is one line. - run: cargo test --all-targets --no-default-features --features ${{ matrix.lua }} -- --test-threads=1 + env: + PMACS_REQUIRE_LSP: ${{ runner.os == 'Linux' && '1' || '' }} + PMACS_REQUIRE_SHELLS: ${{ runner.os == 'Linux' && '1' || '' }} + PMACS_REQUIRE_LUA: ${{ runner.os == 'Linux' && '1' || '' }} - run: cargo test --doc --no-default-features --features ${{ matrix.lua }} # The workspace default member is only the root `pmacs` package, so # the runs above never execute pmacs-protocol's own tests — the diff --git a/docs/active-work.md b/docs/active-work.md index 2dfbaf0..8e216b3 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -521,6 +521,60 @@ has **no branch and no framing yet**. `FrontendView.fold_projection` to `true` for semantic frontends, which Stage 2 deliberately left `false` (Q#FD21). +## 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. +- **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 + tests had therefore **never once executed their bodies** in CI while + reporting success. A suite that cannot distinguish "passed" from + "never ran" is worse than a missing one, because it reads as + coverage. +- **The fix is the project's own pattern.** `PMACS_REQUIRE_*` already + makes a missing GPU fatal for `vterm_stage3_acceptance`; this adds + `PMACS_REQUIRE_LSP`, `PMACS_REQUIRE_SHELLS` and `PMACS_REQUIRE_LUA`, + plus the CI step that installs the tools. Per-tool variables, not one + blanket flag, so a tool that must stay unarmed keeps its decision + visible at the call site. +- **`basedpyright` is deliberately NOT installed and NOT armed.** Its + test has no timeout and hangs forever — root cause is the + non-interruptible reader-thread join in `RuntimeHandles::drop`, + already a named deferral in `src/process.rs`. The `test` job has no + `timeout-minutes` either. Arming it today would trade a vacuous green + for a six-hour hang across four legs. `PMACS_REQUIRE_PYRIGHT` exists + and is never set, so the flip is one line once lane 4 (the hang) and + lane 3 (timeouts) land. **Do not arm it before both.** +- **A trap found while writing the workflow, not after:** the natural + Actions idiom `${{ runner.os == 'Linux' && '1' || '' }}` sets the + variable to the EMPTY STRING elsewhere, and `var_os().is_some()` is + true for `Some("")`. That would have armed the guard on exactly the + runners with no tools installed. The helper therefore treats empty as + unset. `PMACS_REQUIRE_GPU` has the same latent shape and is safe only + because it is set literally. +- **Verified by execution in all three states**, on a tool genuinely + absent from this machine (`vscode-json-language-server`): unset -> + skips green; armed -> hard failure naming the CI step; empty string + -> skips green. The armed failure is the bite, and on `main` it + cannot occur because no guard exists. +- **The tests pass when they actually run** — which was the open + 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. +- 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. +- Also removes the **documentation lane**, whose disposition the ledger + left undecided pending confirmation that its branch carried nothing + unique. Confirmed by measurement: `githubsucks/handoff-2026-07-20` is + **1 ahead, 365 behind**, and its entire unique diff is four doc files + at 42 insertions against 88 deletions — merging it would *revert* + 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` @@ -543,32 +597,6 @@ git worktree add --track \ githubsucks/kill-ring-browser ``` -## Documentation lane — STALE, AND ITS DISPOSITION IS UNDECIDED - -> **Measured 2026-07-28, not inferred:** `githubsucks/handoff-2026-07-20` -> is at `c11d7e7`, **1 commit ahead of `main` and 320 behind**. Its -> whole diff against `main` is four documentation files -> (`docs/active-work.md`, `docs/agent-handoff.md`, -> `docs/roadmap-2026-07.md`, `docs/vterm-framing.md`), every one of -> which has been rewritten repeatedly since by the landed-doc PRs -> #156/#168/#169/#172/#180. Rule 4 removes a lane on merge *or -> abandonment*, and this one looks abandoned in substance — but "looks -> abandoned" is not the same as a decision, and no PR was ever opened -> for it. **This snapshot deliberately annotates rather than deletes: -> whoever confirms the branch carries nothing unique removes the -> section.** The bullets below are its original claims, preserved as -> written and now unverified. - -- Portable branch: `githubsucks/handoff-2026-07-20` -- Carries synchronized `AGENTS.md` / `CLAUDE.md`, this ledger, the - durable handoff refresh, and the keybinding reference correction. -- It changes no runtime code. -- Review and merge this documentation branch separately; it must not be - folded into a feature framing branch. -- Now also absorbs both landed arcs: Vterm Stage 1 (#126) and the config - registry (#127). Canonical `main` is merged into it up to `2e37c04`, - so its diff against `main` is documentation only. - ## Closed since the last snapshot - **Terminal configuration + copy mode arc — BOTH STAGES MERGED, lane diff --git a/tests/m4_acceptance.rs b/tests/m4_acceptance.rs index 691d746..f56dfc7 100644 --- a/tests/m4_acceptance.rs +++ b/tests/m4_acceptance.rs @@ -1298,7 +1298,7 @@ fn fake_spec(label: &str) -> LspServerSpec { #[test] fn m4_5_rust_analyzer_initializes() { let Ok(_) = which_binary("rust-analyzer") else { - eprintln!("rust-analyzer not on PATH; skipping"); + support::skip_or_fail("rust-analyzer", "PMACS_REQUIRE_LSP"); return; }; let (sup, mgr) = make_lsp_test_manager(); @@ -1338,7 +1338,7 @@ fn m4_5_rust_analyzer_initializes() { #[test] fn m4_5_basedpyright_initializes_and_negotiates_encoding() { let Ok(_) = which_binary("basedpyright-langserver") else { - eprintln!("basedpyright-langserver not on PATH; skipping"); + support::skip_or_fail("basedpyright-langserver", "PMACS_REQUIRE_PYRIGHT"); return; }; let (sup, mgr) = make_lsp_test_manager(); @@ -1419,7 +1419,7 @@ fn assert_lsp_initializes_and_negotiates( #[test] fn m4_5_clangd_initializes_and_negotiates_encoding() { let Ok(_) = which_binary("clangd") else { - eprintln!("clangd not on PATH; skipping"); + support::skip_or_fail("clangd", "PMACS_REQUIRE_LSP"); return; }; assert_lsp_initializes_and_negotiates("clangd", "cpp", "clangd", &["--background-index"]); @@ -1431,7 +1431,7 @@ fn m4_5_clangd_initializes_and_negotiates_encoding() { #[test] fn m4_5_gopls_initializes_and_negotiates_encoding() { let Ok(_) = which_binary("gopls") else { - eprintln!("gopls not on PATH; skipping"); + support::skip_or_fail("gopls", "PMACS_REQUIRE_LSP"); return; }; assert_lsp_initializes_and_negotiates("gopls", "go", "gopls", &[]); @@ -3428,6 +3428,9 @@ fn m4_11_snippets_surface_through_completion() { use pmacs::definition::DefinitionKey; use pmacs::formatting::FormattingKey; +#[path = "support/mod.rs"] +mod support; + /// Acceptance (1/3): a `textDocument/definition` request round-trips /// through the manager and lands in the definition store as a parsed /// `Location` list. @@ -5536,7 +5539,7 @@ fn m4_27_real_gopls_analyzes_module_via_auto_attach() { use pmacs::editor::EditorState; let Ok(gopls) = which_binary("gopls") else { - eprintln!("gopls not on PATH; skipping"); + support::skip_or_fail("gopls", "PMACS_REQUIRE_LSP"); return; }; let gopls = gopls.display().to_string(); @@ -5633,7 +5636,7 @@ fn m4_28_real_clangd_diagnostics_and_semantic_tokens_via_auto_attach() { use pmacs::editor::EditorState; let Ok(clangd) = which_binary("clangd") else { - eprintln!("clangd not on PATH; skipping"); + support::skip_or_fail("clangd", "PMACS_REQUIRE_LSP"); return; }; let clangd = clangd.display().to_string(); @@ -5731,7 +5734,7 @@ fn m4_29_real_rust_analyzer_inlay_hints_via_auto_attach() { use pmacs::editor::EditorState; let Ok(rust_analyzer) = which_binary("rust-analyzer") else { - eprintln!("rust-analyzer not on PATH; skipping"); + support::skip_or_fail("rust-analyzer", "PMACS_REQUIRE_LSP"); return; }; let rust_analyzer = rust_analyzer.display().to_string(); @@ -6880,7 +6883,7 @@ fn m4_real_json_provider_receives_config_and_reports_diagnostics() { use pmacs::editor::EditorState; let Ok(command) = which_binary("vscode-json-language-server") else { - eprintln!("vscode-json-language-server not on PATH; skipping"); + support::skip_or_fail("vscode-json-language-server", "PMACS_REQUIRE_LSP"); return; }; let command = command.display().to_string(); @@ -6949,7 +6952,7 @@ fn m4_real_yaml_provider_pulls_config_and_reports_diagnostics() { use pmacs::editor::EditorState; let Ok(command) = which_binary("yaml-language-server") else { - eprintln!("yaml-language-server not on PATH; skipping"); + support::skip_or_fail("yaml-language-server", "PMACS_REQUIRE_LSP"); return; }; let command = command.display().to_string(); diff --git a/tests/m6_5_repl_acceptance.rs b/tests/m6_5_repl_acceptance.rs index fcf9bf4..fc453ba 100644 --- a/tests/m6_5_repl_acceptance.rs +++ b/tests/m6_5_repl_acceptance.rs @@ -35,6 +35,9 @@ use std::path::PathBuf; use std::sync::{Mutex, MutexGuard}; use std::time::{Duration, Instant}; +#[path = "support/mod.rs"] +mod support; + static PUMP_TEST_LOCK: Mutex<()> = Mutex::new(()); fn pump_test_guard() -> MutexGuard<'static, ()> { @@ -196,7 +199,7 @@ fn m6_5_ret_submits_input_to_process() { #[test] fn m6_5_ctrl_d_on_empty_prompt_closes_stdin() { let Some(bash) = locate_shell("bash") else { - eprintln!("skipping: bash not on PATH (set PMACS_TEST_BASH to override)"); + support::skip_or_fail_overridable("bash", "PMACS_REQUIRE_SHELLS", "PMACS_TEST_BASH"); return; }; let setup = format!( @@ -290,7 +293,7 @@ fn m6_5_ctrl_d_on_nonempty_input_deletes_char_forward() { )] fn m6_5_ctrl_c_sends_sigint() { let Some(sleep) = locate_shell("sleep") else { - eprintln!("skipping: sleep not on PATH (set PMACS_TEST_SLEEP to override)"); + support::skip_or_fail_overridable("sleep", "PMACS_REQUIRE_SHELLS", "PMACS_TEST_SLEEP"); return; }; let setup = format!( @@ -344,7 +347,7 @@ fn m6_5_ctrl_c_sends_sigint() { )] fn m6_5_exit_marker_uses_basename_with_leading_newline() { let Some(false_bin) = locate_shell("false") else { - eprintln!("skipping: false not on PATH (set PMACS_TEST_FALSE to override)"); + support::skip_or_fail_overridable("false", "PMACS_REQUIRE_SHELLS", "PMACS_TEST_FALSE"); return; }; let setup = format!( @@ -425,7 +428,7 @@ fn run_shell_smoke_test(shell_path: &std::path::Path, argv_extra: &[&str]) { #[test] fn m6_5_repl_spawns_bash() { let Some(bash) = locate_shell("bash") else { - eprintln!("skipping: bash not on PATH (set PMACS_TEST_BASH to override)"); + support::skip_or_fail_overridable("bash", "PMACS_REQUIRE_SHELLS", "PMACS_TEST_BASH"); return; }; run_shell_smoke_test(&bash, &["-i"]); @@ -435,7 +438,7 @@ fn m6_5_repl_spawns_bash() { #[test] fn m6_5_repl_spawns_zsh() { let Some(zsh) = locate_shell("zsh") else { - eprintln!("skipping: zsh not on PATH (set PMACS_TEST_ZSH to override)"); + support::skip_or_fail_overridable("zsh", "PMACS_REQUIRE_SHELLS", "PMACS_TEST_ZSH"); return; }; run_shell_smoke_test(&zsh, &["-i"]); @@ -446,7 +449,7 @@ fn m6_5_repl_spawns_zsh() { #[test] fn m6_5_repl_spawns_fish() { let Some(fish) = locate_shell("fish") else { - eprintln!("skipping: fish not on PATH (set PMACS_TEST_FISH to override)"); + support::skip_or_fail_overridable("fish", "PMACS_REQUIRE_SHELLS", "PMACS_TEST_FISH"); return; }; run_shell_smoke_test(&fish, &["-i"]); @@ -458,9 +461,7 @@ 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 { - eprintln!( - "skipping: lua/luajit not on PATH (set PMACS_TEST_LUA or PMACS_TEST_LUAJIT to override)" - ); + support::skip_or_fail_overridable("lua/luajit", "PMACS_REQUIRE_LUA", "PMACS_TEST_LUA"); return; }; let setup = format!( diff --git a/tests/m6_8_multi_repl_acceptance.rs b/tests/m6_8_multi_repl_acceptance.rs index 673daa4..c1bd071 100644 --- a/tests/m6_8_multi_repl_acceptance.rs +++ b/tests/m6_8_multi_repl_acceptance.rs @@ -68,6 +68,9 @@ use pmacs::editor::EditorState; use std::path::{Path, PathBuf}; use std::time::{Duration, Instant}; +#[path = "support/mod.rs"] +mod support; + // --------------------------------------------------------------------------- // Test harness // --------------------------------------------------------------------------- @@ -94,9 +97,7 @@ fn locate_lua() -> Option { } } } - eprintln!( - "skipping: lua/luajit not on PATH (set PMACS_TEST_LUA or PMACS_TEST_LUAJIT to override)" - ); + support::skip_or_fail_overridable("lua/luajit", "PMACS_REQUIRE_LUA", "PMACS_TEST_LUA"); None } diff --git a/tests/support/mod.rs b/tests/support/mod.rs new file mode 100644 index 0000000..5b6ca97 --- /dev/null +++ b/tests/support/mod.rs @@ -0,0 +1,94 @@ +//! Shared test-support helpers. +//! +//! Included by `#[path = "support/mod.rs"] mod support;` rather than +//! copied. 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 +//! so. A correct helper in one file and a degraded copy in another is +//! this suite's most repeated defect shape; sharing removes the way it +//! happens. + +#![allow(dead_code)] + +/// Report a missing external tool, and turn the skip into a HARD +/// FAILURE when the environment has promised the tool is present. +/// +/// The bare shape this replaces — +/// +/// ```ignore +/// let Ok(_) = which_binary("gopls") else { +/// eprintln!("gopls not on PATH; skipping"); +/// return; +/// }; +/// ``` +/// +/// passes GREEN when the tool is absent, and is why a large block of +/// external-tool-gated tests had never once executed their bodies in +/// CI: nothing installed the tools, so every one of them reported +/// success without running. A suite that cannot tell "passed" from +/// "never ran" is worse than a missing suite, because it reads as +/// coverage. +/// +/// `PMACS_REQUIRE_*` is the project's own fix, already load-bearing for +/// `PMACS_REQUIRE_GPU` in `vterm_stage3_acceptance`: CI installs the +/// tool, sets the variable, and absence becomes a failure that names +/// the step that should have provided it. Locally the variable is +/// unset, so the skip still works and nobody needs the whole toolchain +/// to run the suite. +/// +/// Deliberately per-tool rather than one blanket variable: a tool that +/// must stay unarmed (because arming it would hang, or because CI does +/// not install it yet) keeps its own variable that CI never sets, and +/// that decision is then visible at the call site instead of buried in +/// a workflow file. +/// True when `var` is set to a non-empty value. +/// +/// Emptiness matters, and the reason is a trap rather than a nicety. +/// The natural GitHub Actions idiom for a conditional environment +/// variable — +/// +/// ```yaml +/// PMACS_REQUIRE_LSP: ${{ runner.os == 'Linux' && '1' || '' }} +/// ``` +/// +/// sets the variable to the EMPTY STRING on every other platform, not +/// to nothing. A bare `var_os(..).is_some()` is therefore true there, +/// which would arm the guard on exactly the runners that have none of +/// the tools installed and fail every one of them. Treating empty as +/// unset makes the common workflow spelling safe instead of subtly +/// wrong. +fn armed(var: &str) -> bool { + std::env::var_os(var).is_some_and(|v| !v.is_empty()) +} + +#[track_caller] +pub fn skip_or_fail(tool: &str, require_var: &str) { + assert!( + !armed(require_var), + "{require_var} is set, but `{tool}` is not on PATH. \ + The CI step that installs it did not run, or installed it \ + somewhere not on PATH. This is a hard failure precisely so \ + the test cannot report green without executing." + ); + eprintln!("{tool} not on PATH; skipping (set {require_var} to make this fatal)"); +} + +/// As [`skip_or_fail`], for tools whose PATH lookup can be overridden +/// by a `PMACS_TEST_*` variable. The skip notice keeps naming that +/// override, because losing it would make the local escape hatch +/// undiscoverable — the REPL suites are routinely run on machines +/// without zsh or fish. +#[track_caller] +pub fn skip_or_fail_overridable(tool: &str, require_var: &str, override_var: &str) { + assert!( + !armed(require_var), + "{require_var} is set, but `{tool}` is not on PATH and {override_var} \ + is unset or points at nothing. The CI step that installs it did not \ + run, or installed it somewhere not on PATH." + ); + eprintln!( + "skipping: {tool} not on PATH (set {override_var} to override, \ + or {require_var} to make this fatal)" + ); +}