diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index eef5a97..3093125 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -46,6 +46,12 @@ jobs: run: mise run lint - name: Test run: mise run test + # A second run with color forced on. The tree assertions match unstyled + # text, and `FORCE_COLOR` outranks the TTY check the default run relies on, + # so this is the leg that catches an assertion straddling a style boundary + # before it reaches a contributor whose shell exports the variable. + - name: Test (color forced) + run: mise run test:color # Run the hermetic test suite on Windows and macOS so those targets are # officially supported. Format/lint are platform-agnostic and stay on the Linux diff --git a/AGENTS.md b/AGENTS.md index e8f5f2a..c31d9f9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -27,6 +27,7 @@ Validate changes before committing: ```bash mise run test # correctness (cargo test --workspace) +mise run test:color # the same suite with terminal color forced on mise run fmt:check # formatting mise run lint # clippy -D warnings mise run coverage # coverage (informational, no threshold) diff --git a/crates/dependable/src/output/tree.rs b/crates/dependable/src/output/tree.rs index a3bbadf..a456453 100644 --- a/crates/dependable/src/output/tree.rs +++ b/crates/dependable/src/output/tree.rs @@ -294,10 +294,85 @@ source = "registry+https://x" DependencyGraph::from_resolved(&resolved, &names, &["app".to_owned()]) } + /// `raw` with any terminal styling removed. + /// + /// [`label`] colours through `if_supports_color`, which asks the ambient + /// environment whether stdout can take ANSI — and `FORCE_COLOR`, which a + /// terminal multiplexer or a task runner may well export, answers yes even + /// under a captured test harness. These tests assert on the shape of the + /// tree, never on its colour, so a styled run must not fail them; stripping + /// here is what states that, rather than leaving it to whatever the process + /// happened to inherit. + /// + /// The contract is two escape forms, which is what `owo-colors` emits: + /// + /// - a CSI sequence, `ESC [` then parameter and intermediate bytes then a + /// final byte in `@..=~` — dropped whole, final byte included; + /// - any other escape, treated as the two-character form `ESC` + one byte + /// and dropped whole. + /// + /// A string escape (OSC, DCS, APC …) carries a payload terminated by BEL or + /// `ESC \\` rather than a single byte, so it is *not* in the contract: this + /// would drop its introducer and leave the payload as text. Nothing here + /// emits one; a styling path that starts to (a hyperlink, say) has to teach + /// this function about it. + fn strip_ansi(raw: &str) -> String { + let mut out = String::with_capacity(raw.len()); + let mut chars = raw.chars(); + while let Some(c) = chars.next() { + if c != '\u{1b}' { + out.push(c); + continue; + } + if chars.next() != Some('[') { + continue; + } + for c in chars.by_ref() { + if matches!(c, '@'..='~') { + break; + } + } + } + out + } + + #[test] + fn strip_ansi_drops_a_csi_sequence_whole() { + assert_eq!( + strip_ansi("\u{1b}[36;1mserde\u{1b}[0m v1.0.0"), + "serde v1.0.0" + ); + } + + /// A CSI whose final byte is not `m`: still terminated by the first byte in + /// `@..=~`, because parameter (`0x30..=0x3F`) and intermediate (`0x20..=0x2F`) + /// bytes all sort below that range. + #[test] + fn strip_ansi_ends_a_csi_at_a_final_byte_other_than_m() { + assert_eq!(strip_ansi("a\u{1b}[2Kb\u{1b}[1;31Hc"), "abc"); + } + + /// A two-character escape — `ESC c` (RIS) — loses both characters. + #[test] + fn strip_ansi_drops_a_two_character_escape() { + assert_eq!(strip_ansi("a\u{1b}cb"), "ab"); + } + + /// A lone trailing `ESC` has nothing after it: the iterator ends and the + /// escape is dropped rather than emitted as text. + #[test] + fn strip_ansi_drops_a_lone_trailing_escape() { + assert_eq!(strip_ansi("serde\u{1b}"), "serde"); + } + + #[test] + fn strip_ansi_leaves_unstyled_text_alone() { + assert_eq!(strip_ansi("├── serde v1.0.0 (*)"), "├── serde v1.0.0 (*)"); + } + #[test] fn ascii_marks_workspace_and_dedupe() { - // Color is disabled in the test harness (not a TTY), so labels are plain. - let out = ascii(&sample(), &TreeOptions::default()); + let out = strip_ansi(&ascii(&sample(), &TreeOptions::default())); assert!(out.contains("app v0.1.0 (workspace)")); assert!(out.contains("├── serde v1.0.0")); assert!(out.contains("└── ")); // last-child connector @@ -329,7 +404,7 @@ source = "registry+https://x" #[test] fn ascii_points_a_member_at_its_own_tree() { - let out = ascii(&workspace(), &TreeOptions::default()); + let out = strip_ansi(&ascii(&workspace(), &TreeOptions::default())); assert!( out.contains("└── lib v0.1.0 (workspace) (see root)"), "under `app`, `lib` is a pointer rather than a copy; {out}" @@ -352,7 +427,7 @@ source = "registry+https://x" collapse_roots: false, ..TreeOptions::default() }; - let out = ascii(&workspace(), &opts); + let out = strip_ansi(&ascii(&workspace(), &opts)); assert!(!out.contains("(see root)"), "{out}"); assert_eq!(out.matches("serde v1.0.0").count(), 2, "{out}"); } @@ -364,7 +439,7 @@ source = "registry+https://x" dedupe: true, ..TreeOptions::default() }; - let out = ascii(&sample(), &opts); + let out = strip_ansi(&ascii(&sample(), &opts)); assert!(out.contains("app v0.1.0 (workspace)")); assert!(!out.contains("serde")); } diff --git a/crates/dependable/tests/fixture_tree.rs b/crates/dependable/tests/fixture_tree.rs index 063ec75..2ce5266 100644 --- a/crates/dependable/tests/fixture_tree.rs +++ b/crates/dependable/tests/fixture_tree.rs @@ -1,6 +1,7 @@ //! End-to-end: `dependable tree` over a committed workspace fixture. Fully -//! offline — the graph comes from the fixture's `Cargo.lock`. Piped stdout is -//! not a TTY, so labels are plain (uncolored) and assertable as text. +//! offline — the graph comes from the fixture's `Cargo.lock`. Labels are +//! asserted as plain text, which [`run`] pins the child's environment to make +//! true rather than inferring it from the pipe. use std::path::PathBuf; use std::process::{Command, Output}; @@ -9,9 +10,20 @@ fn fixture() -> PathBuf { PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("tests/fixtures/sample-workspace") } +/// `dependable` with the given arguments, its output left unstyled. +/// +/// A pipe is not a TTY, so the labels come out plain by default — but that is a +/// default, not a guarantee. `FORCE_COLOR` overrides it, and the child inherits +/// whatever exported it: a terminal multiplexer, a task runner, a CI image. The +/// assertions below are about the shape of the tree rather than its colour, so +/// the environment is pinned here instead of assumed, and the tests state the +/// same thing in a coloured terminal as in a bare one. fn run(args: &[&str]) -> Output { Command::new(env!("CARGO_BIN_EXE_dependable")) .args(args) + .env_remove("FORCE_COLOR") + .env_remove("CLICOLOR_FORCE") + .env("NO_COLOR", "1") .output() .expect("run dependable") } diff --git a/hk.pkl b/hk.pkl index e6bd0fb..fac66a5 100644 --- a/hk.pkl +++ b/hk.pkl @@ -22,6 +22,25 @@ local test = new Step { check = "cargo test --workspace" } +// The same suite with color forced on. `FORCE_COLOR` outranks the TTY check inside +// `supports-color`, so this is a second configuration rather than a repeat: it is what +// catches an assertion that matches unstyled text and would otherwise pass here and +// fail for whoever runs the suite from a terminal that exports the variable. +// +// `depends` on the plain run, because hk schedules steps in parallel up to `HK_JOBS` +// and cargo drops its build lock before any test binary starts: two concurrent runs +// share one target directory, and the integration tests keep fixed-name scratch +// fixtures under `CARGO_TARGET_TMPDIR` that they wipe on entry, so they would delete +// each other's. Serializing is enough — `FORCE_COLOR` is in no fingerprint, so the +// second run rebuilds nothing and costs only its own test execution. +local testColor = new Step { + depends = List("test") + check = "cargo test --workspace" + env { + ["FORCE_COLOR"] = "3" + } +} + local coverage = new Step { check = "cargo llvm-cov --workspace" } @@ -61,6 +80,7 @@ hooks { ["fmt"] = fmt ["clippy"] = clippy ["test"] = test + ["test-color"] = testColor ["coverage"] = coverage } } @@ -71,6 +91,7 @@ hooks { ["fmt"] = fmt ["clippy"] = clippy ["test"] = test + ["test-color"] = testColor } } ["fix"] { diff --git a/mise.toml b/mise.toml index bca6759..5afd8b2 100644 --- a/mise.toml +++ b/mise.toml @@ -28,6 +28,24 @@ run = "cargo install --path crates/dependable --locked --force" description = "Run all tests (live network tests are #[ignore]d and skipped)" run = "cargo test --workspace" +# The tree output is styled through `if_supports_color`, which consults the +# environment. The suite asserts on unstyled text, so a run with color forced on +# is the only thing that proves those assertions do not quietly depend on the +# terminal they happen to be run from. `FORCE_COLOR` outranks the TTY check in +# `supports-color`, which is what makes this a real second configuration rather +# than a repeat of the first. +# +# It `depends` on `test` rather than running beside it: both invocations share one +# Cargo target directory, and the CLI integration tests keep fixed-name scratch +# fixtures under `CARGO_TARGET_TMPDIR` that they wipe on entry, so two concurrent +# runs delete each other's. `FORCE_COLOR` is in no fingerprint, so the serialized +# second run rebuilds nothing and costs only its own test execution. +[tasks."test:color"] +description = "Run the test suite with terminal color forced on" +depends = ["test"] +run = "cargo test --workspace" +env = { FORCE_COLOR = "3" } + [tasks."test:live"] description = "Run the live network smoke tests (hits crates.io + OSV)" run = "cargo test --workspace -- --ignored" @@ -57,5 +75,5 @@ description = "Check that commits on this branch follow Conventional Commits" run = "convco check master..HEAD" [tasks.ci] -description = "Run the full local CI gate: format check, lint, and tests" -depends = ["fmt:check", "lint", "test"] +description = "Run the full local CI gate: format check, lint, and tests (both color modes)" +depends = ["fmt:check", "lint", "test", "test:color"]