//! CI guard for test-suite conventions, the same ratchet idea as //! `migration_hygiene.rs` and `frontend_globals.rs` turned on the tests themselves. //! //! Each rule below has a `HIGH_WATER` count that may only go down. Fixing every //! existing violation in one sweep would be a 3,000-line diff nobody could review, //! so instead the seal freezes today's number and fails the build on a new one. //! When you clean up a file, lower the constant to the number the failure reports. //! Never raise one. //! //! Conventions and their rationale live in `CONTRIBUTING.md`; this file only //! counts. It deliberately does no parsing beyond line matching: a rule cheap //! enough to state as "how many lines look like this" is a rule that stays //! honest, and a clever AST check that drifts from the convention is worse than //! no check. //! //! Run with: cargo test --test test_hygiene use std::fs; use std::path::{Path, PathBuf}; /// Loose status assertions across `tests/`. /// /// `assert!(resp.status.is_success())` passes on a 200 when the handler promised /// 201, and `is_client_error()` passes on the 404 you get when a route silently /// disappears, which is exactly the regression the test existed to catch. The /// exact form, `assert_eq!(resp.status, 403)`, pins the contract. /// /// Convert a loose site by measuring rather than guessing: a temporary /// `#[track_caller]` probe in place of the predicate, one suite run, and every /// site whose observations agree on one code gets pinned to it. Compound /// conditions fall the same way, since half of /// `resp.status.is_redirection() || resp.status.is_success()` is dead the moment /// the handler settles on one. /// /// The sites left are the genuinely uncontracted ones, and each carries a /// comment saying why: branching on the status rather than asserting it, ranging /// over cases that answer with different codes, or a login path that renders a /// 200 page with the error in the body. /// /// The count reads all of `tests/`, not just `tests/workflows/`. Scoping it to /// the workflow modules leaves `harness/mod.rs` and `load/runner.rs` unsealed, /// which is where a route changing shape underneath the whole suite is least /// visible. /// /// `tests/load/scenarios.rs` is left alone deliberately. Its `is_success()` /// calls are a virtual user deciding whether to continue a cycle, not /// assertions, and they do not match the pattern below anyway (the receiver is a /// local, not `.status`). `tests/load/metrics.rs` is the same case: its report /// builder buckets observed statuses into errors and rejections, which counts /// what a run saw rather than asserting what a handler promised. That is the /// line to hold when a new site appears in the load harness. Read a status to /// classify or to branch, bind it to a local and say why; assert one, and pin /// the code. This file excludes itself for the obvious reason: the strings it /// searches for are in its own source. const LOOSE_STATUS_HIGH_WATER: usize = 6; /// `test_`-prefixed test functions across `src/` and `tests/`. /// /// The attribute already says it is a test, so the prefix is noise that pushes the /// behavior being asserted further from the start of the name. The rest of the /// suite names the outcome (`lookups_return_none_when_absent`); these are the /// stragglers. const TEST_PREFIX_HIGH_WATER: usize = 176; /// Workflow modules over [`MAX_MODULE_LINES`]. /// /// Past this a module stops being findable: nobody locates the existing test for /// a behavior, so they write a second one beside it. The fix is to split by /// domain, one module per feature domain. const OVERSIZED_MODULE_HIGH_WATER: usize = 10; /// The point at which a workflow module should have been split. const MAX_MODULE_LINES: usize = 800; const WORKFLOWS_DIR: &str = "tests/workflows"; /// The whole test tree: the harness and the load runner are as much a part of the /// suite's contract as the workflow modules are. const TESTS_DIR: &str = "tests"; #[test] fn loose_status_assertions_do_not_increase() { let mut per_file: Vec<(String, usize)> = Vec::new(); for path in rs_files(Path::new(TESTS_DIR)) { if file_name(&path) == "test_hygiene.rs" { continue; } let text = fs::read_to_string(&path).expect("read test module"); let n = text.matches(".status.is_success()").count() + text.matches(".status.is_client_error()").count(); if n > 0 { per_file.push((file_name(&path), n)); } } let total: usize = per_file.iter().map(|(_, n)| n).sum(); if total > LOOSE_STATUS_HIGH_WATER { per_file.sort_by_key(|(_, n)| std::cmp::Reverse(*n)); panic!( "loose status assertions rose from {LOOSE_STATUS_HIGH_WATER} to {total}.\n\ Assert the exact code the handler contracts: assert_eq!(resp.status, 403).\n\ Heaviest files: {:?}", &per_file[..per_file.len().min(5)], ); } assert_eq!( total, LOOSE_STATUS_HIGH_WATER, "loose status assertions fell to {total}. Lower LOOSE_STATUS_HIGH_WATER to {total}.", ); } #[test] fn prefixed_test_names_do_not_increase() { let mut found: Vec = Vec::new(); for dir in ["src", "tests"] { for path in rs_files(Path::new(dir)) { let text = fs::read_to_string(&path).expect("read rust file"); for line in text.lines() { let line = line.trim_start(); if line.starts_with("fn test_") || line.starts_with("async fn test_") { found.push(format!("{}: {line}", path.display())); } } } } let total = found.len(); assert!( total <= TEST_PREFIX_HIGH_WATER, "`test_`-prefixed test names rose from {TEST_PREFIX_HIGH_WATER} to {total}.\n\ Name the behavior instead: `returns_none_when_absent`, not `test_lookup`.\n\ Examples: {:?}", &found[..found.len().min(5)], ); assert_eq!( total, TEST_PREFIX_HIGH_WATER, "`test_` prefixes fell to {total}. Lower TEST_PREFIX_HIGH_WATER to {total}.", ); } #[test] fn oversized_workflow_modules_do_not_increase() { let mut over: Vec<(String, usize)> = rs_files(Path::new(WORKFLOWS_DIR)) .into_iter() .map(|p| { let lines = fs::read_to_string(&p) .expect("read workflow module") .lines() .count(); (file_name(&p), lines) }) .filter(|(_, lines)| *lines > MAX_MODULE_LINES) .collect(); let total = over.len(); if total > OVERSIZED_MODULE_HIGH_WATER { over.sort_by_key(|(_, n)| std::cmp::Reverse(*n)); panic!( "workflow modules over {MAX_MODULE_LINES} lines rose from \ {OVERSIZED_MODULE_HIGH_WATER} to {total}.\n\ Split the module by domain rather than growing it: {over:?}", ); } assert_eq!( total, OVERSIZED_MODULE_HIGH_WATER, "oversized modules fell to {total}. Lower OVERSIZED_MODULE_HIGH_WATER to {total}.", ); } /// Every test module states what surface it covers. A module with no `//!` header /// is one whose reason for existing lives only in whoever wrote it. #[test] fn every_workflow_module_has_a_doc_header() { let missing: Vec = rs_files(Path::new(WORKFLOWS_DIR)) .into_iter() .filter(|p| file_name(p) != "mod.rs") .filter(|p| { let text = fs::read_to_string(p).expect("read workflow module"); !text.lines().next().is_some_and(|l| l.starts_with("//!")) }) .map(|p| file_name(&p)) .collect(); assert!( missing.is_empty(), "workflow modules without a `//!` header: {missing:?}\n\ Say what surface the module covers and what would break if it were deleted.", ); } /// `#[ignore]` without a reason is coverage that silently stopped running while /// still costing compile time. The attribute takes a string; use it to say why, /// and how to run the test instead. #[test] fn every_ignored_test_states_a_reason() { let mut bare: Vec = Vec::new(); for dir in ["src", "tests"] { for path in rs_files(Path::new(dir)) { let text = fs::read_to_string(&path).expect("read rust file"); for (i, line) in text.lines().enumerate() { if line.trim() == "#[ignore]" { bare.push(format!("{}:{}", path.display(), i + 1)); } } } } assert!( bare.is_empty(), "`#[ignore]` with no reason string: {bare:?}\n\ Write `#[ignore = \"why, and how to run it\"]`.", ); } /// Every `.rs` file under `dir`, recursively, in a stable order. fn rs_files(dir: &Path) -> Vec { let mut out = Vec::new(); let Ok(entries) = fs::read_dir(dir) else { return out; }; for entry in entries.flatten() { let path = entry.path(); if path.is_dir() { out.extend(rs_files(&path)); } else if path.extension().is_some_and(|e| e == "rs") { out.push(path); } } out.sort(); out } fn file_name(path: &Path) -> String { path.file_name() .expect("workflow path has a file name") .to_string_lossy() .into_owned() }