From c7edddb746fe4c4561de502563c0282cb4f848fc Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Mon, 24 Aug 2026 18:06:58 +0000 Subject: [PATCH 1/2] docs: say what Trips is, and replace the Runner impl count with the shape The trait's doc said it was "implemented once for real processes and once for tests". There are nine impls: ProcessRunner, FakeRunner, and seven test-local wrappers over those two. Three of the wrappers hold a real ProcessRunner and hand it whatever they do not fake -- FakeDevpodRealGit, lifecycle's Devpod, workspace_clone's StubbedLfs -- which is the fact a reader currently learns by grepping and then by reading a fixture. A corrected count would rot the same way, so the doc states the shape instead: one production impl, one shared fake carrying the response table and DevpodMachine, and wrappers over those two, with the grep named as the enumeration. What it does say about the wrappers is the two shapes worth recognising before reading one -- part real (a fake devpod beside a real git) and recorder -- neither of which is a number. The half of that shape which is load-bearing is now asserted rather than claimed: tests/one_seam.rs scans the workspace and fails if anything but ProcessRunner implements Runner outside test code. A second real implementation is not a stale doc, it is a second way to start a process. Proven by adding one and watching it fail. Trips gets the doc #405 settled: it is a recorder, not a third fake devpod. It answers by call index from a list the test handed the constructor, never reads argv to decide anything, and holds no workspace state -- so a corpus row, which is given state -> argv -> exit + then state, has nothing to bind to at either end, and driving it over conformance.json would assert a fixture against itself. The next person asking why it is not in the corpus finds the answer at the definition. devpod.rs's module doc says the same thing from the other side: the two hand-written fakes are the whole population the corpus covers, and the Runner wrappers reach DevpodMachine through FakeRunner rather than being a third. Also Trip::script, which re-parses the argv setup_pass builds: it now says so, and `&self.argv[at + 1]` becomes `.get(at + 1).expect("a --command with a payload")`, so a payload that moved says which of the two assumptions broke instead of panicking on an unnamed index. Closes nothing; #409, under map #406. --- rust/devlaunch-core/src/flows/provision.rs | 29 +++- rust/devlaunch-runner/src/lib.rs | 27 +++- rust/devlaunch-runner/tests/one_seam.rs | 161 +++++++++++++++++++++ rust/devlaunch-test-support/src/devpod.rs | 8 + 4 files changed, 222 insertions(+), 3 deletions(-) create mode 100644 rust/devlaunch-runner/tests/one_seam.rs diff --git a/rust/devlaunch-core/src/flows/provision.rs b/rust/devlaunch-core/src/flows/provision.rs index 8c7b3ba0..a7cc1ed7 100644 --- a/rust/devlaunch-core/src/flows/provision.rs +++ b/rust/devlaunch-core/src/flows/provision.rs @@ -2345,18 +2345,45 @@ fi impl Trip { /// The payload of this trip's `ssh --command`. + /// + /// Re-parses the argv [`setup_pass`] builds, `ssh --command + /// `, and the two expects are the guard on that coupling: rename + /// the flag or move the payload and this fails at the read, saying which of + /// the two assumptions broke, rather than handing the test a neighbouring + /// element to assert against. The shape is not this fixture's invention — + /// `--command` is in `SSH_VALUE_FLAGS` + /// (`devlaunch-test-support/src/devpod.rs`), and + /// `test/fixtures/devpod/conformance.json` carries an `ssh ... --command` + /// row measured against real devpod v0.26.1. fn script(&self) -> &str { let at = self .argv .iter() .position(|arg| arg == "--command") .expect("a trip with a --command"); - &self.argv[at + 1] + self.argv.get(at + 1).expect("a --command with a payload") } } /// Stands in for the one devpod spawn point, recording what was asked of it. /// + /// A recorder, not a third fake devpod, and that is why nothing here belongs in + /// `test/fixtures/devpod/conformance.json`. A fake devpod decides an outcome + /// *from argv*; `record` never reads argv to decide anything — it returns the + /// `call`-th entry of a list the test handed the constructor, and stores the + /// argv only so the assertions can read it back afterwards. It holds no + /// workspaces and has no `Running`/`Stopped` to move between. + /// + /// A corpus row is `given` state → argv → exit + `then` state, so a row written + /// against this has nothing to bind to at either end: there is no state to seed + /// and none to read back, and the exit is whatever the test wrote in the + /// constructor. Driving it over the corpus would assert a fixture against + /// itself, which is the failure the corpus was built to end. + /// + /// The fake devpod the rest of the suite meets is + /// `devlaunch_test_support::devpod::DevpodMachine`, reached through + /// `FakeRunner`, and that one *is* corpus-covered — no third fake escaped it. + /// /// `answers` is consumed one per trip, the last repeating — the three-trip flow /// (probe, transfer, install) needs different answers to different trips, and a /// single number could only play one of them. diff --git a/rust/devlaunch-runner/src/lib.rs b/rust/devlaunch-runner/src/lib.rs index 6f598858..6cedafef 100644 --- a/rust/devlaunch-runner/src/lib.rs +++ b/rust/devlaunch-runner/src/lib.rs @@ -381,8 +381,31 @@ pub enum DetachOutcome { NotStarted(OsFailure), } -/// The one seam. Implemented once for real processes ([`ProcessRunner`]) and -/// once for tests (`devlaunch_test_support::FakeRunner`). +/// The one seam: every process devlaunch starts is started through here. +/// +/// One implementation does real work, [`ProcessRunner`], and it is the only impl +/// outside test code — which is what makes this a seam rather than a habit, and +/// is asserted in `tests/one_seam.rs` rather than left to this sentence. One +/// implementation stands in for it across the suite: +/// `devlaunch_test_support::FakeRunner`, a call recorder, an argv-prefix response +/// table, and the fake devpod behind them (`DevpodMachine`, the one the +/// conformance corpus pins). +/// +/// Tests wrap those two rather than implementing the trait afresh, and the +/// wrappers come and go, so `grep -rn "impl Runner for"` is the enumeration +/// rather than a number written here. Two shapes are worth recognising before +/// reading one: +/// +/// - **Part real.** A wrapper holding a [`ProcessRunner`] beside a fake answers +/// for the programs a unit test must not really run — devpod above all, and +/// docker, whose real daemon would be the developer's own — and hands +/// everything else, git above all, to real processes. That is how a test drives +/// a real repository against a workspace that never existed; +/// `flows::listing`'s `FakeDevpodRealGit` is the one named after the pattern. +/// - **A recorder.** A wrapper that answers by call index rather than from argv +/// is not a fake devpod at all: it plays back a list the test handed it and +/// keeps the argv for the assertions to read. `flows::provision`'s `Trips` is +/// the example, and its doc says why a recorder cannot join the corpus. pub trait Runner { /// Run to completion, reading both streams as text. fn capture(&self, spec: &SpawnSpec) -> Outcome; diff --git a/rust/devlaunch-runner/tests/one_seam.rs b/rust/devlaunch-runner/tests/one_seam.rs new file mode 100644 index 00000000..7f404b26 --- /dev/null +++ b/rust/devlaunch-runner/tests/one_seam.rs @@ -0,0 +1,161 @@ +//! `ProcessRunner` is the only [`Runner`] outside test code, asserted rather than +//! asked for. +//! +//! The trait's doc used to say it was "implemented once for real processes and +//! once for tests", and by the time anyone read that sentence there were nine +//! impls. Rewriting it as a count would rot the same way, so the doc now states a +//! shape — one production impl, one shared fake, wrappers over those two — and +//! this test holds the half of that shape which is load-bearing. A second +//! implementation doing real work is not a doc that has gone stale: it is a second +//! way for devlaunch to start a process, and the seam has stopped being one. +//! +//! The scan is deliberately dumb — an `impl ... for` line in a source file, +//! classified by where it sits — because the alternative is a hand-written list of +//! the wrappers, which is the artefact that rotted in the first place. Everything +//! it cannot see for itself is [`TEST_ONLY_FILES`], where dropping a file is a +//! visible edit rather than an omission. +//! +//! [`Runner`]: devlaunch_runner::Runner + +use std::fs; +use std::path::{Path, PathBuf}; + +/// Files whose every impl is test code but which do not say so in the file: the +/// module is declared `#[cfg(test)] mod ...` somewhere else. One entry today, and +/// a new one arrives as a failure here rather than as silence. +const TEST_ONLY_FILES: &[&str] = &["devlaunch-core/src/testing.rs"]; + +/// The one implementation that may do real work. +const PRODUCTION: &str = "ProcessRunner"; + +/// What an implementation of the seam opens with, once the line is trimmed. +const OPENER: &str = "impl Runner for "; + +/// The cargo workspace root: this crate's directory, one level up. +fn workspace() -> PathBuf { + Path::new(env!("CARGO_MANIFEST_DIR")) + .parent() + .expect("the cargo workspace above this crate") + .to_path_buf() +} + +fn rust_sources(dir: &Path, found: &mut Vec) { + let entries = + fs::read_dir(dir).unwrap_or_else(|why| panic!("reading {}: {why}", dir.display())); + for entry in entries { + let path = entry.expect("a directory entry").path(); + if path.is_dir() { + if path.file_name().is_some_and(|name| name == "target") { + continue; + } + rust_sources(&path, found); + } else if path.extension().is_some_and(|kind| kind == "rs") { + found.push(path); + } + } +} + +/// The type named by an implementation line, without its lifetimes or generics. +fn implementer(line: &str) -> Option<&str> { + let rest = line.trim().strip_prefix(OPENER)?; + let end = rest.find(['<', '{', ' ']).unwrap_or(rest.len()); + Some(rest[..end].trim()) +} + +/// Whether the file opens a top-level `#[cfg(test)]` module inline at `at`. +/// +/// The attribute alone is not the signal: `devlaunch-runner/src/lib.rs` carries +/// `#[cfg(test)] mod tests;` near the top, and the production impl is four hundred +/// lines below it. An *inline* module is the one that swallows what follows. +fn opens_a_test_module(lines: &[&str], at: usize) -> bool { + if lines[at] != "#[cfg(test)]" { + return false; + } + lines + .get(at + 1) + .is_some_and(|next| next.contains("mod ") && next.ends_with('{')) +} + +/// Whether the implementation at `at` is test code. +/// +/// Three ways to be: living in a test-support crate, living under a `tests/` +/// directory, or sitting below the `#[cfg(test)] mod tests {` its file ends with — +/// which is where every wrapper a unit test defines sits. +fn is_test_code(relative: &str, lines: &[&str], at: usize) -> bool { + // Read from the workspace-relative path, not the absolute one: whatever + // directories this checkout happens to live under are not evidence about the + // code. + let test_tree = relative + .split('/') + .any(|part| part.ends_with("-test-support") || part == "tests"); + test_tree + || TEST_ONLY_FILES.contains(&relative) + || (0..at).any(|line| opens_a_test_module(lines, line)) +} + +/// Every implementation of the seam in the workspace: relative path, type, and +/// whether it is test code. +fn implementations() -> Vec<(String, String, bool)> { + let root = workspace(); + let mut sources = Vec::new(); + rust_sources(&root, &mut sources); + sources.sort(); + + let mut found = Vec::new(); + for path in sources { + let text = fs::read_to_string(&path) + .unwrap_or_else(|why| panic!("reading {}: {why}", path.display())); + let lines: Vec<&str> = text.lines().collect(); + let relative = path + .strip_prefix(&root) + .expect("a path under the workspace") + .to_string_lossy() + .into_owned(); + for (at, line) in lines.iter().enumerate() { + if let Some(name) = implementer(line) { + let test = is_test_code(&relative, &lines, at); + found.push((relative.clone(), name.to_string(), test)); + } + } + } + found +} + +#[test] +fn the_scan_finds_the_implementations_it_is_meant_to_judge() { + let found = implementations(); + assert!( + found + .iter() + .any(|(path, name, _)| path.contains("devlaunch-runner") && name == PRODUCTION), + "the scan did not find {PRODUCTION}, so it is not reading this workspace: \ + {found:#?}" + ); + assert!( + found.iter().filter(|(_, _, test)| *test).count() >= 2, + "the scan found no test implementations, which no version of this tree has \ + been true of: {found:#?}" + ); +} + +#[test] +fn only_process_runner_does_real_work() { + let production: Vec<_> = implementations() + .into_iter() + .filter(|(_, _, test)| !*test) + .map(|(path, name, _)| format!("{name} ({path})")) + .collect(); + assert_eq!( + production.len(), + 1, + "Runner is the one seam onto the OS, and {PRODUCTION} is meant to be the \ + only implementation of it outside test code. Found: {production:#?}. A \ + second real implementation is a second way to start a process; if that is \ + deliberate, the trait's doc in src/lib.rs says otherwise and needs the \ + edit first." + ); + assert!( + production[0].starts_with(PRODUCTION), + "the one production Runner is no longer {PRODUCTION}: {production:#?}" + ); +} diff --git a/rust/devlaunch-test-support/src/devpod.rs b/rust/devlaunch-test-support/src/devpod.rs index 21973c2b..b458d5bf 100644 --- a/rust/devlaunch-test-support/src/devpod.rs +++ b/rust/devlaunch-test-support/src/devpod.rs @@ -24,6 +24,14 @@ //! --ignore-not-found` that refused survived here for months after the shim was //! fixed for it. A behaviour change in here belongs in a corpus row first; see //! the `conformance` module below. +//! +//! Those two are the whole population the corpus has to cover: one per language, +//! and nothing else in the tree decides a devpod outcome from argv. The +//! `Runner` wrappers the unit tests define reach *this* machine through +//! [`FakeRunner`](crate::FakeRunner) rather than faking devpod themselves, and +//! the ones that answer without consulting argv at all — `flows::provision`'s +//! `Trips` — are recorders, which is why they are absent here rather than +//! missing. use std::collections::{BTreeMap, BTreeSet}; use std::path::Path; From 1dead6dc76e5907f8732a9014778ad4c16163686 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Mon, 24 Aug 2026 18:49:57 +0000 Subject: [PATCH 2/2] fix: a line was the wrong unit for the one-seam guard Review found two spellings that walked past it and one that broke it, all three reproduced. `impl<'a> Runner for X` and `impl devlaunch_runner::Runner for X` dropped into devlaunch-core/src/ as production files left the guard green: it matched the literal text "impl Runner for " at the start of a trimmed line, and neither spelling starts that way. The first is the wrapper shape the trait's own doc invites; the second is how any crate that has not imported the trait names it, which is the likeliest form of a genuinely new second seam. Meanwhile one #[allow(...)] between #[cfg(test)] and mod tests { made every wrapper in that module read as production, because the gate was only honoured when the module line was literally the next one. Six files here would have broken on that. The instrument is still "read the source" -- the alternative is a hand-written list of the wrappers, which is the artefact that rotted in the first place, and nothing on stable enumerates trait impls from the compiler. What changed is the unit: a token rather than a line. Comments and literals are blanked out first, preserving offsets, so a brace in a comment cannot unbalance anything and a quoted "impl Runner for " is not an impl; the trait is matched by the last segment of whatever path names it, after optional generics; and #[cfg(test)] gates a brace-matched region, skipping any attributes between it and the item. That last one closes a fourth hole nobody had reached: the old scan called everything below an inline test module test code, so the way past the guard was to write underneath it. Rejected: a syn dev-dependency, which is exact but pulls a second copy of syn with the "full" feature into every build of this workspace for one test, and sealing the trait, which cannot tell a production impl in devlaunch-core from a #[cfg(test)] one in the same crate without exporting the seal. Proof is seven cases running against scan() with source as a string, so no probe file has to be dropped in the tree to exercise them. All seven were run against the old line logic first: four red -- both bypasses, the false positive, and the below-the-module hole. End to end, the reviewer's own probe2/probe3 now fail the guard by name and probe4 does not. Also from the review: - #409 asks to "say the structure and name the three routers". The PR body claimed the ticket steered away from naming them; reading it again, it does not -- the same sentence that forbids the count asks for the names. So the "Part real" bullet names FakeDevpodRealGit, lifecycle's Devpod and workspace_clone's StubbedLfs, and says which of the three not to read by analogy: StubbedLfs routes on the `git lfs` subcommand rather than the program, and only its capture reaches ProcessRunner. The bullet said such a wrapper hands "everything else" to real processes, which was true of two of the three. - The doc's recommended `grep -rn "impl Runner for"` returned 11 lines for 9 impls, this PR having added two of the false hits -- self-refuting for a change about enumerations that rot. The enumeration is now named as the scan, which prints every impl with its file and verdict, with the grep kept as the eyeball version and marked as over-reporting. - devpod.rs claimed "nothing else in the tree decides a devpod outcome from argv", which the response table three lines above it does. It now says why the table is outside the corpus rather than pretending it is not an argv reader: a scripted entry is one test saying what it wants back, not a claim about real devpod. - The scan skips a build directory by CACHEDIR.TAG as well as by the name `target`, so a moved CARGO_TARGET_DIR cannot feed generated sources into it. #409, under map #406. --- rust/devlaunch-runner/src/lib.rs | 21 +- rust/devlaunch-runner/tests/one_seam.rs | 640 +++++++++++++++++++--- rust/devlaunch-test-support/src/devpod.rs | 18 +- 3 files changed, 602 insertions(+), 77 deletions(-) diff --git a/rust/devlaunch-runner/src/lib.rs b/rust/devlaunch-runner/src/lib.rs index 6cedafef..c5bb4c3f 100644 --- a/rust/devlaunch-runner/src/lib.rs +++ b/rust/devlaunch-runner/src/lib.rs @@ -392,16 +392,23 @@ pub enum DetachOutcome { /// conformance corpus pins). /// /// Tests wrap those two rather than implementing the trait afresh, and the -/// wrappers come and go, so `grep -rn "impl Runner for"` is the enumeration -/// rather than a number written here. Two shapes are worth recognising before -/// reading one: +/// wrappers come and go, so the enumeration is the scan in `tests/one_seam.rs`, +/// which lists every impl in the workspace with its file and its verdict, rather +/// than a number written here. (`grep -rn "impl Runner for"` is the eyeball +/// version and over-reports: this sentence matches it, and so do that test's own +/// fixtures.) Two shapes are worth recognising before reading one: /// /// - **Part real.** A wrapper holding a [`ProcessRunner`] beside a fake answers /// for the programs a unit test must not really run — devpod above all, and -/// docker, whose real daemon would be the developer's own — and hands -/// everything else, git above all, to real processes. That is how a test drives -/// a real repository against a workspace that never existed; -/// `flows::listing`'s `FakeDevpodRealGit` is the one named after the pattern. +/// docker, whose real daemon would be the developer's own — and hands it +/// whatever they do not fake, git above all. That is how a test drives a real +/// repository against a workspace that never existed. Three do this today, and +/// they are worth naming because a reader otherwise finds them by grepping: +/// `flows::listing`'s `FakeDevpodRealGit`, which is named after the pattern, +/// `flows::lifecycle`'s `Devpod`, and `flows::workspace_clone`'s `StubbedLfs`. +/// The last is the one to read carefully rather than by analogy: it routes on +/// the `git lfs` subcommand rather than on the program, and only its `capture` +/// reaches `ProcessRunner` at all. /// - **A recorder.** A wrapper that answers by call index rather than from argv /// is not a fake devpod at all: it plays back a list the test handed it and /// keeps the argv for the assertions to read. `flows::provision`'s `Trips` is diff --git a/rust/devlaunch-runner/tests/one_seam.rs b/rust/devlaunch-runner/tests/one_seam.rs index 7f404b26..be11d547 100644 --- a/rust/devlaunch-runner/tests/one_seam.rs +++ b/rust/devlaunch-runner/tests/one_seam.rs @@ -9,11 +9,31 @@ //! implementation doing real work is not a doc that has gone stale: it is a second //! way for devlaunch to start a process, and the seam has stopped being one. //! -//! The scan is deliberately dumb — an `impl ... for` line in a source file, -//! classified by where it sits — because the alternative is a hand-written list of -//! the wrappers, which is the artefact that rotted in the first place. Everything -//! it cannot see for itself is [`TEST_ONLY_FILES`], where dropping a file is a -//! visible edit rather than an omission. +//! The instrument is still "read the source", because the alternative is a +//! hand-written list of the wrappers, which is the artefact that rotted in the +//! first place. What changed is the unit. A line-at-a-time scan matching the +//! literal text `impl Runner for ` reads three ordinary spellings wrong, and every +//! one of the three was demonstrated against the first version of this file: +//! +//! - `impl Runner for Timed` does not begin with that text, so a +//! generic wrapper — the exact shape the trait's doc invites — walked past it. +//! - `impl devlaunch_runner::Runner for X` does not either, which is how the trait +//! is named from any crate that has not imported it: the likeliest spelling of a +//! genuinely new second seam. +//! - `#[cfg(test)]` was only read as a gate when `mod tests {` was the very next +//! line, so one `#[allow(...)]` between them turned every wrapper in that module +//! into a reported production impl. +//! +//! So the unit is now a token rather than a line: comments and literals are +//! blanked out first — which is also why the doc sentence quoting the grep, and +//! the fixtures below, do not count as implementations — and `#[cfg(test)]` gates +//! a brace-matched region rather than the line beneath it. That last one closes a +//! fourth hole nobody had reached yet: the old scan called *everything* below an +//! inline test module test code, including whatever was written after the module +//! closed. +//! +//! Everything the scan cannot see for itself is [`TEST_ONLY_FILES`], where +//! dropping a file is a visible edit rather than an omission. //! //! [`Runner`]: devlaunch_runner::Runner @@ -28,8 +48,378 @@ const TEST_ONLY_FILES: &[&str] = &["devlaunch-core/src/testing.rs"]; /// The one implementation that may do real work. const PRODUCTION: &str = "ProcessRunner"; -/// What an implementation of the seam opens with, once the line is trimmed. -const OPENER: &str = "impl Runner for "; +/// The last segment of the trait's path, whatever the impl calls it by. +const SEAM: &[u8] = b"Runner"; + +/// An implementation of the seam: what it implements the seam for, the file it +/// was read out of, and whether the scan judged it test code. +struct Found { + path: String, + name: String, + test: bool, +} + +// --------------------------------------------------------------------------- +// Blanking out what is not code. +// --------------------------------------------------------------------------- + +fn is_ident_byte(byte: u8) -> bool { + byte.is_ascii_alphanumeric() || byte == b'_' +} + +fn blank(out: &mut [u8], from: usize, to: usize) { + let to = to.min(out.len()); + for byte in &mut out[from..to] { + if *byte != b'\n' { + *byte = b' '; + } + } +} + +/// The source with every comment, string and char literal replaced by spaces. +/// +/// Offsets and line breaks survive, so the result can be scanned as if it were the +/// file. Doing this first is what makes the rest of the scan safe to write with +/// byte matching: a brace inside a comment cannot unbalance a `#[cfg(test)]` +/// region, and `"impl Runner for "` written as a string — this file used to hold +/// exactly that constant, and the trait's doc quotes the grep — is not an impl. +fn code_only(text: &str) -> Vec { + let src = text.as_bytes(); + let mut out = src.to_vec(); + let mut at = 0; + while at < src.len() { + let start = at; + match src[at] { + b'/' if src.get(at + 1) == Some(&b'/') => { + while at < src.len() && src[at] != b'\n' { + at += 1; + } + blank(&mut out, start, at); + } + b'/' if src.get(at + 1) == Some(&b'*') => { + at += 2; + let mut depth = 1usize; + while at < src.len() && depth > 0 { + if src[at] == b'/' && src.get(at + 1) == Some(&b'*') { + depth += 1; + at += 2; + } else if src[at] == b'*' && src.get(at + 1) == Some(&b'/') { + depth -= 1; + at += 2; + } else { + at += 1; + } + } + blank(&mut out, start, at); + } + // A raw string, `r"..."` or `r#"..."#`, but not the raw identifier + // `r#type` and not the `r` at the end of somebody's name. + b'r' if start == 0 || !is_ident_byte(src[start - 1]) => { + let mut hashes = 0; + let mut probe = at + 1; + while src.get(probe) == Some(&b'#') { + hashes += 1; + probe += 1; + } + if src.get(probe) != Some(&b'"') { + at += 1; + continue; + } + at = probe + 1; + loop { + match src.get(at) { + None => break, + Some(&b'"') => { + let closed = (1..=hashes).all(|n| src.get(at + n) == Some(&b'#')); + at += 1; + if closed { + at += hashes; + break; + } + } + Some(_) => at += 1, + } + } + blank(&mut out, start, at); + } + b'"' => { + at += 1; + while at < src.len() && src[at] != b'"' { + at += if src[at] == b'\\' { 2 } else { 1 }; + } + at = (at + 1).min(src.len()); + blank(&mut out, start, at); + } + // `'a'` is a char literal; `'a` is a lifetime. Only the first is + // blanked, and the difference is whether a closing quote follows the + // one character. + b'\'' => { + let escaped = src.get(at + 1) == Some(&b'\\'); + if escaped || src.get(at + 2) == Some(&b'\'') { + at += 1; + while at < src.len() && src[at] != b'\'' { + at += if src[at] == b'\\' { 2 } else { 1 }; + } + at = (at + 1).min(src.len()); + blank(&mut out, start, at); + } else { + at += 1; + } + } + _ => at += 1, + } + } + out +} + +// --------------------------------------------------------------------------- +// Walking blanked source. +// --------------------------------------------------------------------------- + +fn skip_space(src: &[u8], mut at: usize) -> usize { + while at < src.len() && src[at].is_ascii_whitespace() { + at += 1; + } + at +} + +/// The offset just past the balanced `<...>` opening at `at`. +/// +/// `->` is stepped over whole, so a bound like `R: Fn() -> T` does not close the +/// parameter list early. +fn past_angles(src: &[u8], mut at: usize) -> usize { + let mut depth = 0usize; + while at < src.len() { + match src[at] { + b'-' if src.get(at + 1) == Some(&b'>') => at += 1, + b'<' => depth += 1, + b'>' => { + depth -= 1; + if depth == 0 { + return at + 1; + } + } + _ => {} + } + at += 1; + } + at +} + +/// The offset of the `}` closing the `{` at `at`. +fn closing_brace(src: &[u8], mut at: usize) -> usize { + let mut depth = 0usize; + while at < src.len() { + match src[at] { + b'{' => depth += 1, + b'}' => { + depth -= 1; + if depth == 0 { + return at; + } + } + _ => {} + } + at += 1; + } + src.len() +} + +/// The end of the identifier starting at `at`, if one does. +fn ident_end(src: &[u8], at: usize) -> Option { + let mut end = at; + while end < src.len() && is_ident_byte(src[end]) { + end += 1; + } + (end > at).then_some(end) +} + +/// A `foo::bar::Baz` path starting at `at`: where it ends, and its last segment. +fn path(src: &[u8], mut at: usize) -> Option<(usize, &[u8])> { + if src.get(at) == Some(&b':') && src.get(at + 1) == Some(&b':') { + at += 2; + } + let mut end = ident_end(src, at)?; + let mut last = &src[at..end]; + while src.get(end) == Some(&b':') && src.get(end + 1) == Some(&b':') { + let from = end + 2; + end = ident_end(src, from)?; + last = &src[from..end]; + } + Some((end, last)) +} + +/// Whether `word` sits at `at` as a whole token. +fn keyword_at(src: &[u8], at: usize, word: &[u8]) -> bool { + src.len() >= at + word.len() + && &src[at..at + word.len()] == word + && !src.get(at + word.len()).copied().is_some_and(is_ident_byte) +} + +/// The `#[...]` attribute opening at `at`, if one does: the offset just past its +/// closing bracket. +fn past_attribute(src: &[u8], at: usize) -> Option { + let mut probe = at + 1; + if src.get(probe) == Some(&b'!') { + probe += 1; + } + if src.get(probe) != Some(&b'[') { + return None; + } + let mut depth = 0usize; + while probe < src.len() { + match src[probe] { + b'[' => depth += 1, + b']' => { + depth -= 1; + if depth == 0 { + return Some(probe + 1); + } + } + _ => {} + } + probe += 1; + } + None +} + +/// Whether an attribute's text gates on `test`. +/// +/// Read off blanked source, so `#[cfg(feature = "test-support")]` cannot match: +/// the only `test` left in a literal is one nobody wrote as a string. +fn gates_on_test(attribute: &[u8]) -> bool { + let has = |word: &[u8]| { + (0..attribute.len()).any(|at| { + keyword_at(attribute, at, word) && (at == 0 || !is_ident_byte(attribute[at - 1])) + }) + }; + has(b"cfg") && has(b"test") +} + +/// The byte ranges this file puts behind `#[cfg(test)]`. +/// +/// A gate covers the item it is written on, from the attribute to the closing +/// brace of that item's body — the module, the impl, the function. Attributes +/// between the gate and the item are skipped rather than ending the search, which +/// is the whole of the false positive this replaced: `#[cfg(test)]` followed by +/// `#[allow(...)]` followed by `mod tests {` is still a test module. A gate on a +/// declaration with no body (`#[cfg(test)] mod tests;`) covers nothing here — the +/// file it names is judged on its own, or listed in [`TEST_ONLY_FILES`]. +fn test_gated(src: &[u8]) -> Vec<(usize, usize)> { + let mut gated = Vec::new(); + let mut at = 0; + while at < src.len() { + if src[at] != b'#' { + at += 1; + continue; + } + let Some(mut after) = past_attribute(src, at) else { + at += 1; + continue; + }; + if gates_on_test(&src[at..after]) { + loop { + let next = skip_space(src, after); + match src.get(next) { + Some(&b'#') => match past_attribute(src, next) { + Some(past) => after = past, + None => break, + }, + _ => break, + } + } + let mut item = skip_space(src, after); + while item < src.len() && src[item] != b'{' && src[item] != b';' { + item += 1; + } + if src.get(item) == Some(&b'{') { + gated.push((at, closing_brace(src, item))); + } + } + at = after; + } + gated +} + +/// Every `impl ... Runner for Type` in blanked source: where it starts, and the +/// name of the type it is written for. +/// +/// The trait may be reached by any path (`Runner`, `devlaunch_runner::Runner`, +/// `crate::Runner`) and the impl may carry any generics, which is the pair of +/// bypasses this replaced. An inherent `impl Type` has no `for` and is skipped. +fn implementations_in(src: &[u8]) -> Vec<(usize, String)> { + let mut found = Vec::new(); + let mut at = 0; + while at < src.len() { + if !keyword_at(src, at, b"impl") || (at > 0 && is_ident_byte(src[at - 1])) { + at += 1; + continue; + } + let opens = at; + at += 4; + let mut probe = skip_space(src, at); + if src.get(probe) == Some(&b'<') { + probe = skip_space(src, past_angles(src, probe)); + } + let Some((after_trait, segment)) = path(src, probe) else { + continue; + }; + if segment != SEAM { + continue; + } + let mut probe = skip_space(src, after_trait); + if src.get(probe) == Some(&b'<') { + probe = skip_space(src, past_angles(src, probe)); + } + if !keyword_at(src, probe, b"for") { + continue; + } + probe = skip_space(src, probe + 3); + while src.get(probe) == Some(&b'&') || src.get(probe) == Some(&b'\'') { + probe = skip_space(src, probe + 1); + if keyword_at(src, probe, b"mut") { + probe = skip_space(src, probe + 3); + } + while probe < src.len() && is_ident_byte(src[probe]) { + probe += 1; + } + probe = skip_space(src, probe); + } + if let Some((_, name)) = path(src, probe) { + found.push((opens, String::from_utf8_lossy(name).into_owned())); + } + } + found +} + +/// Every implementation of the seam in one file, classified. +/// +/// Three ways to be test code: living in a test-support crate, living under a +/// `tests/` directory or [`TEST_ONLY_FILES`], or sitting inside a `#[cfg(test)]` +/// item — which is where every wrapper a unit test defines sits. +fn scan(relative: &str, text: &str) -> Vec { + let src = code_only(text); + let gated = test_gated(&src); + // Read from the workspace-relative path, not the absolute one: whatever + // directories this checkout happens to live under are not evidence about the + // code. + let test_tree = relative + .split('/') + .any(|part| part.ends_with("-test-support") || part == "tests") + || TEST_ONLY_FILES.contains(&relative); + implementations_in(&src) + .into_iter() + .map(|(at, name)| Found { + path: relative.to_owned(), + name, + test: test_tree || gated.iter().any(|&(from, to)| (from..=to).contains(&at)), + }) + .collect() +} + +// --------------------------------------------------------------------------- +// The workspace. +// --------------------------------------------------------------------------- /// The cargo workspace root: this crate's directory, one level up. fn workspace() -> PathBuf { @@ -45,7 +435,12 @@ fn rust_sources(dir: &Path, found: &mut Vec) { for entry in entries { let path = entry.expect("a directory entry").path(); if path.is_dir() { - if path.file_name().is_some_and(|name| name == "target") { + // `target` by name, and by cargo's own marker, so a build directory + // that `CARGO_TARGET_DIR` moved or renamed does not feed generated + // sources into the scan. + if path.file_name().is_some_and(|name| name == "target") + || path.join("CACHEDIR.TAG").is_file() + { continue; } rust_sources(&path, found); @@ -55,47 +450,8 @@ fn rust_sources(dir: &Path, found: &mut Vec) { } } -/// The type named by an implementation line, without its lifetimes or generics. -fn implementer(line: &str) -> Option<&str> { - let rest = line.trim().strip_prefix(OPENER)?; - let end = rest.find(['<', '{', ' ']).unwrap_or(rest.len()); - Some(rest[..end].trim()) -} - -/// Whether the file opens a top-level `#[cfg(test)]` module inline at `at`. -/// -/// The attribute alone is not the signal: `devlaunch-runner/src/lib.rs` carries -/// `#[cfg(test)] mod tests;` near the top, and the production impl is four hundred -/// lines below it. An *inline* module is the one that swallows what follows. -fn opens_a_test_module(lines: &[&str], at: usize) -> bool { - if lines[at] != "#[cfg(test)]" { - return false; - } - lines - .get(at + 1) - .is_some_and(|next| next.contains("mod ") && next.ends_with('{')) -} - -/// Whether the implementation at `at` is test code. -/// -/// Three ways to be: living in a test-support crate, living under a `tests/` -/// directory, or sitting below the `#[cfg(test)] mod tests {` its file ends with — -/// which is where every wrapper a unit test defines sits. -fn is_test_code(relative: &str, lines: &[&str], at: usize) -> bool { - // Read from the workspace-relative path, not the absolute one: whatever - // directories this checkout happens to live under are not evidence about the - // code. - let test_tree = relative - .split('/') - .any(|part| part.ends_with("-test-support") || part == "tests"); - test_tree - || TEST_ONLY_FILES.contains(&relative) - || (0..at).any(|line| opens_a_test_module(lines, line)) -} - -/// Every implementation of the seam in the workspace: relative path, type, and -/// whether it is test code. -fn implementations() -> Vec<(String, String, bool)> { +/// Every implementation of the seam in the workspace. +fn implementations() -> Vec { let root = workspace(); let mut sources = Vec::new(); rust_sources(&root, &mut sources); @@ -105,36 +461,194 @@ fn implementations() -> Vec<(String, String, bool)> { for path in sources { let text = fs::read_to_string(&path) .unwrap_or_else(|why| panic!("reading {}: {why}", path.display())); - let lines: Vec<&str> = text.lines().collect(); let relative = path .strip_prefix(&root) .expect("a path under the workspace") .to_string_lossy() .into_owned(); - for (at, line) in lines.iter().enumerate() { - if let Some(name) = implementer(line) { - let test = is_test_code(&relative, &lines, at); - found.push((relative.clone(), name.to_string(), test)); - } - } + found.extend(scan(&relative, &text)); } found } +fn describe(found: &[Found]) -> Vec { + found + .iter() + .map(|one| { + let kind = if one.test { "test" } else { "production" }; + format!("{} ({}, {kind})", one.name, one.path) + }) + .collect() +} + +// --------------------------------------------------------------------------- +// The scanner, against the spellings that broke the line-at-a-time version. +// --------------------------------------------------------------------------- + +/// What the scanner made of one file's worth of source, as `name/production` or +/// `name/test`, so a case can be written as one line. +fn verdicts(relative: &str, text: &str) -> Vec { + scan(relative, text) + .into_iter() + .map(|one| { + format!( + "{}/{}", + one.name, + if one.test { "test" } else { "production" } + ) + }) + .collect() +} + +#[test] +fn a_generic_wrapper_is_an_implementation() { + // The shape the trait's doc invites, and the one the line scan missed: the + // text does not begin `impl Runner for `. + assert_eq!( + verdicts( + "devlaunch-core/src/probe.rs", + "pub struct Sneaky2<'a>(&'a str);\nimpl<'a> Runner for Sneaky2<'a> {}\n", + ), + ["Sneaky2/production"] + ); + assert_eq!( + verdicts( + "devlaunch-core/src/probe.rs", + "impl Runner for Timed {}\n", + ), + ["Timed/production"] + ); +} + +#[test] +fn a_trait_named_by_its_path_is_the_same_trait() { + // The spelling any crate that has not imported the trait would use, which is + // the likeliest form of a genuinely new second seam. + assert_eq!( + verdicts( + "devlaunch-core/src/probe.rs", + "impl devlaunch_runner::Runner for Sneaky3 {}\n", + ), + ["Sneaky3/production"] + ); + assert_eq!( + verdicts("dl/src/probe.rs", "impl crate::Runner for Sneaky4 {}\n"), + ["Sneaky4/production"] + ); +} + +#[test] +fn an_attribute_between_the_gate_and_the_module_is_still_the_gate() { + // The false positive: one `#[allow(...)]` used to turn every wrapper in the + // module into a reported production impl, accusing its author of adding a + // second seam. + assert_eq!( + verdicts( + "devlaunch-core/src/probe.rs", + "#[cfg(test)]\n#[allow(clippy::too_many_lines)]\nmod tests {\n struct LocalFake;\n impl Runner for LocalFake {}\n}\n", + ), + ["LocalFake/test"] + ); + assert_eq!( + verdicts( + "devlaunch-core/src/probe.rs", + "#[cfg(any(test, feature = \"x\"))]\nmod tests {\n impl Runner for LocalFake {}\n}\n", + ), + ["LocalFake/test"] + ); + assert_eq!( + verdicts( + "devlaunch-core/src/probe.rs", + "#[cfg(test)]\nimpl Runner for LocalFake {}\n", + ), + ["LocalFake/test"] + ); +} + +#[test] +fn the_gate_ends_where_the_module_does() { + // The hole nobody had reached: everything below an inline test module used to + // read as test code, so the way past the guard was to write underneath it. + assert_eq!( + verdicts( + "devlaunch-core/src/probe.rs", + "#[cfg(test)]\nmod tests {\n impl Runner for LocalFake {}\n}\nimpl Runner for Afterwards {}\n", + ), + ["LocalFake/test", "Afterwards/production"] + ); +} + +#[test] +fn a_gate_on_a_declaration_swallows_nothing() { + // `devlaunch-runner/src/lib.rs` really is written this way, with the + // production impl hundreds of lines below the declaration. + assert_eq!( + verdicts( + "devlaunch-runner/src/lib.rs", + "#[cfg(test)]\nmod tests;\n\nimpl Runner for ProcessRunner {}\n", + ), + ["ProcessRunner/production"] + ); +} + +#[test] +fn only_code_counts() { + // Both of these are in this tree: the trait's doc names the grep, and a test + // fixture spells an impl out as a string. Neither is an implementation, and a + // scan over raw text calls both one. + assert!( + verdicts( + "devlaunch-runner/src/lib.rs", + "/// `grep -rn \"impl Runner for\"` is the enumeration.\npub trait Runner {}\n", + ) + .is_empty() + ); + assert!( + verdicts( + "devlaunch-core/src/probe.rs", + "const OPENER: &str = \"impl Runner for \";\n", + ) + .is_empty() + ); + assert!( + verdicts( + "devlaunch-core/src/probe.rs", + "/* impl Runner for Commented {} */\nimpl Something for Real {}\n", + ) + .is_empty() + ); +} + +#[test] +fn an_inherent_impl_is_not_an_implementation_of_the_seam() { + assert!( + verdicts( + "devlaunch-runner/src/lib.rs", + "impl Runner {}\nimpl ProcessRunner {}\nimpl Debug for Runner {}\n", + ) + .is_empty() + ); +} + +// --------------------------------------------------------------------------- +// The workspace, against the shape the trait's doc states. +// --------------------------------------------------------------------------- + #[test] fn the_scan_finds_the_implementations_it_is_meant_to_judge() { let found = implementations(); assert!( found .iter() - .any(|(path, name, _)| path.contains("devlaunch-runner") && name == PRODUCTION), - "the scan did not find {PRODUCTION}, so it is not reading this workspace: \ - {found:#?}" + .any(|one| one.path.contains("devlaunch-runner") && one.name == PRODUCTION), + "the scan did not find {PRODUCTION}, so it is not reading this workspace: {:#?}", + describe(&found) ); assert!( - found.iter().filter(|(_, _, test)| *test).count() >= 2, + found.iter().filter(|one| one.test).count() >= 2, "the scan found no test implementations, which no version of this tree has \ - been true of: {found:#?}" + been true of: {:#?}", + describe(&found) ); } @@ -142,8 +656,8 @@ fn the_scan_finds_the_implementations_it_is_meant_to_judge() { fn only_process_runner_does_real_work() { let production: Vec<_> = implementations() .into_iter() - .filter(|(_, _, test)| !*test) - .map(|(path, name, _)| format!("{name} ({path})")) + .filter(|one| !one.test) + .map(|one| format!("{} ({})", one.name, one.path)) .collect(); assert_eq!( production.len(), diff --git a/rust/devlaunch-test-support/src/devpod.rs b/rust/devlaunch-test-support/src/devpod.rs index b458d5bf..dc4e53c2 100644 --- a/rust/devlaunch-test-support/src/devpod.rs +++ b/rust/devlaunch-test-support/src/devpod.rs @@ -25,13 +25,17 @@ //! fixed for it. A behaviour change in here belongs in a corpus row first; see //! the `conformance` module below. //! -//! Those two are the whole population the corpus has to cover: one per language, -//! and nothing else in the tree decides a devpod outcome from argv. The -//! `Runner` wrappers the unit tests define reach *this* machine through -//! [`FakeRunner`](crate::FakeRunner) rather than faking devpod themselves, and -//! the ones that answer without consulting argv at all — `flows::provision`'s -//! `Trips` — are recorders, which is why they are absent here rather than -//! missing. +//! Those two are the whole population the corpus has to cover, one per language, +//! and the reason is what each of the other argv readers is. The `Runner` +//! wrappers the unit tests define reach *this* machine through +//! [`FakeRunner`](crate::FakeRunner) rather than faking devpod themselves; the +//! ones that answer without consulting argv at all — `flows::provision`'s +//! `Trips` — are recorders, absent here rather than missing. The response table +//! above is the third argv reader, and it is deliberately outside the corpus: a +//! scripted entry is one test saying what it wants back from one call, not a +//! claim about what real devpod does, and pinning it against a fixture would +//! freeze the exception rather than the behaviour. What the corpus holds is +//! everything that answers *as devpod*. use std::collections::{BTreeMap, BTreeSet}; use std::path::Path;