max / makenotwork

9.2 KB · 221 lines History Blame Raw

1	//! CI guard for test-suite conventions, the same ratchet idea as
2	//! `migration_hygiene.rs` and `frontend_globals.rs` turned on the tests themselves.
3	//!
4	//! Each rule below has a `HIGH_WATER` count that may only go down. Fixing every
5	//! existing violation in one sweep would be a 3,000-line diff nobody could review,
6	//! so instead the seal freezes today's number and fails the build on a new one.
7	//! When you clean up a file, lower the constant to the number the failure reports.
8	//! Never raise one.
9	//!
10	//! Conventions and their rationale live in `CONTRIBUTING.md`; this file only
11	//! counts. It deliberately does no parsing beyond line matching: a rule cheap
12	//! enough to state as "how many lines look like this" is a rule that stays
13	//! honest, and a clever AST check that drifts from the convention is worse than
14	//! no check.
15	//!
16	//! Run with: cargo test --test test_hygiene
17
18	use std::fs;
19	use std::path::{Path, PathBuf};
20
21	/// Loose status assertions across `tests/`.
22	///
23	/// `assert!(resp.status.is_success())` passes on a 200 when the handler promised
24	/// 201, and `is_client_error()` passes on the 404 you get when a route silently
25	/// disappears, which is exactly the regression the test existed to catch. The
26	/// exact form, `assert_eq!(resp.status, 403)`, pins the contract.
27	///
28	/// Convert a loose site by measuring rather than guessing: a temporary
29	/// `#[track_caller]` probe in place of the predicate, one suite run, and every
30	/// site whose observations agree on one code gets pinned to it. Compound
31	/// conditions fall the same way, since half of
32	/// `resp.status.is_redirection() \|\| resp.status.is_success()` is dead the moment
33	/// the handler settles on one.
34	///
35	/// The sites left are the genuinely uncontracted ones, and each carries a
36	/// comment saying why: branching on the status rather than asserting it, ranging
37	/// over cases that answer with different codes, or a login path that renders a
38	/// 200 page with the error in the body.
39	///
40	/// The count reads all of `tests/`, not just `tests/workflows/`. Scoping it to
41	/// the workflow modules leaves `harness/mod.rs` and `load/runner.rs` unsealed,
42	/// which is where a route changing shape underneath the whole suite is least
43	/// visible.
44	///
45	/// `tests/load/scenarios.rs` is left alone deliberately. Its `is_success()`
46	/// calls are a virtual user deciding whether to continue a cycle, not
47	/// assertions, and they do not match the pattern below anyway (the receiver is a
48	/// local, not `.status`). `tests/load/metrics.rs` is the same case: its report
49	/// builder buckets observed statuses into errors and rejections, which counts
50	/// what a run saw rather than asserting what a handler promised. That is the
51	/// line to hold when a new site appears in the load harness. Read a status to
52	/// classify or to branch, bind it to a local and say why; assert one, and pin
53	/// the code. This file excludes itself for the obvious reason: the strings it
54	/// searches for are in its own source.
55	const LOOSE_STATUS_HIGH_WATER: usize = 6;
56
57	/// `test_`-prefixed test functions across `src/` and `tests/`.
58	///
59	/// The attribute already says it is a test, so the prefix is noise that pushes the
60	/// behavior being asserted further from the start of the name. The rest of the
61	/// suite names the outcome (`lookups_return_none_when_absent`); these are the
62	/// stragglers.
63	const TEST_PREFIX_HIGH_WATER: usize = 176;
64
65	const WORKFLOWS_DIR: &str = "tests/workflows";
66
67	/// The whole test tree: the harness and the load runner are as much a part of the
68	/// suite's contract as the workflow modules are.
69	const TESTS_DIR: &str = "tests";
70
71	#[test]
72	fn loose_status_assertions_do_not_increase() {
73	let mut per_file: Vec<(String, usize)> = Vec::new();
74	for path in rs_files(Path::new(TESTS_DIR)) {
75	if file_name(&path) == "test_hygiene.rs" {
76	continue;
77	}
78	let text = fs::read_to_string(&path).expect("read test module");
79	let n = text.matches(".status.is_success()").count()
80	+ text.matches(".status.is_client_error()").count();
81	if n > 0 {
82	per_file.push((file_name(&path), n));
83	}
84	}
85	let total: usize = per_file.iter().map(\|(_, n)\| n).sum();
86
87	if total > LOOSE_STATUS_HIGH_WATER {
88	per_file.sort_by_key(\|(_, n)\| std::cmp::Reverse(*n));
89	panic!(
90	"loose status assertions rose from {LOOSE_STATUS_HIGH_WATER} to {total}.\n\
91	Assert the exact code the handler contracts: assert_eq!(resp.status, 403).\n\
92	Heaviest files: {:?}",
93	&per_file[..per_file.len().min(5)],
94	);
95	}
96	assert_eq!(
97	total, LOOSE_STATUS_HIGH_WATER,
98	"loose status assertions fell to {total}. Lower LOOSE_STATUS_HIGH_WATER to {total}.",
99	);
100	}
101
102	#[test]
103	fn prefixed_test_names_do_not_increase() {
104	let mut found: Vec<String> = Vec::new();
105	for dir in ["src", "tests"] {
106	for path in rs_files(Path::new(dir)) {
107	let text = fs::read_to_string(&path).expect("read rust file");
108	for line in text.lines() {
109	let line = line.trim_start();
110	if line.starts_with("fn test_") \|\| line.starts_with("async fn test_") {
111	found.push(format!("{}: {line}", path.display()));
112	}
113	}
114	}
115	}
116	let total = found.len();
117
118	assert!(
119	total <= TEST_PREFIX_HIGH_WATER,
120	"`test_`-prefixed test names rose from {TEST_PREFIX_HIGH_WATER} to {total}.\n\
121	Name the behavior instead: `returns_none_when_absent`, not `test_lookup`.\n\
122	Examples: {:?}",
123	&found[..found.len().min(5)],
124	);
125	assert_eq!(
126	total, TEST_PREFIX_HIGH_WATER,
127	"`test_` prefixes fell to {total}. Lower TEST_PREFIX_HIGH_WATER to {total}.",
128	);
129	}
130
131	// The module-size rule that stood here is retired, as of 2026-09-03. It set
132	// MAX_MODULE_LINES = 800 and froze the count of `tests/workflows` files over it
133	// at 10. The astra sweep's `module-size` check now owns that budget:
134	// Apps/witchbroom/scripts/module-size.mjs against
135	// Apps/witchbroom/policy/module-size.toml.
136	//
137	// The sweep does the job on four axes this could not. Its budget counts
138	// PRODUCTION lines — non-comment, non-blank lines above the file's inline test
139	// module — so a file is not charged for being well tested, and a file that is
140	// wholly test code (under `tests/` or `benches/`, or named `tests.rs`) carries
141	// no budget at all. That rule, ruled by Max, is why none of the six files this
142	// seal counted are violations: they are integration tests. The sweep also
143	// covers the whole tree rather than one directory, so `src/` is guarded for the
144	// first time; it reports per-file line counts rather than a count of files over
145	// a threshold, so there is a gradient inside a violation; and its exemptions
146	// are per-file with a written reason, so cleaning up one file is not an edit to
147	// a shared constant that every parallel session collides on.
148	//
149	// Do not reinstate a line budget here. If a server module is too large, it
150	// shows up in the sweep's `module-size` cell, and the remedy is a per-file
151	// exemption with a reason or a split.
152
153	/// Every test module states what surface it covers. A module with no `//!` header
154	/// is one whose reason for existing lives only in whoever wrote it.
155	#[test]
156	fn every_workflow_module_has_a_doc_header() {
157	let missing: Vec<String> = rs_files(Path::new(WORKFLOWS_DIR))
158	.into_iter()
159	.filter(\|p\| file_name(p) != "mod.rs")
160	.filter(\|p\| {
161	let text = fs::read_to_string(p).expect("read workflow module");
162	!text.lines().next().is_some_and(\|l\| l.starts_with("//!"))
163	})
164	.map(\|p\| file_name(&p))
165	.collect();
166
167	assert!(
168	missing.is_empty(),
169	"workflow modules without a `//!` header: {missing:?}\n\
170	Say what surface the module covers and what would break if it were deleted.",
171	);
172	}
173
174	/// `#[ignore]` without a reason is coverage that silently stopped running while
175	/// still costing compile time. The attribute takes a string; use it to say why,
176	/// and how to run the test instead.
177	#[test]
178	fn every_ignored_test_states_a_reason() {
179	let mut bare: Vec<String> = Vec::new();
180	for dir in ["src", "tests"] {
181	for path in rs_files(Path::new(dir)) {
182	let text = fs::read_to_string(&path).expect("read rust file");
183	for (i, line) in text.lines().enumerate() {
184	if line.trim() == "#[ignore]" {
185	bare.push(format!("{}:{}", path.display(), i + 1));
186	}
187	}
188	}
189	}
190	assert!(
191	bare.is_empty(),
192	"`#[ignore]` with no reason string: {bare:?}\n\
193	Write `#[ignore = \"why, and how to run it\"]`.",
194	);
195	}
196
197	/// Every `.rs` file under `dir`, recursively, in a stable order.
198	fn rs_files(dir: &Path) -> Vec<PathBuf> {
199	let mut out = Vec::new();
200	let Ok(entries) = fs::read_dir(dir) else {
201	return out;
202	};
203	for entry in entries.flatten() {
204	let path = entry.path();
205	if path.is_dir() {
206	out.extend(rs_files(&path));
207	} else if path.extension().is_some_and(\|e\| e == "rs") {
208	out.push(path);
209	}
210	}
211	out.sort();
212	out
213	}
214
215	fn file_name(path: &Path) -> String {
216	path.file_name()
217	.expect("workflow path has a file name")
218	.to_string_lossy()
219	.into_owned()
220	}
221