diff --git a/docs/adr-006-directory-based-module-layout.md b/docs/adr-006-directory-based-module-layout.md new file mode 100644 index 000000000..490373a35 --- /dev/null +++ b/docs/adr-006-directory-based-module-layout.md @@ -0,0 +1,164 @@ +# Architectural decision record (ADR) 006: directory-based module layout + +## Status + +Accepted (2026-10-16): Adopt `mod.rs`-rooted directory modules, and extract an +inline test module that outgrows the 400-line budget into a sibling file wired +with `#[cfg(test)] #[path = "..."] mod ...;`. + +## Date + +2026-10-16. + +## Context and problem statement + +`AGENTS.md` states that no single code file may be longer than 400 lines. The +rule is a readability constraint rather than a compile-time one: a module of +that length has stopped being reviewable in one sitting, so growth past the +limit is answered by decomposition, not by raising the threshold. + +Rust offers two ways to turn a module file into a directory. The first is a +directory whose root file is named `mod.rs`. The second is a *self-named +sibling*: `install_flow.rs` sits next to an `install_flow/` directory that +holds its children. Both are legal, and the choice is not visible to the +compiler's semantics — only to the reader who has to find a module's source. + +A second and larger source of growth is the inline test module. Whitaker +colocates unit tests with the code they exercise, and a +`#[cfg(test)] mod tests { … }` block grows faster than the production code +above it, because every edge case adds an arm. Several installer modules had +reached the point where the test block alone carried the file past the budget, +which means the budget could not be met without either weakening the tests or +moving them. + +The decision is how to structure modules at the point they pass the budget, and +how to move a test module without disturbing the tests themselves. + +## Decision drivers + +- Keep every module within the 400-line budget that `AGENTS.md` sets. +- Keep a structural change purely structural, so the diff can be reviewed as a + move rather than re-read as behaviour. +- Preserve each test module's path, so `super::` and `crate::` imports inside it + and `cargo test` filter expressions that name it continue to resolve. +- Choose one directory-module form and apply it consistently. +- Keep the extracted file a real module, subject to the same documentation rules + as the code it came from. + +## Options considered + +### Option A: keep test modules inline and raise the threshold + +Rejected. The budget is the reason to decompose, so exempting the largest files +from it inverts the rule. It also leaves the production code and its tests in +the same file with no mechanism to separate them later. + +### Option B: extract tests but use the self-named sibling form + +Rejected. The repository's convention is the `mod.rs` form: 46 `mod.rs` files +are tracked outside `vendor/`, and no path in the tree has both `name.rs` and +`name/mod.rs`, so no module is ambiguous between the two. Two file modules +deviate and reach a child through a same-named directory: +`installer/src/cli.rs` resolves `mod source_policy;` to `cli/source_policy.rs`, +and `crates/conditional_max_n_branches/src/driver.rs` resolves `mod numeric;` to +`driver/numeric.rs`. Both are single-child outliers, and neither is a +convention anyone is expected to follow. Adopting the form deliberately would +make a module's location depend on which convention its author happened to use. + +### Option C: `mod.rs` directory modules with `#[path]` sibling test files + +Accepted. + +## Decision outcome + +A module that has grown submodules becomes a directory whose root file is +`mod.rs`. Directory modules nest, so `installer/src/toolchain/tests/` is itself +a `tests/` directory module with its own `mod.rs`. + +An inline test module that outgrows the budget moves to a sibling file. The +declaration keeps the module's name, and an explicit `#[path]` attribute points +it at the new file: + +```rust +#[cfg(test)] +#[path = "list_tests.rs"] +mod tests; +``` + +A module's name and its file are separate things. `mod tests;` fixes the name, +and with it the module path `crate::list::tests`; `#[path]` selects the file +that backs it. The attribute is what makes the move reviewable: the test bodies +stay byte-identical across it, so the extraction can be checked as a move +rather than re-read as behaviour. + +The attribute also supplies the placement. Left to the default location, a bare +`mod tests;` in `list.rs` resolves to `list/tests.rs`, inside a `list/` +directory of its own. Naming the declaration after the file, as +`mod list_tests;`, would rename the module to `crate::list::list_tests` — and +still resolve to `list/list_tests.rs`, not to a sibling. Only the attribute +puts the file beside `list.rs` while keeping the module named `tests`. + +Renaming the module would not, on its own, break the tests: `super::` keeps +working, because the module remains a child of `list`. What breaks is absolute +paths such as `crate::list::tests::…` and `cargo test` filter expressions that +name `tests`. + +Three shapes occur, and the rule that distinguishes them is whether anything +above the extracted module already restricts it to test builds: + +- *File-module sibling.* The extracted file sits beside the module file and is + wired with `#[cfg(test)]` plus `#[path = "_tests.rs"]`. +- *Directory-module `tests.rs`.* Inside a directory module the attribute is + unnecessary, because Rust already resolves a bare `#[cfg(test)] mod tests;` to + `tests.rs` in the same directory. +- *Nested tests directory.* When the tests need helpers and submodules of their + own, they become a `tests/` directory module with a `mod.rs`. + +`installer/src/deps/tests.rs` shows the variation that follows from the rule. +It is declared as `#[cfg(test)] mod tests;` by `installer/src/deps/mod.rs`, so +the whole file is already test-only and its children omit `#[cfg(test)]` for +that reason alone. They still carry `#[path]`, because there is no +`deps/tests/` directory for a bare `mod` to resolve into. + +An extracted test file still opens with a `//!` module-level doc comment, so it +satisfies the repository rule that every module documents its purpose. + +## Known risks and limitations + +- `#[path]` is a manual anchor. Renaming or moving an extracted test file is not + checked by the compiler until the module is built, so the attribute can go + stale in a way that a conventional module path cannot. +- The attribute's *meaning* moves with the module's form. In a file module + `name.rs`, `#[path = "p"]` resolves relative to the containing directory; in + a directory module `name/mod.rs` it resolves relative to `name/` itself. + Converting a file module to a directory module therefore doubles the module's + own segment in any `#[path]` that already opens with it, as + `install_flow/mod.rs` did when a rebase renamed `install_flow.rs` around a + pre-existing anchor. A bare `mod` in a directory module carries no such + hazard, which is why the decision outcome above confines the attribute to the + file-module case. +- The 400-line budget is a repository convention that this decision serves, and + it is **not** yet machine-enforced in this repository. The `module_max_lines` + dylint lint implements the check and its own UI tests cover it, but the + workspace does not run Whitaker's lints over itself: `make lint` runs + `cargo doc` and `cargo clippy` only, and the repository's own `dylint.toml` + configures `no_std_fs_operations` alone. Self-linting is a separate layer of + work, and until it lands the budget is held by review. +- The extraction adds a file per module, so a directory listing no longer shows + a module and its tests together. The `#[path]` attribute is the link between + them, which is why it is written explicitly rather than inferred. + +## Architectural rationale + +The decision keeps two properties that matter more than the specific file +names. The first is that structure and behaviour stay separable: because the +declaration keeps the module name and `#[path]` only redirects the file, a +decomposition commit can be reviewed as a move, and a behaviour change can +never hide inside one. The second is that the budget is answered by +decomposition rather than exemption, so the rule continues to describe the +codebase instead of describing the exceptions to itself. + +Both properties follow from treating the module layout as a repository-wide +convention rather than a per-module judgement. 65 tracked `.rs` files already +carry a `#[path = "` anchor, so the mechanism this record standardizes was +already the repository's practice; the record fixes which cases use it and why. diff --git a/docs/contents.md b/docs/contents.md index 0115566ba..4d685d6f4 100644 --- a/docs/contents.md +++ b/docs/contents.md @@ -94,6 +94,10 @@ - [Architectural decision record (ADR) 005: adopt the shared CV-005 contract library](adr-005-adopt-the-shared-cv005-contract-library.md) records the move from a local copy of the CV-005 contract to the pinned shared check. +- [Architectural decision record (ADR) 006: directory-based module + layout](adr-006-directory-based-module-layout.md) records the `mod.rs` module + convention and the extraction of oversized test modules into sibling files, + and states plainly that the 400-line budget is not yet machine-enforced here. ## Requests for comments diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 35ab73668..478cb04a4 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -1404,6 +1404,145 @@ make check-fmt # Verify formatting make fmt # Apply formatting ``` +## Module layout and test placement + +### The 400-line module budget + +`AGENTS.md` states that no single code file may be longer than 400 lines. The +limit has a machine implementation — `module_max_lines`, one of Whitaker's own +dylint lints, measures the source lines occupied by a module and warns when the +count exceeds a configurable `max_lines` threshold, which defaults to 400 — but +that implementation does not yet run against this repository. See +[Where the budget is checked](#where-the-budget-is-checked) for what does and +does not execute. + +The budget is a readability rule. A module of that length has stopped being +reviewable in one sitting, so growth past the limit is answered by decomposing +the module, not by raising the threshold. + +The reasoning behind the layout rules below, including the alternatives that +were rejected, is recorded in +[ADR 006](./adr-006-directory-based-module-layout.md). + +### Directory modules + +A module that has grown into submodules becomes a directory whose root file is +`mod.rs`, for example `installer/src/install_flow/mod.rs` and +`installer/src/toolchain/mod.rs`. The self-named-sibling form, where +`install_flow.rs` sits next to an `install_flow/` directory, is not the +convention here: 46 `mod.rs` files are tracked outside `vendor/`, and no path +has both `name.rs` and `name/mod.rs`. Two file modules do reach a child that +way — `installer/src/cli.rs` and +`crates/conditional_max_n_branches/src/driver.rs` — but they are single-child +outliers, not the pattern to copy. + +Directory modules nest. `installer/src/toolchain/tests/` is a `tests/` +directory module with its own `mod.rs`, holding `test_helpers.rs` and a further +`failure_mocks/` subdirectory. + +### Sibling test modules + +An inline `#[cfg(test)] mod tests { … }` block that grows large enough to +threaten the 400-line budget moves to a sibling file. The declaration keeps the +module's name, and an explicit `#[path]` attribute points it at the file: + +```rust +#[cfg(test)] +#[path = "list_tests.rs"] +mod tests; +``` + +Three shapes of this appear in the tree. + +- **File-module sibling.** The extracted file is a sibling of the module file, + wired with `#[cfg(test)]` plus `#[path = "_tests.rs"]`. + `installer/src/list.rs` gains `installer/src/list_tests.rs`, + `installer/src/dependency_binaries/install/downloader.rs` gains + `downloader_tests.rs`, and `installer/src/deps/tests.rs` gains + `policy_tests.rs` and `tests_repository.rs`. +- **Directory-module `tests.rs`.** Inside a directory module the attribute is + unnecessary, because Rust already resolves it, so a bare + `#[cfg(test)] mod tests;` resolves to `tests.rs` in the same directory. + `installer/src/toolchain/mod.rs` ends with that declaration, and + `installer/src/install_flow/mod.rs` reaches `dependency_check_tests.rs` the + same way, with a bare `#[cfg(test)] mod dependency_check_tests;`. +- **Nested tests directory.** When the tests themselves need helpers and + submodules, they become a `tests/` directory module with a `mod.rs`. + +The `#[path]` form is preferred for the file-module case because `mod tests;` +keeps the test module's name, `tests`, and therefore its module path unchanged, +while the attribute places the file beside the module rather than in a +directory of its own. Left to the default location, a bare `mod tests;` in +`list.rs` resolves to `list/tests.rs`; naming the declaration after the file, as +`mod list_tests;`, would rename the module to `crate::list::list_tests` and +still resolve to `list/list_tests.rs`, not to a sibling. Only the attribute +puts the file beside `list.rs` while keeping the module named `tests`. + +Renaming the module would not break `super::`, since the module stays a child of +`list`; what breaks is `crate::` paths such as `crate::list::tests::…` and +`cargo test` filters naming `tests`. With the attribute the production file +drops back under the budget without folding a behaviour change into a purely +structural edit. A test file extracted this way still carries a `//!` +module-level doc comment, matching the repository rule that every module begins +with one. + +`installer/src/deps/tests.rs` shows the one variation on this. It is not a +module file in its own right: `installer/src/deps/mod.rs` declares it as +`#[cfg(test)] mod tests;`, so the whole file is already test-only. Its own +children therefore omit `#[cfg(test)]`, which would be redundant. They still +carry `#[path]`, because no `deps/tests/` directory exists for a bare `mod` to +resolve into: + +```rust +#[path = "policy_tests.rs"] +mod policy; + +#[path = "tests_repository.rs"] +mod repository; +``` + +Add `#[cfg(test)]` to an extracted test module only when nothing above it +already restricts it to test builds. + +The convention is repository-wide rather than installer-only: 65 tracked `.rs` +files contain a `#[path = "` anchor. + +### Where the budget is checked + +Be precise about this, because the current answer is narrower than it looks. + +`make lint` runs +`cargo clippy --workspace --all-targets --all-features -- -D warnings`, +preceded by a `cargo doc` pass. Neither runs the 400-line budget. The budget is +implemented by `module_max_lines`, one of Whitaker's own dylint lints, and a +dylint lint is not a rustc or Clippy built-in: it is a separate compiler plugin +that runs only when a workspace loads it deliberately. This workspace does not. +`rg 'dylint run' Makefile scripts/` finds nothing, and the root `dylint.toml` +configures `[no_std_fs_operations]` alone. The `[workspace.lints.rust]` and +`[workspace.lints.rustdoc]` tables in `Cargo.toml` do not change this: they +configure rustc and rustdoc lints, and neither names `module_max_lines`. Within +the CI workflow the dylint host tools are restored for the `make publish-check` +step alone, and that step's single dylint invocation is +`dylint list --no-metadata --no-build` — it enumerates built libraries to +verify packaging, and lints nothing. + +So the budget is **not** currently enforced automatically against this +repository's own source. The lint itself is real and tested — its UI fixtures +under `crates/module_max_lines/ui/` pin both the failing and passing cases — +but nothing runs it over these crates. Running Whitaker's lints over Whitaker +is the final layer of the lint-hardening stack, and it has not landed yet. + +Until it does, the budget is held by review. That is a weaker guarantee than a +gate, and the difference matters: a module that drifts past 400 lines will not +turn a check red. + +The threshold, when self-linting arrives, is set as TOML under +`[module_max_lines]`, not by a compiler flag. The shape is documented in +[`docs/whitaker-cli-design.md`](./whitaker-cli-design.md). + +The layout convention itself is recorded in +[ADR 006](./adr-006-directory-based-module-layout.md). + ## Python script interpreter convention Every Python script that a workflow or the `Makefile` runs by path carries the diff --git a/installer/src/dependency_binaries/install/downloader.rs b/installer/src/dependency_binaries/install/downloader.rs index 2b5d4d709..dbd3c15a8 100644 --- a/installer/src/dependency_binaries/install/downloader.rs +++ b/installer/src/dependency_binaries/install/downloader.rs @@ -287,114 +287,5 @@ fn open_destination_dir(destination: &Utf8Path) -> io::Result<(Dir, &str)> { } #[cfg(test)] -mod tests { - //! Tests for downloader error mapping and archive checksum verification. - - use super::*; - use crate::hex::to_lower_hex; - use sha2::{Digest, Sha256}; - use std::io::Write; - use tempfile::TempDir; - - // The under-cap success path is covered end to end by the local-server - // boundary tests; this exercises the over-cap rejection they cannot. - #[test] - fn copy_capped_rejects_a_body_exceeding_the_limit() { - let url = "https://example.test/a.tgz"; - let error = copy_capped(&[0u8; 100][..], &mut Vec::new(), 8, url) - .expect_err("an over-cap body must be rejected"); - assert!( - matches!(&error, DependencyBinaryInstallError::Download { url: u, reason } - if u == url && reason.contains("exceeds the maximum")), - "unexpected error: {error:?}", - ); - } - - #[test] - fn open_destination_dir_rejects_a_path_without_a_file_name() { - // The filesystem root has no file name, so the capability boundary - // cannot derive an archive name and must reject it up front. - let root = Utf8Path::new("/"); - let error = open_destination_dir(root).expect_err("root path has no file name"); - - assert_eq!(error.kind(), io::ErrorKind::InvalidInput); - assert!( - error.to_string().contains("has no file name"), - "error should identify the missing file name, got: {error}", - ); - } - - /// Removes a probe file from a capability-scoped directory when dropped, so - /// a leaked artefact is cleaned up even if an assertion panics first. - struct ProbeCleanup { - dir: Dir, - name: String, - } - - impl Drop for ProbeCleanup { - fn drop(&mut self) { - // A correct run never writes the probe into this directory, so a - // missing file is the expected case. - let _ = self.dir.remove_file(&self.name); - } - } - - #[test] - fn open_destination_dir_writes_into_the_destination_parent_not_the_cwd() { - let temp = TempDir::new().expect("create temp dir"); - let temp_dir = Utf8Path::from_path(temp.path()).expect("temp path is UTF-8"); - // A unique, test-owned probe name derived from the temp directory, so - // it can never collide with — or delete — a pre-existing file in the - // working directory. - let archive_file = format!( - "{}.tgz", - temp_dir.file_name().expect("temp dir has a file name"), - ); - let destination = temp_dir.join(&archive_file); - - // Open the process working directory as a capability, paired with RAII - // cleanup: if a regression leaks the probe here, it is removed on drop - // even when an assertion below panics first. - let cwd_probe = ProbeCleanup { - dir: Dir::open_ambient_dir(".", ambient_authority()).expect("open cwd capability"), - name: archive_file.clone(), - }; - // An independent capability for the destination's directory, used to - // confirm the archive actually lands there rather than trusting the - // directory handle returned by the code under test. - let destination_dir = Dir::open_ambient_dir(temp_dir, ambient_authority()) - .expect("open destination capability"); - - let (dir, archive_name) = open_destination_dir(&destination).expect("open destination dir"); - assert_eq!(archive_name, archive_file.as_str()); - - let mut file = dir - .create(archive_name) - .expect("create archive via capability"); - file.write_all(b"hello world").expect("write archive"); - drop(file); - - // The capability must write into the destination's parent directory... - assert!( - destination_dir.exists(&archive_file), - "archive must exist at the destination path", - ); - // ...and never into the process working directory. This assertion fails - // if `open_destination_dir` opens `.` for a destination with a real - // parent. - assert!( - !cwd_probe.dir.exists(&archive_file), - "capability must not create the archive in the current working directory", - ); - - // Re-open through the same capability and keep the end-to-end checksum - // assertion. - let expected = to_lower_hex(&Sha256::digest(b"hello world")); - let archive = dir.open(archive_name).expect("open archive via capability"); - assert!(verify_archive_checksum(archive, destination.as_std_path(), &expected).is_ok()); - - // `cwd_probe` drops here (or on any earlier panic), removing a leaked - // probe through its capability. - drop(cwd_probe); - } -} +#[path = "downloader_tests.rs"] +mod tests; diff --git a/installer/src/dependency_binaries/install/downloader_tests.rs b/installer/src/dependency_binaries/install/downloader_tests.rs new file mode 100644 index 000000000..ee1813531 --- /dev/null +++ b/installer/src/dependency_binaries/install/downloader_tests.rs @@ -0,0 +1,109 @@ +//! Tests for downloader error mapping and archive checksum verification. + +use super::*; +use crate::hex::to_lower_hex; +use sha2::{Digest, Sha256}; +use std::io::Write; +use tempfile::TempDir; + +// The under-cap success path is covered end to end by the local-server +// boundary tests; this exercises the over-cap rejection they cannot. +#[test] +fn copy_capped_rejects_a_body_exceeding_the_limit() { + let url = "https://example.test/a.tgz"; + let error = copy_capped(&[0u8; 100][..], &mut Vec::new(), 8, url) + .expect_err("an over-cap body must be rejected"); + assert!( + matches!(&error, DependencyBinaryInstallError::Download { url: u, reason } + if u == url && reason.contains("exceeds the maximum")), + "unexpected error: {error:?}", + ); +} + +#[test] +fn open_destination_dir_rejects_a_path_without_a_file_name() { + // The filesystem root has no file name, so the capability boundary + // cannot derive an archive name and must reject it up front. + let root = Utf8Path::new("/"); + let error = open_destination_dir(root).expect_err("root path has no file name"); + + assert_eq!(error.kind(), io::ErrorKind::InvalidInput); + assert!( + error.to_string().contains("has no file name"), + "error should identify the missing file name, got: {error}", + ); +} + +/// Removes a probe file from a capability-scoped directory when dropped, so +/// a leaked artefact is cleaned up even if an assertion panics first. +struct ProbeCleanup { + dir: Dir, + name: String, +} + +impl Drop for ProbeCleanup { + fn drop(&mut self) { + // A correct run never writes the probe into this directory, so a + // missing file is the expected case. + let _ = self.dir.remove_file(&self.name); + } +} + +#[test] +fn open_destination_dir_writes_into_the_destination_parent_not_the_cwd() { + let temp = TempDir::new().expect("create temp dir"); + let temp_dir = Utf8Path::from_path(temp.path()).expect("temp path is UTF-8"); + // A unique, test-owned probe name derived from the temp directory, so + // it can never collide with — or delete — a pre-existing file in the + // working directory. + let archive_file = format!( + "{}.tgz", + temp_dir.file_name().expect("temp dir has a file name"), + ); + let destination = temp_dir.join(&archive_file); + + // Open the process working directory as a capability, paired with RAII + // cleanup: if a regression leaks the probe here, it is removed on drop + // even when an assertion below panics first. + let cwd_probe = ProbeCleanup { + dir: Dir::open_ambient_dir(".", ambient_authority()).expect("open cwd capability"), + name: archive_file.clone(), + }; + // An independent capability for the destination's directory, used to + // confirm the archive actually lands there rather than trusting the + // directory handle returned by the code under test. + let destination_dir = + Dir::open_ambient_dir(temp_dir, ambient_authority()).expect("open destination capability"); + + let (dir, archive_name) = open_destination_dir(&destination).expect("open destination dir"); + assert_eq!(archive_name, archive_file.as_str()); + + let mut file = dir + .create(archive_name) + .expect("create archive via capability"); + file.write_all(b"hello world").expect("write archive"); + drop(file); + + // The capability must write into the destination's parent directory... + assert!( + destination_dir.exists(&archive_file), + "archive must exist at the destination path", + ); + // ...and never into the process working directory. This assertion fails + // if `open_destination_dir` opens `.` for a destination with a real + // parent. + assert!( + !cwd_probe.dir.exists(&archive_file), + "capability must not create the archive in the current working directory", + ); + + // Re-open through the same capability and keep the end-to-end checksum + // assertion. + let expected = to_lower_hex(&Sha256::digest(b"hello world")); + let archive = dir.open(archive_name).expect("open archive via capability"); + assert!(verify_archive_checksum(archive, destination.as_std_path(), &expected).is_ok()); + + // `cwd_probe` drops here (or on any earlier panic), removing a leaked + // probe through its capability. + drop(cwd_probe); +} diff --git a/installer/src/deps/policy_tests.rs b/installer/src/deps/policy_tests.rs new file mode 100644 index 000000000..0cec658a1 --- /dev/null +++ b/installer/src/deps/policy_tests.rs @@ -0,0 +1,141 @@ +//! Source-fallback policy tests for Dylint dependency installation. +//! +//! These cover the `SourcePolicy::no_source_fallback` guard: a run that +//! forbids a source build must refuse rather than fall back to `cargo +//! install`. Split from `tests.rs` to keep each file inside the repository's +//! 400-line module limit. + +use super::*; +use crate::dependency_binaries::MockDependencyBinaryInstaller; +use crate::error::InstallerError; +use crate::test_utils::dependency_binary_helpers::{ + binstall_install, binstall_version_check_with_result, cargo_dylint_check, +}; +use crate::test_utils::{StubExecutor, success_output}; + +/// Install options that refuse a source build, as CI sets them. +fn strict_install_options( + repository_installer: &dyn DependencyBinaryInstaller, +) -> DependencyInstallOptions<'_> { + let mut options = install_options(repository_installer, false); + options.policy.no_source_fallback = true; + options +} + +/// A repository installer whose archive is absent, as during a republish. +fn installer_missing_its_archive() -> MockDependencyBinaryInstaller { + let mut repository_installer = MockDependencyBinaryInstaller::new(); + repository_installer.expect_install().returning(|_, _, _| { + Err(DependencyBinaryInstallError::Download { + url: "https://example.test/cargo-dylint-x86_64-unknown-linux-gnu-v6.0.1.tgz".to_owned(), + reason: "not found".to_owned(), + }) + }); + repository_installer +} + +#[test] +fn a_missing_dylint_archive_fails_under_the_no_source_fallback_policy() { + // The executor expects nothing: refusing means no Cargo command runs at + // all, which is the assertion that matters. A test that only checked the + // error would pass while `cargo install` had already been attempted. + let repository_installer = installer_missing_its_archive(); + let executor = StubExecutor::new(vec![binstall_version_check_with_result(Ok( + success_output(), + ))]); + let mut stderr = Vec::new(); + + let error = install_dylint_tools_with_options( + &executor, + &DylintToolStatus { + cargo_dylint: false, + dylint_link: true, + }, + &mut stderr, + strict_install_options(&repository_installer), + ) + .expect_err("an absent archive must fail when a source build is forbidden"); + + assert!( + matches!(error, InstallerError::SourceFallbackForbidden { .. }), + "expected a source-fallback refusal, got: {error}" + ); + assert!( + error.to_string().contains("cargo-dylint"), + "the refusal must name the tool, got: {error}" + ); +} + +#[test] +fn the_same_missing_archive_falls_back_without_the_policy() { + // The pair is the point: the flag changes the outcome for one input, so + // the default path is asserted on the same input rather than assumed. + let repository_installer = installer_missing_its_archive(); + let executor = StubExecutor::new(vec![ + binstall_version_check_with_result(Ok(success_output())), + binstall_install("cargo-dylint", Ok(success_output())) + .expect("cargo-dylint dependency version should resolve"), + cargo_dylint_check().expect("cargo-dylint dependency version should resolve"), + ]); + let mut stderr = Vec::new(); + + install_dylint_tools_with_options( + &executor, + &DylintToolStatus { + cargo_dylint: false, + dylint_link: true, + }, + &mut stderr, + install_options(&repository_installer, false), + ) + .expect("the default path should still fall back"); + + let output = String::from_utf8(stderr).expect("stderr should be UTF-8"); + assert!(output.contains("Repository install for cargo-dylint unavailable")); + executor.assert_finished(); +} + +#[test] +fn an_unresolvable_repository_context_still_refuses_a_source_build() { + // The hole this closes: with no base directories the repository install is + // never attempted, so the refusals inside the failure arms never run and + // `cargo install` was reached unguarded. A run that forbade a source build + // would have taken one, and reported success. + // + // The executor expects nothing. Asserting only the error would pass while + // Cargo had already been invoked, which is the failure itself. + let executor = StubExecutor::new(vec![]); + let mut stderr = Vec::new(); + + let error = install_missing_tools( + &executor, + &DylintToolStatus { + cargo_dylint: false, + dylint_link: true, + }, + &mut stderr, + &InstallContext { + repo: None, + cargo_fallback_mode: InstallMode::CargoInstall, + quiet: false, + no_source_fallback: true, + }, + ) + .expect_err("no repository context must not become a licence to build"); + + assert!( + matches!(error, InstallerError::SourceFallbackForbidden { .. }), + "expected a source-fallback refusal, got: {error}" + ); + let message = error.to_string(); + assert!( + message.contains("cargo-dylint"), + "the refusal must name the tool, got: {message}" + ); + assert!( + message.contains("base") && message.contains("directories"), + "the reason must say why no repository install was attempted, not \ + repeat the generic one, got: {message}" + ); + executor.assert_finished(); +} diff --git a/installer/src/deps/tests.rs b/installer/src/deps/tests.rs index 1049fd2aa..9bb135a6b 100644 --- a/installer/src/deps/tests.rs +++ b/installer/src/deps/tests.rs @@ -294,274 +294,8 @@ fn install_dylint_tools_skips_dylint_link_when_cargo_dylint_source_build_install executor.assert_finished(); } -/// Writes a fake `dylint-link` into a temporary directory and returns the -/// directory guard alongside the binary path. -/// -/// The fake is staged with a failing exit status so that any attempt to -/// execute it as a health check would be observable as a verification -/// failure. -fn staged_unrunnable_dylint_link() -> std::io::Result<(tempfile::TempDir, PathBuf)> { - let dir = tempfile::tempdir()?; - let path = crate::test_utils::dependency_binary_helpers::path_binary_location( - dir.path(), - "dylint-link", - ); - crate::test_utils::dependency_binary_helpers::write_fake_binary_with_status(&path, true, 1)?; - Ok((dir, path)) -} - -#[test] -fn install_dylint_tools_accepts_repository_dylint_link_without_executing_it() { - // `dylint-link` forwards its argument list to the underlying linker and - // has no reliable self-reporting subcommand, so a successful repository - // install is accepted on the strength of the download, checksum, - // extraction, and permission steps alone. Staging a fake that exits - // non-zero proves the binary is never executed as a health check: any - // such probe would reject it and fall back to Cargo. - let (_dir, binary_path) = staged_unrunnable_dylint_link().expect("stage fake dylint-link"); - let mut repository_installer = MockDependencyBinaryInstaller::new(); - repository_installer - .expect_install() - .once() - .returning(move |_, _, _| Ok(binary_path.clone())); - let executor = StubExecutor::new(vec![binstall_version_check_with_result(Ok( - success_output(), - ))]); - let mut stderr = Vec::new(); - - install_dylint_tools_with_options( - &executor, - &DylintToolStatus { - cargo_dylint: true, - dylint_link: false, - }, - &mut stderr, - install_options(&repository_installer, false), - ) - .expect("repository install should satisfy dylint-link"); - - let output = String::from_utf8(stderr).expect("stderr should be UTF-8"); - assert!(output.contains("Installed dylint-link from repository release.")); - assert!(!output.contains("failed verification")); - // No Cargo command beyond the binstall-availability probe may run: a - // source build of dylint-link cannot succeed on toolchains below the - // crate's rustc floor. - executor.assert_finished(); -} - -#[test] -fn install_dylint_tools_falls_back_when_repository_dylint_link_install_fails() { - // Genuine repository failures — missing asset, checksum mismatch, - // extraction failure, or an unwritable executable — must still fall back - // to Cargo. - let mut repository_installer = MockDependencyBinaryInstaller::new(); - repository_installer.expect_install().returning(|_, _, _| { - Err(DependencyBinaryInstallError::Install { - binary: "dylint-link".to_owned(), - reason: "checksum mismatch".to_owned(), - }) - }); - let executor = StubExecutor::new(vec![ - binstall_version_check_with_result(Ok(success_output())), - binstall_install("dylint-link", Ok(success_output())) - .expect("dylint-link dependency version should resolve"), - // The post-binstall check resolves the PATH binary and then confirms - // the version against Cargo's installed-binary registry. - dylint_link_install_list_check().expect("dylint-link dependency version should resolve"), - ]); - let mut stderr = Vec::new(); - - with_fake_binary_on_path("dylint-link", || { - install_dylint_tools_with_options( - &executor, - &DylintToolStatus { - cargo_dylint: true, - dylint_link: false, - }, - &mut stderr, - install_options(&repository_installer, false), - ) - .expect("binstall fallback should succeed"); - }) - .expect("stage fake dylint-link on PATH"); +#[path = "policy_tests.rs"] +mod policy; - let output = String::from_utf8(stderr).expect("stderr should be UTF-8"); - assert!(output.contains("Repository install for dylint-link unavailable")); - assert!(output.contains("Installed dylint-link with cargo binstall.")); - executor.assert_finished(); -} - -#[test] -fn install_tool_errors_when_dependency_manifest_entry_is_missing() { - let missing_tool = DependencyTool { - package: "missing-tool", - command: "missing-tool", - args: &["--version"], - }; - let executor = StubExecutor::new(vec![]); - let mut repository_installer = MockDependencyBinaryInstaller::new(); - repository_installer.expect_install().never(); - let dirs = StubDirs { - bin_dir: Some(PathBuf::from("/tmp/bin")), - }; - let target = TargetTriple::try_from("x86_64-unknown-linux-gnu").expect("valid target"); - let mut stderr = Vec::new(); - - let error = install_tool( - &executor, - &missing_tool, - &mut stderr, - &InstallContext { - repo: repository_install_context( - Some(&dirs), - Some(&repository_installer as &dyn DependencyBinaryInstaller), - Some(&target), - ), - cargo_fallback_mode: InstallMode::Binstall, - quiet: false, - no_source_fallback: false, - }, - ) - .expect_err("missing dependency manifest entry should be an install error"); - - match error { - InstallerError::DependencyInstall { tool, message } => { - assert_eq!(tool, "missing-tool"); - assert_eq!( - message, - "dependency manifest is missing an entry for missing-tool" - ); - } - other => panic!("unexpected error: {other}"), - } - - assert!(stderr.is_empty()); - executor.assert_finished(); -} - -/// Install options that refuse a source build, as CI sets them. -fn strict_install_options( - repository_installer: &dyn DependencyBinaryInstaller, -) -> DependencyInstallOptions<'_> { - let mut options = install_options(repository_installer, false); - options.policy.no_source_fallback = true; - options -} - -/// A repository installer whose archive is absent, as during a republish. -fn installer_missing_its_archive() -> MockDependencyBinaryInstaller { - let mut repository_installer = MockDependencyBinaryInstaller::new(); - repository_installer.expect_install().returning(|_, _, _| { - Err(DependencyBinaryInstallError::Download { - url: "https://example.test/cargo-dylint-x86_64-unknown-linux-gnu-v6.0.1.tgz".to_owned(), - reason: "not found".to_owned(), - }) - }); - repository_installer -} - -#[test] -fn a_missing_dylint_archive_fails_under_the_no_source_fallback_policy() { - // The executor expects nothing: refusing means no Cargo command runs at - // all, which is the assertion that matters. A test that only checked the - // error would pass while `cargo install` had already been attempted. - let repository_installer = installer_missing_its_archive(); - let executor = StubExecutor::new(vec![binstall_version_check_with_result(Ok( - success_output(), - ))]); - let mut stderr = Vec::new(); - - let error = install_dylint_tools_with_options( - &executor, - &DylintToolStatus { - cargo_dylint: false, - dylint_link: true, - }, - &mut stderr, - strict_install_options(&repository_installer), - ) - .expect_err("an absent archive must fail when a source build is forbidden"); - - assert!( - matches!(error, InstallerError::SourceFallbackForbidden { .. }), - "expected a source-fallback refusal, got: {error}" - ); - assert!( - error.to_string().contains("cargo-dylint"), - "the refusal must name the tool, got: {error}" - ); -} - -#[test] -fn the_same_missing_archive_falls_back_without_the_policy() { - // The pair is the point: the flag changes the outcome for one input, so - // the default path is asserted on the same input rather than assumed. - let repository_installer = installer_missing_its_archive(); - let executor = StubExecutor::new(vec![ - binstall_version_check_with_result(Ok(success_output())), - binstall_install("cargo-dylint", Ok(success_output())) - .expect("cargo-dylint dependency version should resolve"), - cargo_dylint_check().expect("cargo-dylint dependency version should resolve"), - ]); - let mut stderr = Vec::new(); - - install_dylint_tools_with_options( - &executor, - &DylintToolStatus { - cargo_dylint: false, - dylint_link: true, - }, - &mut stderr, - install_options(&repository_installer, false), - ) - .expect("the default path should still fall back"); - - let output = String::from_utf8(stderr).expect("stderr should be UTF-8"); - assert!(output.contains("Repository install for cargo-dylint unavailable")); - executor.assert_finished(); -} - -#[test] -fn an_unresolvable_repository_context_still_refuses_a_source_build() { - // The hole this closes: with no base directories the repository install is - // never attempted, so the refusals inside the failure arms never run and - // `cargo install` was reached unguarded. A run that forbade a source build - // would have taken one, and reported success. - // - // The executor expects nothing. Asserting only the error would pass while - // Cargo had already been invoked, which is the failure itself. - let executor = StubExecutor::new(vec![]); - let mut stderr = Vec::new(); - - let error = install_missing_tools( - &executor, - &DylintToolStatus { - cargo_dylint: false, - dylint_link: true, - }, - &mut stderr, - &InstallContext { - repo: None, - cargo_fallback_mode: InstallMode::CargoInstall, - quiet: false, - no_source_fallback: true, - }, - ) - .expect_err("no repository context must not become a licence to build"); - - assert!( - matches!(error, InstallerError::SourceFallbackForbidden { .. }), - "expected a source-fallback refusal, got: {error}" - ); - let message = error.to_string(); - assert!( - message.contains("cargo-dylint"), - "the refusal must name the tool, got: {message}" - ); - assert!( - message.contains("base") && message.contains("directories"), - "the reason must say why no repository install was attempted, not \ - repeat the generic one, got: {message}" - ); - executor.assert_finished(); -} +#[path = "tests_repository.rs"] +mod repository; diff --git a/installer/src/deps/tests_repository.rs b/installer/src/deps/tests_repository.rs new file mode 100644 index 000000000..07ccc6bcc --- /dev/null +++ b/installer/src/deps/tests_repository.rs @@ -0,0 +1,169 @@ +//! Repository-release installation tests for the Dylint dependency tools. +//! +//! These exercise `install_tool` and `repository_install_context` with a +//! mocked repository installer. Split from `tests.rs` to keep each file +//! inside the repository's 400-line module limit. + +use std::path::PathBuf; + +use super::{ + DependencyBinaryInstaller, DependencyTool, DylintToolStatus, + install::{InstallContext, InstallMode, install_tool, repository_install_context}, + install_dylint_tools_with_options, install_options, +}; +use crate::dependency_binaries::{DependencyBinaryInstallError, MockDependencyBinaryInstaller}; +use crate::error::InstallerError; +use crate::installer_packaging::TargetTriple; +use crate::test_utils::{ + StubDirs, StubExecutor, + dependency_binary_helpers::{ + binstall_install, binstall_version_check_with_result, dylint_link_install_list_check, + with_fake_binary_on_path, + }, + success_output, +}; + +/// Writes a fake `dylint-link` into a temporary directory and returns the +/// directory guard alongside the binary path. +/// +/// The fake is staged with a failing exit status so that any attempt to +/// execute it as a health check would be observable as a verification +/// failure. +fn staged_unrunnable_dylint_link() -> std::io::Result<(tempfile::TempDir, PathBuf)> { + let dir = tempfile::tempdir()?; + let path = crate::test_utils::dependency_binary_helpers::path_binary_location( + dir.path(), + "dylint-link", + ); + crate::test_utils::dependency_binary_helpers::write_fake_binary_with_status(&path, true, 1)?; + Ok((dir, path)) +} + +#[test] +fn install_dylint_tools_accepts_repository_dylint_link_without_executing_it() { + // `dylint-link` forwards its argument list to the underlying linker and + // has no reliable self-reporting subcommand, so a successful repository + // install is accepted on the strength of the download, checksum, + // extraction, and permission steps alone. Staging a fake that exits + // non-zero proves the binary is never executed as a health check: any + // such probe would reject it and fall back to Cargo. + let (_dir, binary_path) = staged_unrunnable_dylint_link().expect("stage fake dylint-link"); + let mut repository_installer = MockDependencyBinaryInstaller::new(); + repository_installer + .expect_install() + .once() + .returning(move |_, _, _| Ok(binary_path.clone())); + let executor = StubExecutor::new(vec![binstall_version_check_with_result(Ok( + success_output(), + ))]); + let mut stderr = Vec::new(); + + install_dylint_tools_with_options( + &executor, + &DylintToolStatus { + cargo_dylint: true, + dylint_link: false, + }, + &mut stderr, + install_options(&repository_installer, false), + ) + .expect("repository install should satisfy dylint-link"); + + let output = String::from_utf8(stderr).expect("stderr should be UTF-8"); + assert!(output.contains("Installed dylint-link from repository release.")); + assert!(!output.contains("failed verification")); + // No Cargo command beyond the binstall-availability probe may run: a + // source build of dylint-link cannot succeed on toolchains below the + // crate's rustc floor. + executor.assert_finished(); +} + +#[test] +fn install_dylint_tools_falls_back_when_repository_dylint_link_install_fails() { + // Genuine repository failures — missing asset, checksum mismatch, + // extraction failure, or an unwritable executable — must still fall back + // to Cargo. + let mut repository_installer = MockDependencyBinaryInstaller::new(); + repository_installer.expect_install().returning(|_, _, _| { + Err(DependencyBinaryInstallError::Install { + binary: "dylint-link".to_owned(), + reason: "checksum mismatch".to_owned(), + }) + }); + let executor = StubExecutor::new(vec![ + binstall_version_check_with_result(Ok(success_output())), + binstall_install("dylint-link", Ok(success_output())) + .expect("dylint-link dependency version should resolve"), + // The post-binstall check resolves the PATH binary and then confirms + // the version against Cargo's installed-binary registry. + dylint_link_install_list_check().expect("dylint-link dependency version should resolve"), + ]); + let mut stderr = Vec::new(); + + with_fake_binary_on_path("dylint-link", || { + install_dylint_tools_with_options( + &executor, + &DylintToolStatus { + cargo_dylint: true, + dylint_link: false, + }, + &mut stderr, + install_options(&repository_installer, false), + ) + .expect("binstall fallback should succeed"); + }) + .expect("stage fake dylint-link on PATH"); + + let output = String::from_utf8(stderr).expect("stderr should be UTF-8"); + assert!(output.contains("Repository install for dylint-link unavailable")); + assert!(output.contains("Installed dylint-link with cargo binstall.")); + executor.assert_finished(); +} + +#[test] +fn install_tool_errors_when_dependency_manifest_entry_is_missing() { + let missing_tool = DependencyTool { + package: "missing-tool", + command: "missing-tool", + args: &["--version"], + }; + let executor = StubExecutor::new(vec![]); + let mut repository_installer = MockDependencyBinaryInstaller::new(); + repository_installer.expect_install().never(); + let dirs = StubDirs { + bin_dir: Some(PathBuf::from("/tmp/bin")), + }; + let target = TargetTriple::try_from("x86_64-unknown-linux-gnu").expect("valid target"); + let mut stderr = Vec::new(); + + let error = install_tool( + &executor, + &missing_tool, + &mut stderr, + &InstallContext { + repo: repository_install_context( + Some(&dirs), + Some(&repository_installer as &dyn DependencyBinaryInstaller), + Some(&target), + ), + cargo_fallback_mode: InstallMode::Binstall, + quiet: false, + no_source_fallback: false, + }, + ) + .expect_err("missing dependency manifest entry should be an install error"); + + match error { + InstallerError::DependencyInstall { tool, message } => { + assert_eq!(tool, "missing-tool"); + assert_eq!( + message, + "dependency manifest is missing an entry for missing-tool" + ); + } + other => panic!("unexpected error: {other}"), + } + + assert!(stderr.is_empty()); + executor.assert_finished(); +} diff --git a/installer/src/install_flow.rs b/installer/src/install_flow/mod.rs similarity index 99% rename from installer/src/install_flow.rs rename to installer/src/install_flow/mod.rs index 2e2c6c36e..1895fc577 100644 --- a/installer/src/install_flow.rs +++ b/installer/src/install_flow/mod.rs @@ -365,5 +365,4 @@ fn write_metrics_summary(quiet: bool, record_outcome: &RecordOutcome, stderr: &m mod tests; #[cfg(test)] -#[path = "install_flow/dependency_check_tests.rs"] mod dependency_check_tests; diff --git a/installer/src/list.rs b/installer/src/list.rs index b2a0ddfcb..6de1bf449 100644 --- a/installer/src/list.rs +++ b/installer/src/list.rs @@ -175,253 +175,5 @@ where } #[cfg(test)] -mod tests { - use super::*; - use rstest::{fixture, rstest}; - use std::fs; - use tempfile::TempDir; - - // ------------------------------------------------------------------------- - // Fixtures - // ------------------------------------------------------------------------- - - /// A temporary directory converted to a UTF-8 path for test isolation. - struct TempTarget { - _temp: TempDir, - path: Utf8PathBuf, - } - - #[fixture] - fn temp_target() -> TempTarget { - let temp = TempDir::new().expect("failed to create temp dir"); - let path = Utf8PathBuf::try_from(temp.path().to_owned()).expect("non-UTF8 temp path"); - TempTarget { _temp: temp, path } - } - - /// A Write implementation that always fails, for testing error paths. - struct FailingWriter; - - impl std::io::Write for FailingWriter { - fn write(&mut self, _buf: &[u8]) -> std::io::Result { - Err(std::io::Error::other("simulated write failure")) - } - - fn flush(&mut self) -> std::io::Result<()> { - Err(std::io::Error::other("simulated flush failure")) - } - } - - // ------------------------------------------------------------------------- - // Helpers - // ------------------------------------------------------------------------- - - #[derive(Debug, Clone, Copy)] - enum MockLibraryKind { - Local, - Prebuilt { target: &'static str }, - } - - impl MockLibraryKind { - fn library_dir(&self, target_dir: &Utf8Path, toolchain: &str) -> Utf8PathBuf { - match self { - Self::Local => target_dir.join(toolchain).join("release"), - Self::Prebuilt { target } => target_dir.join(toolchain).join(target).join("lib"), - } - } - - fn content(&self) -> &'static [u8] { - match self { - Self::Local => b"mock library", - Self::Prebuilt { .. } => b"mock prebuilt library", - } - } - } - - fn create_mock_library_internal(target_dir: &Utf8Path, toolchain: &str, kind: MockLibraryKind) { - use crate::builder::{library_extension, library_prefix}; - - let lib_dir = kind.library_dir(target_dir, toolchain); - fs::create_dir_all(&lib_dir).expect("failed to create target library directory"); - - let filename = format!( - "{}whitaker_suite@{toolchain}{}", - library_prefix(), - library_extension() - ); - - let error_msg = match kind { - MockLibraryKind::Local => "failed to create mock library", - MockLibraryKind::Prebuilt { .. } => "failed to create prebuilt mock library", - }; - fs::write(lib_dir.join(filename), kind.content()).expect(error_msg); - } - - /// Helper to create a mock installed library in the target directory for tests. - fn create_mock_library(target_dir: &Utf8Path, toolchain: &str) { - create_mock_library_internal(target_dir, toolchain, MockLibraryKind::Local); - } - - fn create_mock_prebuilt_library(target_dir: &Utf8Path, toolchain: &str, target: &'static str) { - create_mock_library_internal(target_dir, toolchain, MockLibraryKind::Prebuilt { target }); - } - - // ------------------------------------------------------------------------- - // run_list tests - // ------------------------------------------------------------------------- - - #[rstest] - fn run_list_outputs_human_readable_format(temp_target: TempTarget) { - let args = ListArgs { - json: false, - target_dir: Some(temp_target.path.clone()), - }; - let mut stdout = Vec::new(); - - let result = run_list_with(&args, &mut stdout, || None); - - assert!(result.is_ok(), "expected success, got: {result:?}"); - let output = String::from_utf8_lossy(&stdout); - assert!(output.contains("No lints installed"), "got: {output}"); - } - - #[rstest] - #[case::json_format(true, &["toolchains", "\"active\""])] - #[case::human_format(false, &["nightly-2026-05-28", "whitaker_suite"])] - fn run_list_with_installed_library_includes_expected_output( - temp_target: TempTarget, - #[case] json: bool, - #[case] expected: &[&str], - ) { - create_mock_library(&temp_target.path, "nightly-2026-05-28"); - let args = ListArgs { - json, - target_dir: Some(temp_target.path.clone()), - }; - let mut stdout = Vec::new(); - - let result = run_list_with(&args, &mut stdout, || Some("nightly-2026-05-28".to_owned())); - - assert!(result.is_ok(), "expected success, got: {result:?}"); - let output = String::from_utf8_lossy(&stdout); - for needle in expected { - assert!( - output.contains(needle), - "expected '{needle}' in output: {output}" - ); - } - } - - #[rstest] - fn run_list_finds_prebuilt_layout_libraries(temp_target: TempTarget) { - create_mock_prebuilt_library( - &temp_target.path, - "nightly-2026-05-28", - "x86_64-unknown-linux-gnu", - ); - let args = ListArgs { - json: false, - target_dir: Some(temp_target.path.clone()), - }; - let mut stdout = Vec::new(); - - let result = run_list_with(&args, &mut stdout, || Some("nightly-2026-05-28".to_owned())); - - assert!(result.is_ok(), "expected success, got: {result:?}"); - let output = String::from_utf8_lossy(&stdout); - assert!(output.contains("nightly-2026-05-28"), "got: {output}"); - assert!(output.contains("whitaker_suite"), "got: {output}"); - } - - #[rstest] - fn run_list_returns_write_failed_on_stdout_error(temp_target: TempTarget) { - let args = ListArgs { - json: false, - target_dir: Some(temp_target.path.clone()), - }; - let mut failing_stdout = FailingWriter; - - let result = run_list_with(&args, &mut failing_stdout, || None); - - let err = result.expect_err("expected error on write failure"); - assert!( - matches!(err, InstallerError::WriteFailed { .. }), - "expected WriteFailed error, got: {err:?}" - ); - } - - // ------------------------------------------------------------------------- - // detect_active_toolchain_in tests - // ------------------------------------------------------------------------- - - #[rstest] - fn detect_active_toolchain_in_returns_none_when_no_toolchain_file(temp_target: TempTarget) { - let result = detect_active_toolchain_in(&temp_target.path); - assert!( - result.is_none(), - "expected None for directory without rust-toolchain.toml" - ); - } - - #[rstest] - fn detect_active_toolchain_in_returns_channel_when_toolchain_file_exists( - temp_target: TempTarget, - ) { - // Create a rust-toolchain.toml file - let toolchain_content = r#"[toolchain] -channel = "nightly-2026-05-28" -"#; - fs::write( - temp_target.path.join("rust-toolchain.toml"), - toolchain_content, - ) - .expect("failed to write rust-toolchain.toml"); - - let result = detect_active_toolchain_in(&temp_target.path); - - assert_eq!(result, Some("nightly-2026-05-28".to_owned())); - } - - // ------------------------------------------------------------------------- - // determine_target_dir tests - // ------------------------------------------------------------------------- - - #[rstest] - fn determine_target_dir_returns_cli_value_when_provided(temp_target: TempTarget) { - let result = determine_target_dir_with(Some(&temp_target.path), || None); - - assert!(result.is_ok(), "expected success, got: {result:?}"); - assert_eq!(result.expect("already checked"), temp_target.path); - } - - #[rstest] - fn determine_target_dir_falls_back_to_default_when_cli_is_none(temp_target: TempTarget) { - let default_path = temp_target.path.clone(); - - let result = determine_target_dir_with(None, || Some(default_path.clone())); - - assert!(result.is_ok(), "expected success, got: {result:?}"); - assert_eq!(result.expect("already checked"), default_path); - } - - #[test] - fn determine_target_dir_returns_error_when_no_default_available() { - let result = determine_target_dir_with(None, || None); - - let err = result.expect_err("expected error when no default"); - assert!( - matches!(err, InstallerError::StagingFailed { .. }), - "expected StagingFailed error, got: {err:?}" - ); - } - - #[rstest] - fn determine_target_dir_prefers_cli_over_default(temp_target: TempTarget) { - let cli_path = temp_target.path.clone(); - let default_path = temp_target.path.join("should_not_be_used"); - - let result = determine_target_dir_with(Some(&cli_path), || Some(default_path)); - - assert!(result.is_ok(), "expected success, got: {result:?}"); - assert_eq!(result.expect("already checked"), cli_path); - } -} +#[path = "list_tests.rs"] +mod tests; diff --git a/installer/src/list_tests.rs b/installer/src/list_tests.rs new file mode 100644 index 000000000..88d2a871b --- /dev/null +++ b/installer/src/list_tests.rs @@ -0,0 +1,248 @@ +//! Tests for the `list` command handler and its path resolution helpers. + +use super::*; +use rstest::{fixture, rstest}; +use std::fs; +use tempfile::TempDir; + +// ------------------------------------------------------------------------- +// Fixtures +// ------------------------------------------------------------------------- + +/// A temporary directory converted to a UTF-8 path for test isolation. +struct TempTarget { + _temp: TempDir, + path: Utf8PathBuf, +} + +#[fixture] +fn temp_target() -> TempTarget { + let temp = TempDir::new().expect("failed to create temp dir"); + let path = Utf8PathBuf::try_from(temp.path().to_owned()).expect("non-UTF8 temp path"); + TempTarget { _temp: temp, path } +} + +/// A Write implementation that always fails, for testing error paths. +struct FailingWriter; + +impl std::io::Write for FailingWriter { + fn write(&mut self, _buf: &[u8]) -> std::io::Result { + Err(std::io::Error::other("simulated write failure")) + } + + fn flush(&mut self) -> std::io::Result<()> { + Err(std::io::Error::other("simulated flush failure")) + } +} + +// ------------------------------------------------------------------------- +// Helpers +// ------------------------------------------------------------------------- + +#[derive(Debug, Clone, Copy)] +enum MockLibraryKind { + Local, + Prebuilt { target: &'static str }, +} + +impl MockLibraryKind { + fn library_dir(&self, target_dir: &Utf8Path, toolchain: &str) -> Utf8PathBuf { + match self { + Self::Local => target_dir.join(toolchain).join("release"), + Self::Prebuilt { target } => target_dir.join(toolchain).join(target).join("lib"), + } + } + + fn content(&self) -> &'static [u8] { + match self { + Self::Local => b"mock library", + Self::Prebuilt { .. } => b"mock prebuilt library", + } + } +} + +fn create_mock_library_internal(target_dir: &Utf8Path, toolchain: &str, kind: MockLibraryKind) { + use crate::builder::{library_extension, library_prefix}; + + let lib_dir = kind.library_dir(target_dir, toolchain); + fs::create_dir_all(&lib_dir).expect("failed to create target library directory"); + + let filename = format!( + "{}whitaker_suite@{toolchain}{}", + library_prefix(), + library_extension() + ); + + let error_msg = match kind { + MockLibraryKind::Local => "failed to create mock library", + MockLibraryKind::Prebuilt { .. } => "failed to create prebuilt mock library", + }; + fs::write(lib_dir.join(filename), kind.content()).expect(error_msg); +} + +/// Helper to create a mock installed library in the target directory for tests. +fn create_mock_library(target_dir: &Utf8Path, toolchain: &str) { + create_mock_library_internal(target_dir, toolchain, MockLibraryKind::Local); +} + +fn create_mock_prebuilt_library(target_dir: &Utf8Path, toolchain: &str, target: &'static str) { + create_mock_library_internal(target_dir, toolchain, MockLibraryKind::Prebuilt { target }); +} + +// ------------------------------------------------------------------------- +// run_list tests +// ------------------------------------------------------------------------- + +#[rstest] +fn run_list_outputs_human_readable_format(temp_target: TempTarget) { + let args = ListArgs { + json: false, + target_dir: Some(temp_target.path.clone()), + }; + let mut stdout = Vec::new(); + + let result = run_list_with(&args, &mut stdout, || None); + + assert!(result.is_ok(), "expected success, got: {result:?}"); + let output = String::from_utf8_lossy(&stdout); + assert!(output.contains("No lints installed"), "got: {output}"); +} + +#[rstest] +#[case::json_format(true, &["toolchains", "\"active\""])] +#[case::human_format(false, &["nightly-2026-05-28", "whitaker_suite"])] +fn run_list_with_installed_library_includes_expected_output( + temp_target: TempTarget, + #[case] json: bool, + #[case] expected: &[&str], +) { + create_mock_library(&temp_target.path, "nightly-2026-05-28"); + let args = ListArgs { + json, + target_dir: Some(temp_target.path.clone()), + }; + let mut stdout = Vec::new(); + + let result = run_list_with(&args, &mut stdout, || Some("nightly-2026-05-28".to_owned())); + + assert!(result.is_ok(), "expected success, got: {result:?}"); + let output = String::from_utf8_lossy(&stdout); + for needle in expected { + assert!( + output.contains(needle), + "expected '{needle}' in output: {output}" + ); + } +} + +#[rstest] +fn run_list_finds_prebuilt_layout_libraries(temp_target: TempTarget) { + create_mock_prebuilt_library( + &temp_target.path, + "nightly-2026-05-28", + "x86_64-unknown-linux-gnu", + ); + let args = ListArgs { + json: false, + target_dir: Some(temp_target.path.clone()), + }; + let mut stdout = Vec::new(); + + let result = run_list_with(&args, &mut stdout, || Some("nightly-2026-05-28".to_owned())); + + assert!(result.is_ok(), "expected success, got: {result:?}"); + let output = String::from_utf8_lossy(&stdout); + assert!(output.contains("nightly-2026-05-28"), "got: {output}"); + assert!(output.contains("whitaker_suite"), "got: {output}"); +} + +#[rstest] +fn run_list_returns_write_failed_on_stdout_error(temp_target: TempTarget) { + let args = ListArgs { + json: false, + target_dir: Some(temp_target.path.clone()), + }; + let mut failing_stdout = FailingWriter; + + let result = run_list_with(&args, &mut failing_stdout, || None); + + let err = result.expect_err("expected error on write failure"); + assert!( + matches!(err, InstallerError::WriteFailed { .. }), + "expected WriteFailed error, got: {err:?}" + ); +} + +// ------------------------------------------------------------------------- +// detect_active_toolchain_in tests +// ------------------------------------------------------------------------- + +#[rstest] +fn detect_active_toolchain_in_returns_none_when_no_toolchain_file(temp_target: TempTarget) { + let result = detect_active_toolchain_in(&temp_target.path); + assert!( + result.is_none(), + "expected None for directory without rust-toolchain.toml" + ); +} + +#[rstest] +fn detect_active_toolchain_in_returns_channel_when_toolchain_file_exists(temp_target: TempTarget) { + // Create a rust-toolchain.toml file + let toolchain_content = r#"[toolchain] +channel = "nightly-2026-05-28" +"#; + fs::write( + temp_target.path.join("rust-toolchain.toml"), + toolchain_content, + ) + .expect("failed to write rust-toolchain.toml"); + + let result = detect_active_toolchain_in(&temp_target.path); + + assert_eq!(result, Some("nightly-2026-05-28".to_owned())); +} + +// ------------------------------------------------------------------------- +// determine_target_dir tests +// ------------------------------------------------------------------------- + +#[rstest] +fn determine_target_dir_returns_cli_value_when_provided(temp_target: TempTarget) { + let result = determine_target_dir_with(Some(&temp_target.path), || None); + + assert!(result.is_ok(), "expected success, got: {result:?}"); + assert_eq!(result.expect("already checked"), temp_target.path); +} + +#[rstest] +fn determine_target_dir_falls_back_to_default_when_cli_is_none(temp_target: TempTarget) { + let default_path = temp_target.path.clone(); + + let result = determine_target_dir_with(None, || Some(default_path.clone())); + + assert!(result.is_ok(), "expected success, got: {result:?}"); + assert_eq!(result.expect("already checked"), default_path); +} + +#[test] +fn determine_target_dir_returns_error_when_no_default_available() { + let result = determine_target_dir_with(None, || None); + + let err = result.expect_err("expected error when no default"); + assert!( + matches!(err, InstallerError::StagingFailed { .. }), + "expected StagingFailed error, got: {err:?}" + ); +} + +#[rstest] +fn determine_target_dir_prefers_cli_over_default(temp_target: TempTarget) { + let cli_path = temp_target.path.clone(); + let default_path = temp_target.path.join("should_not_be_used"); + + let result = determine_target_dir_with(Some(&cli_path), || Some(default_path)); + + assert!(result.is_ok(), "expected success, got: {result:?}"); + assert_eq!(result.expect("already checked"), cli_path); +} diff --git a/installer/src/toolchain.rs b/installer/src/toolchain/mod.rs similarity index 100% rename from installer/src/toolchain.rs rename to installer/src/toolchain/mod.rs