From 6dceaad1c726c1d06651e2a191d464bea0988aba Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 08:56:26 -0700 Subject: [PATCH] every_shipped_document_loads walks the index, not a .gitignore reading (improvise-c0aj) --- context/test-evidence/tui.md | 73 +++++++++++++++-------- tests/shipped_documents_load.rs | 102 ++++++++++++++++---------------- 2 files changed, 97 insertions(+), 78 deletions(-) diff --git a/context/test-evidence/tui.md b/context/test-evidence/tui.md index 48c7165e..4318bf1f 100644 --- a/context/test-evidence/tui.md +++ b/context/test-evidence/tui.md @@ -426,38 +426,59 @@ generator-driven proptests in `improvise-io`. What *those* cannot see is a checked-in document drifting out of the format, because they generate their inputs. This test is aimed at exactly that gap. -**It is deliberately a directory walk rather than a list of names**: a list is a +**It walks the files the repository tracks, not a list of names**: a list is a second place to remember, and the file that gets forgotten is the one nobody -added to it. The scope is the directories `.gitignore` un-ignores `*.improv` -in. Everything else matching `*.improv` is somebody's working file. +added to it. "Shipped" means "tracked", and `shipped_documents` reads it off +the index: every `*.improv` and `*.improv.gz` path that `ls-files` lists. It does +not include untracked files, so a working `.improv.gz` in someone's tree cannot +fail their run. A document not yet added is walked once it is, which is the +state CI tests. -**The directory list is read from `.gitignore`, not written in the test -(improvise-qlrh).** The test used to hard-code `["examples", "web"]`, a list of -exactly the kind the paragraph above warns against. When improvise-plcx -un-ignored `harness/fixtures/*.improv`, nothing added that directory to the -walk. Its two fixtures were then loaded only by the Playwright job, where a -missing seed degrades to an empty model and surfaces as a missing-label -failure, not a load error. `shipped_directories` now takes every -`!/*.improv` line, so un-ignoring a directory is what enlists it. +**How the walk got here: two steps, each closing the previous one's holes.** -The precondition is per directory: each derived directory must yield at least -one document. The old whole-walk `>= 2` could not see a directory silently lost. +- **improvise-qlrh.** The test hard-coded `["examples", "web"]`. When + improvise-plcx un-ignored `harness/fixtures/*.improv`, nothing added that + directory. Its two fixtures were then loaded only by the Playwright job, + where a missing seed degrades to an empty model and fails as a missing label, + not a load error. The fix read `!/*.improv` lines from the root + `.gitignore`, with a per-directory precondition. Red at `705c92d`: + `harness/fixtures/tall.improv` replaced with `{"legacy": true}` passed the old + test. +- **improvise-c0aj.** That `.gitignore` reading was a parser for one shape of + one file, so four other ways of shipping a document were missed silently: + - a single-file un-ignore (`!web/demo.improv`); + - a `.improv.gz` anywhere, since `*.improv` never ignores it and so no + un-ignore line names it; + - a `!` line in a nested `.gitignore`; + - a force-added (`add -f`) document. -**Red, measured at `705c92d`:** `harness/fixtures/tall.improv` was replaced with -`{"legacy": true}`, and the old test still passed. **Rivals, each run against -the new test:** + Each now has its own test, which builds the shape in a scratch repository + and asserts the walk lists the document. **All four were red** against the + `.gitignore` walk, each failing at its `walked` assertion with its setup + commands succeeding. Walking the index closes all four by construction and + deletes the parser, because git has already answered what the parser + approximated. -- **That same broken fixture.** Fails with "…/harness/fixtures/tall.improv does - not load". -- **A walk that drops every `harness` path.** Fails the per-directory - precondition, naming `harness/fixtures`. -- **A derivation that matches no `.gitignore` line.** Fails "…must un-ignore at - least one *.improv directory". +**The one new failure mode, answered: no index.** `ls-files` needs a checkout. +In CI (`actions/checkout`) and every worktree it has one. No nix build or +packaging step runs this test; the flake is a dev shell only. Where there is no +index, the walk must not answer "nothing", because an empty walk passes every +per-document assertion vacuously. So it asserts `ls-files` succeeded, and +`a_directory_that_is_not_a_checkout_fails_loudly` pins that. The whole-walk +precondition "at least one document" stays as the second line of defence. -**It matches on the full name, not on `Path::extension()`**: `.improv.gz` is the -same document gzipped and `extension()` calls it `gz`, so matching on the -extension alone would skip it and put back the very hole this test exists to -close. +**Rivals, each run against the new walk:** + +- **The `.improv.gz` pathspec removed.** Fails `a_gzipped_document_is_walked`, + and nothing else. +- **A failed `ls-files` answered with an empty list instead of a panic.** Fails + `a_directory_that_is_not_a_checkout_fails_loudly`. +- **`harness/fixtures/tall.improv` replaced with `{"legacy": true}`.** Fails + `every_shipped_document_loads`, naming the file. + +**It matches on the full name, not on `Path::extension()`.** `.improv.gz` is the +same document gzipped, and `extension()` calls it `gz`. The pathspec lists both +suffixes for the same reason. --- diff --git a/tests/shipped_documents_load.rs b/tests/shipped_documents_load.rs index e8db9d6c..777b1b1b 100644 --- a/tests/shipped_documents_load.rs +++ b/tests/shipped_documents_load.rs @@ -1,71 +1,52 @@ //! Every `.improv` document this repository ships must load (improvise-odbc). -//! Why a DIRECTORY WALK and not a list: `context/test-evidence/tui.md`. +//! "Shipped" is "tracked", read off git's index: `context/test-evidence/tui.md`. use std::path::{Path, PathBuf}; -/// The directories `.gitignore` un-ignores `*.improv` in: the definition of -/// "shipped", read from the one place that states it (improvise-qlrh). -fn shipped_directories(root: &Path) -> Vec { - let gitignore = std::fs::read_to_string(root.join(".gitignore")).expect("a .gitignore"); - gitignore - .lines() - .filter_map(|line| line.trim().strip_prefix('!')) - .filter_map(|pattern| pattern.strip_suffix("/*.improv")) - .map(|dir| root.join(dir)) - .collect() -} - -fn documents_in(dir: &Path) -> Vec { - let mut found: Vec = std::fs::read_dir(dir) - .expect("a shipped-document directory") - .map(|entry| entry.expect("a readable directory entry").path()) - // `extension()` of `x.improv.gz` is `gz`, so matching on the extension - // alone would skip it and put back the hole this test closes. - .filter(|p| { - p.to_str() - .is_some_and(|s| s.ends_with(".improv") || s.ends_with(".improv.gz")) - }) +/// Every tracked `.improv` and `.improv.gz` under `root`. Outside a checkout this +/// panics rather than answering empty (improvise-c0aj). +fn shipped_documents(root: &Path) -> Vec { + let out = std::process::Command::new("git") + .arg("-C") + .arg(root) + .args(["ls-files", "-z", "--", "*.improv", "*.improv.gz"]) + .output() + .expect("git runs: this test reads the index, so it needs a checkout"); + assert!( + out.status.success(), + "git ls-files failed in {}: {}", + root.display(), + String::from_utf8_lossy(&out.stderr) + ); + let mut found: Vec = String::from_utf8(out.stdout) + .expect("UTF-8 paths") + .split('\0') + .filter(|p| !p.is_empty()) + .map(|p| root.join(p)) .collect(); found.sort(); found } -/// Every shipped document under `root`: the walk the test below loads. -fn shipped_documents(root: &Path) -> Vec { - shipped_directories(root) - .iter() - .flat_map(|dir| documents_in(dir)) - .collect() -} - #[test] fn every_shipped_document_loads() { let root = Path::new(env!("CARGO_MANIFEST_DIR")); - let dirs = shipped_directories(root); + let documents = shipped_documents(root); assert!( - !dirs.is_empty(), - "precondition: .gitignore must un-ignore at least one *.improv directory" + !documents.is_empty(), + "precondition: the index must list at least one shipped document" ); - for dir in dirs { - let documents = documents_in(&dir); - // Per directory, so a walk that silently loses one cannot pass (qlrh). + for path in documents { + let workbook = improvise::persistence::load(&path) + .unwrap_or_else(|e| panic!("{} does not load: {e}", path.display())); assert!( - !documents.is_empty(), - "precondition: {} is un-ignored for *.improv but the walk found none there", - dir.display() + workbook.views.contains_key(&workbook.active_view), + "{} loads but its initial view {:?} is not one of {:?}", + path.display(), + workbook.active_view, + workbook.views.keys().collect::>() ); - for path in documents { - let workbook = improvise::persistence::load(&path) - .unwrap_or_else(|e| panic!("{} does not load: {e}", path.display())); - assert!( - workbook.views.contains_key(&workbook.active_view), - "{} loads but its initial view {:?} is not one of {:?}", - path.display(), - workbook.active_view, - workbook.views.keys().collect::>() - ); - } } } @@ -125,10 +106,27 @@ fn a_document_un_ignored_by_a_nested_gitignore_is_walked() { write(root, ".gitignore", "*.improv\n"); write(root, "harness/.gitignore", "!keep.improv\n"); write(root, "harness/keep.improv", "x"); - run_git(root, &["add", ".gitignore", "harness/.gitignore", "harness/keep.improv"]); + run_git( + root, + &[ + "add", + ".gitignore", + "harness/.gitignore", + "harness/keep.improv", + ], + ); assert!(walked(root, "harness/keep.improv")); } +/// An empty walk would pass every per-document assertion vacuously, so a +/// directory git cannot read must stop the test instead. +#[test] +#[should_panic(expected = "git ls-files failed")] +fn a_directory_that_is_not_a_checkout_fails_loudly() { + let not_a_repo = tempfile::tempdir().expect("a temp dir"); + shipped_documents(not_a_repo.path()); +} + #[test] fn a_force_added_document_is_walked() { let repo = scratch_repo();