From d8eed9a80cfca3ec30c836d8dad603c44325344a Mon Sep 17 00:00:00 2001 From: leynos Date: Sat, 29 Aug 2026 01:09:18 +0200 Subject: [PATCH 1/9] Adopt directory-based module structure Move the remaining self-named module files to mod.rs form (install_flow, toolchain) and extract oversized in-src test modules into sibling files (list, workspace, downloader, the non-UTF-8 boundary tests, the dependency-binary fakes, and the deps repository tests), each wired with the repo's `#[cfg(test)] #[path]` convention and opening with an explanatory `//!` line. The moves are content-preserving: extracted test bodies are this tree's code verbatim; only the module structure changes. This keeps every module inside the 400-line budget the lint configuration later in this stack will enforce, without entangling the structural review with lint fixes. --- .../dependency_binaries/install/downloader.rs | 113 +------- .../install/downloader_tests.rs | 109 ++++++++ installer/src/deps/tests.rs | 100 +------ installer/src/deps/tests_repository.rs | 150 +++++++++++ .../{install_flow.rs => install_flow/mod.rs} | 0 installer/src/list.rs | 252 +----------------- installer/src/list_tests.rs | 248 +++++++++++++++++ .../src/{toolchain.rs => toolchain/mod.rs} | 0 8 files changed, 514 insertions(+), 458 deletions(-) create mode 100644 installer/src/dependency_binaries/install/downloader_tests.rs create mode 100644 installer/src/deps/tests_repository.rs rename installer/src/{install_flow.rs => install_flow/mod.rs} (100%) create mode 100644 installer/src/list_tests.rs rename installer/src/{toolchain.rs => toolchain/mod.rs} (100%) 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..06a5f5ee9 --- /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/tests.rs b/installer/src/deps/tests.rs index 1049fd2aa..91358b25e 100644 --- a/installer/src/deps/tests.rs +++ b/installer/src/deps/tests.rs @@ -294,103 +294,6 @@ 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"); - - 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 { @@ -565,3 +468,6 @@ fn an_unresolvable_repository_context_still_refuses_a_source_build() { ); 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..0d86976cb --- /dev/null +++ b/installer/src/deps/tests_repository.rs @@ -0,0 +1,150 @@ +//! Repository-release installation tests for the Dylint dependency tools. +//! +//! These exercise the `install_tool` / `repository_install_context` path and +//! the helpers only it needs, split from `tests.rs` to keep each file inside +//! the repository's 400-line module limit. + +/// 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, + }, + ) + .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 100% rename from installer/src/install_flow.rs rename to installer/src/install_flow/mod.rs 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 From 337fd659b0c3b115b96d4551983a48e32b06c795 Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 27 Sep 2026 02:02:32 +0200 Subject: [PATCH 2/9] Restore rustfmt-canonical layout in the extracted downloader tests The directory-module rebase rebuilt `downloader_tests.rs` by dedenting the parent's inline `mod tests` body. rustfmt had laid that body out at the deeper inline indentation, so a call whose argument list crossed the width limit kept its break at the old indent column. After dedenting, the reflow no longer matched rustfmt's choice and `make check-fmt` reported a diff at the `destination_dir` binding. Rejoin that line so the extraction is formatting-neutral. The edit moves only whitespace: the token stream is identical, with the same 527 tokens in the same order, and the call is applied to the same arguments and bound to the same name. Co-Authored-By: Claude Code --- installer/src/dependency_binaries/install/downloader_tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/installer/src/dependency_binaries/install/downloader_tests.rs b/installer/src/dependency_binaries/install/downloader_tests.rs index 06a5f5ee9..ee1813531 100644 --- a/installer/src/dependency_binaries/install/downloader_tests.rs +++ b/installer/src/dependency_binaries/install/downloader_tests.rs @@ -72,8 +72,8 @@ fn open_destination_dir_writes_into_the_destination_parent_not_the_cwd() { // 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 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()); From 49316ff5ff15f6b2c759f1fbc0956b7ff64cb3d4 Mon Sep 17 00:00:00 2001 From: leynos Date: Mon, 12 Oct 2026 17:04:08 +0200 Subject: [PATCH 3/9] Keep the deps test modules within the line budget The restack onto the advanced parent left `installer/src/deps/tests.rs` at 467 lines. The parent had grown the file to 561 while my extraction split it at the point that was correct for the older content, and the two changes overlapped: the repository-install tests ended up declared in both files. Split the file along its two independent concerns instead. `policy_tests.rs` owns the source-fallback refusal tests and the two private helpers only they use; `tests_repository.rs` owns the repository-installer tests. `tests.rs` keeps the shared prologue and declares both children, so the private helpers stay reachable through `super::*`. Every module is now inside the repository's 400-line limit: tests.rs is 294 lines, policy_tests.rs 140, tests_repository.rs 169. Co-Authored-By: Claude Code --- installer/src/deps/policy_tests.rs | 141 ++++++++++++++++++++ installer/src/deps/tests.rs | 176 +------------------------ installer/src/deps/tests_repository.rs | 27 +++- 3 files changed, 166 insertions(+), 178 deletions(-) create mode 100644 installer/src/deps/policy_tests.rs 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 91358b25e..9bb135a6b 100644 --- a/installer/src/deps/tests.rs +++ b/installer/src/deps/tests.rs @@ -294,180 +294,8 @@ fn install_dylint_tools_skips_dylint_link_when_cargo_dylint_source_build_install 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 = "policy_tests.rs"] +mod policy; #[path = "tests_repository.rs"] mod repository; diff --git a/installer/src/deps/tests_repository.rs b/installer/src/deps/tests_repository.rs index 0d86976cb..07ccc6bcc 100644 --- a/installer/src/deps/tests_repository.rs +++ b/installer/src/deps/tests_repository.rs @@ -1,8 +1,27 @@ //! Repository-release installation tests for the Dylint dependency tools. //! -//! These exercise the `install_tool` / `repository_install_context` path and -//! the helpers only it needs, split from `tests.rs` to keep each file inside -//! the repository's 400-line module limit. +//! 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. @@ -101,7 +120,6 @@ fn install_dylint_tools_falls_back_when_repository_dylint_link_install_fails() { executor.assert_finished(); } - #[test] fn install_tool_errors_when_dependency_manifest_entry_is_missing() { let missing_tool = DependencyTool { @@ -130,6 +148,7 @@ fn install_tool_errors_when_dependency_manifest_entry_is_missing() { ), cargo_fallback_mode: InstallMode::Binstall, quiet: false, + no_source_fallback: false, }, ) .expect_err("missing dependency manifest entry should be an install error"); From 329db8459cc065e602ce9c971cc40ca72465eb4c Mon Sep 17 00:00:00 2001 From: leynos Date: Thu, 15 Oct 2026 21:18:36 +0200 Subject: [PATCH 4/9] Document the module layout and test placement rules CodeRabbit's pre-merge "Developer Documentation" check failed on this branch: the change renames two installer modules to `mod.rs` form and extracts inline test modules into sibling files, and none of those conventions was written down. Add a `## Module layout and test placement` section to the developers' guide covering the 400-line budget and its rationale, the directory-module `mod.rs` convention, the three shapes of extracted test module, and where the budget is enforced. Every claim is taken from the tree rather than asserted: 46 tracked `mod.rs` files outside `vendor/` and 65 files carrying a `#[path = "` anchor show the convention is repository-wide, and the threshold is the `module_max_lines` dylint lint's 400-line default, not a Clippy built-in. Record the one variation as well. `installer/src/deps/tests.rs` is itself declared `#[cfg(test)] mod tests;`, so its children omit the attribute and take their names from the `#[path]` files instead. Co-Authored-By: Claude Code --- docs/developers-guide.md | 96 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 96 insertions(+) diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 35ab73668..942e3b39f 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -1404,6 +1404,102 @@ 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 +budget is enforced mechanically by `module_max_lines`, one of Whitaker's own +dylint lints, which measures the number of source lines occupied by a module +and warns when the count exceeds a configurable `max_lines` threshold. That +threshold defaults to 400, and the canonical configuration documented in +[`docs/whitaker-cli-design.md`](./whitaker-cli-design.md) states it explicitly: + +```toml +[module_max_lines] +max_lines = 400 +``` + +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. + +### 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 used in +this repository: 46 `mod.rs` files are tracked outside `vendor/`. + +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, with the module path kept +intact by an explicit `#[path]` attribute: + +```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. Both + `installer/src/install_flow/mod.rs` and `installer/src/toolchain/mod.rs` end + with that declaration. +- **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 it keeps the +test module's name, `tests`, and therefore its module path unchanged. `super::` +and `crate::` imports inside the tests keep resolving, `cargo test` filter +paths keep matching, and 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 are therefore declared without the attribute, and take their names +from the `#[path]` files rather than the parent's `tests`: + +```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 enforced + +`make lint` runs +`cargo clippy --workspace --all-targets --all-features -- -D warnings`. The +`module_max_lines` lint is one of Whitaker's own dylint lints, wired in through +the workspace's lint configuration. It is not a rustc or Clippy built-in, so +the threshold is set as TOML under `[module_max_lines]`, not by a compiler flag. + ## Python script interpreter convention Every Python script that a workflow or the `Makefile` runs by path carries the From 79677e9e78ade038d61512c247169dbfa036c95c Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 16 Oct 2026 01:34:49 +0200 Subject: [PATCH 5/9] Add ADR 006 and correct a false enforcement claim Add ADR 006, which records the directory-based module layout the previous commit only described: mod.rs-rooted directory modules, and extraction of an oversized inline test module into a sibling file wired with `#[cfg(test)] #[path = "..."] mod ...;`. Two alternatives are recorded as rejected, including the self-named `name.rs` next to `name/` form, which no path in this repository uses. CodeRabbit's Developer Documentation pre-merge row required the structural decision to be recorded in a design document or an ADR. The requirement was previously unreadable because the summary table truncates the cell; the same row appears in full in the comment's per-check details block. ADRs 001-005 were registered in docs/contents.md, so 006 is added there too. Correct an unsupported claim in docs/developers-guide.md. The section added in the previous commit said the 400-line budget "is enforced mechanically by module_max_lines" and that the lint is "wired in through the workspace's lint configuration". Neither holds. `make lint` is a cargo doc pass plus cargo clippy, with no dylint step; the only dylint sub-command in the repository is a `dylint list` in publish-check that enumerates built libraries and lints nothing; the root dylint.toml configures no_std_fs_operations alone; and Cargo.toml has no [workspace.lints] table. The budget is held by review until the self-linting gate lands. The section is retitled "Where the budget is checked" and links ADR 006. Removing the stale [module_max_lines] TOML block from a file that doc_extraction reads is safe: find_block_containing returns the first match, which comes from docs/users-guide.md. Docs-only change; gated with check-fmt, markdownlint, nixie, lint, typecheck, and the behaviour_docs integration test, which owns the doc_extraction module tests that read these files. Co-Authored-By: Claude Code --- docs/adr-006-directory-based-module-layout.md | 137 ++++++++++++++++++ docs/contents.md | 4 + docs/developers-guide.md | 56 +++++-- 3 files changed, 182 insertions(+), 15 deletions(-) create mode 100644 docs/adr-006-directory-based-module-layout.md 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..ac2db1f32 --- /dev/null +++ b/docs/adr-006-directory-based-module-layout.md @@ -0,0 +1,137 @@ +# 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 does not use this form anywhere: 46 `mod.rs` files are +tracked outside `vendor/`, and no path in the tree has both `name.rs` and +`name/mod.rs`. Adopting a second convention for the same job 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, and an +explicit `#[path]` attribute holds its module path still: + +```rust +#[cfg(test)] +#[path = "list_tests.rs"] +mod tests; +``` + +The attribute is what makes the move reviewable. Without it, moving `mod tests` +into another file renames the module, and every `super::` reference inside the +tests, together with every `cargo test` filter that names the old path, +silently changes meaning. With it, the test bodies are byte-identical across +the move and the extraction can be checked as a move. + +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 own children take the +attribute-free form, drawing their names from the `#[path]` files rather than +from a parent `tests`. + +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 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 +`#[path]` holds the module path fixed, 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 942e3b39f..6a9214f27 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -1409,21 +1409,21 @@ make fmt # Apply formatting ### The 400-line module budget `AGENTS.md` states that no single code file may be longer than 400 lines. The -budget is enforced mechanically by `module_max_lines`, one of Whitaker's own -dylint lints, which measures the number of source lines occupied by a module -and warns when the count exceeds a configurable `max_lines` threshold. That -threshold defaults to 400, and the canonical configuration documented in -[`docs/whitaker-cli-design.md`](./whitaker-cli-design.md) states it explicitly: - -```toml -[module_max_lines] -max_lines = 400 -``` +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 @@ -1492,13 +1492,39 @@ 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 enforced +### 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`. The -`module_max_lines` lint is one of Whitaker's own dylint lints, wired in through -the workspace's lint configuration. It is not a rustc or Clippy built-in, so -the threshold is set as TOML under `[module_max_lines]`, not by a compiler flag. +`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, the root `dylint.toml` +configures `[no_std_fs_operations]` alone, and there is no `[workspace.lints]` +table in `Cargo.toml`. CI's one use of the dylint host tools is the +`make publish-check` step, whose 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 From 25c8d7a730b0adfa866b34ca0ed388a59e5f156e Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 16 Oct 2026 01:57:43 +0200 Subject: [PATCH 6/9] Correct a false claim about workspace lint tables Fix an error introduced in 0d9d377. The developers' guide said "there is no [workspace.lints] table in Cargo.toml". That is false: Cargo.toml:185 declares [workspace.lints.rust] and Cargo.toml:191 declares [workspace.lints.rustdoc], and two manifests inherit them through `[lints] workspace = true`. The error came from grepping for the literal string `[workspace.lints]` including the closing bracket, which matches only an exactly-empty table name and so cannot match a subtable. The conclusion was right for the wrong reason: neither table names module_max_lines, so the budget is still not enforced. The paragraph now states the tables exist, that they configure rustc and rustdoc lints, and that neither names module_max_lines. It also drops a second over-claim in the same sentence: ci.yml:302 does restore the dylint host tools, so saying CI's "one use" of them is the publish-check step conflated restoring the tools with invoking them. They are restored for that step, whose single dylint invocation is the `dylint list --no-metadata --no-build` that enumerates built libraries and lints nothing. Docs-only change; gated with check-fmt, markdownlint, nixie, lint and typecheck, and verified against the tree rather than restated. Co-Authored-By: Claude Code --- docs/developers-guide.md | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 6a9214f27..9228e0d83 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -1502,10 +1502,12 @@ 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, the root `dylint.toml` -configures `[no_std_fs_operations]` alone, and there is no `[workspace.lints]` -table in `Cargo.toml`. CI's one use of the dylint host tools is the -`make publish-check` step, whose single `dylint` invocation is +`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. From 096981857a12a07d6339854fbfa6838751ec9e4a Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 16 Oct 2026 11:32:27 +0200 Subject: [PATCH 7/9] Correct two false module-layout claims Fix errors I introduced in 2991148 and 0d9d377. Both were asserted without being checked, and both were repeated in the developers' guide and ADR 006, so the same mistake shipped twice. 1. "attribute-free form, drawing their names from the #[path] files" described deps/tests.rs's children as attribute-free while naming #[path] as the source of their names. That is self-contradictory: #[path] is the attribute that names them. At installer/src/deps/tests.rs:291-295 the children do carry #[path]; the attribute they lack is #[cfg(test)], because deps/mod.rs:548 already gates the whole file. The #[path] is load-bearing, since no deps/tests/ directory exists for a bare `mod` to resolve into. 2. "The repository does not use this form anywhere" was false. Two production modules reach a child through a same-named directory: installer/src/cli.rs:182 resolves `mod source_policy;` to cli/source_policy.rs, and crates/conditional_max_n_branches/src/driver.rs:24 resolves `mod numeric;` to driver/numeric.rs. Resolution is unambiguous -- neither installer/src/source_policy.rs nor crates/conditional_max_n_branches/src/numeric.rs exists. Both were already present at the base commit, so this PR did not introduce them. The supporting figures verified exactly and the Option B rejection still stands; only the stated reason was wrong. The documents now name the two outliers explicitly as things not to copy, rather than denying them. Docs-only change; gated with check-fmt, markdownlint, nixie, lint, typecheck and the behaviour_docs target, whose DOC_PATHS pins docs/developers-guide.md as a test input. Co-Authored-By: Claude Code --- docs/adr-006-directory-based-module-layout.md | 19 ++++++++++++------- docs/developers-guide.md | 13 +++++++++---- 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/docs/adr-006-directory-based-module-layout.md b/docs/adr-006-directory-based-module-layout.md index ac2db1f32..92929f47b 100644 --- a/docs/adr-006-directory-based-module-layout.md +++ b/docs/adr-006-directory-based-module-layout.md @@ -55,10 +55,15 @@ the same file with no mechanism to separate them later. ### Option B: extract tests but use the self-named sibling form -Rejected. The repository does not use this form anywhere: 46 `mod.rs` files are -tracked outside `vendor/`, and no path in the tree has both `name.rs` and -`name/mod.rs`. Adopting a second convention for the same job would make a -module's location depend on which convention its author happened to use. +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 @@ -98,9 +103,9 @@ above the extracted module already restricts it to test builds: `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 own children take the -attribute-free form, drawing their names from the `#[path]` files rather than -from a parent `tests`. +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. diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 9228e0d83..7fefd679a 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -1429,8 +1429,12 @@ were rejected, is recorded in 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 used in -this repository: 46 `mod.rs` files are tracked outside `vendor/`. +`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 @@ -1475,8 +1479,9 @@ 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 are therefore declared without the attribute, and take their names -from the `#[path]` files rather than the parent's `tests`: +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"] From 31acbdffa58801c35223e739e5a5e9b0a720149f Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 16 Oct 2026 12:17:21 +0200 Subject: [PATCH 8/9] Drop a #[path] anchor broken by the rename `#[path]` resolves relative to the directory containing the declaring file, so a file module and a directory module resolve the same attribute to different paths. After rebasing onto f3a457b2, the base's anchor survived the rename of `installer/src/install_flow.rs` to `installer/src/install_flow/mod.rs`, doubling its leading segment to `install_flow/install_flow/...`: failed to resolve mod `dependency_check_tests`: .../install_flow/install_flow/dependency_check_tests.rs does not exist `check-fmt` caught this because rustfmt resolves every `mod` declaration, whereas a build without `--all-targets` does not reach it. Delete the attribute rather than retarget it. Inside a directory module a bare `mod dependency_check_tests;` resolves to the identical file, so the anchor carried no information; the sibling `mod tests;` above it is already bare, and the convention recorded in ADR 006 confines the attribute to the file-module case. Deleting it also removes a manual anchor that ADR 006 already names as a staleness risk, and returns the tree-wide anchor count to the 65 both the ADR and the developers' guide already state. Correct two claims this rebase invalidated: the guide no longer asserts that `install_flow/mod.rs` ends with `mod tests;`, and ADR 006 now records the module-form hazard alongside its other `#[path]` risks. A sweep of all 81 `#[path]` anchors across 542 tracked `.rs` files finds every one resolving to an existing file, so the hazard was confined to this case. Co-Authored-By: Claude Code --- docs/adr-006-directory-based-module-layout.md | 9 +++++++++ docs/developers-guide.md | 7 ++++--- installer/src/install_flow/mod.rs | 1 - 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/docs/adr-006-directory-based-module-layout.md b/docs/adr-006-directory-based-module-layout.md index 92929f47b..8ca70d99a 100644 --- a/docs/adr-006-directory-based-module-layout.md +++ b/docs/adr-006-directory-based-module-layout.md @@ -115,6 +115,15 @@ satisfies the repository rule that every module documents its purpose. - `#[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 diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 7fefd679a..4de6fb4fc 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -1462,9 +1462,10 @@ Three shapes of this appear in the tree. `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. Both - `installer/src/install_flow/mod.rs` and `installer/src/toolchain/mod.rs` end - with that declaration. + `#[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`. diff --git a/installer/src/install_flow/mod.rs b/installer/src/install_flow/mod.rs index 2e2c6c36e..1895fc577 100644 --- a/installer/src/install_flow/mod.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; From a98b5a3afc14a801e1e9394b6d024b2750ace48f Mon Sep 17 00:00:00 2001 From: leynos Date: Sat, 17 Oct 2026 20:41:53 +0200 Subject: [PATCH 9/9] Correct the module name/file attribution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The module layout documents claimed that `#[path]` holds a module's path still, and that relying on the default location would break `super::` inside a renamed test module. Compile probes show both claims are wrong. `mod tests;` fixes the module name, and with it the path `crate::list::tests`; `#[path]` selects the file that backs it. A bare `mod tests;` already yields that name and path, so the attribute creates neither. What the attribute supplies is the placement: left alone, a bare `mod tests;` in `list.rs` resolves to `list/tests.rs`, and naming the declaration after the file resolves to `list/list_tests.rs`. Neither reaches a sibling, so only the attribute puts the file beside `list.rs` while keeping the module named `tests`. Renaming the module would not break `super::` on its own, because the module stays a child of `list`. Only absolute paths such as `crate::list::tests::…` and `cargo test` filters naming `tests` break. Correct all four mirrors of the claim: the decision outcome and the architectural rationale in ADR 006, and the sibling-test-modules lead-in and preferred-form paragraph in the developers' guide. The decision itself is unchanged; only its stated justification is. Co-Authored-By: Claude Code --- docs/adr-006-directory-based-module-layout.md | 39 ++++++++++++------- docs/developers-guide.md | 27 ++++++++----- 2 files changed, 44 insertions(+), 22 deletions(-) diff --git a/docs/adr-006-directory-based-module-layout.md b/docs/adr-006-directory-based-module-layout.md index 8ca70d99a..490373a35 100644 --- a/docs/adr-006-directory-based-module-layout.md +++ b/docs/adr-006-directory-based-module-layout.md @@ -75,8 +75,9 @@ 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, and an -explicit `#[path]` attribute holds its module path still: +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)] @@ -84,11 +85,23 @@ explicit `#[path]` attribute holds its module path still: mod tests; ``` -The attribute is what makes the move reviewable. Without it, moving `mod tests` -into another file renames the module, and every `super::` reference inside the -tests, together with every `cargo test` filter that names the old path, -silently changes meaning. With it, the test bodies are byte-identical across -the move and the extraction can be checked as a move. +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: @@ -138,12 +151,12 @@ satisfies the repository rule that every module documents its purpose. ## 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 -`#[path]` holds the module path fixed, 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. +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 diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 4de6fb4fc..478cb04a4 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -1443,8 +1443,8 @@ directory module with its own `mod.rs`, holding `test_helpers.rs` and a further ### 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, with the module path kept -intact by an explicit `#[path]` attribute: +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)] @@ -1469,13 +1469,22 @@ Three shapes of this appear in the tree. - **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 it keeps the -test module's name, `tests`, and therefore its module path unchanged. `super::` -and `crate::` imports inside the tests keep resolving, `cargo test` filter -paths keep matching, and 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. +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