max / makenotwork

9.2 KB · 238 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	/// Workflow modules over [`MAX_MODULE_LINES`].
66	///
67	/// Past this a module stops being findable: nobody locates the existing test for
68	/// a behavior, so they write a second one beside it. The fix is to split by
69	/// domain, one module per feature domain.
70	const OVERSIZED_MODULE_HIGH_WATER: usize = 10;
71
72	/// The point at which a workflow module should have been split.
73	const MAX_MODULE_LINES: usize = 800;
74
75	const WORKFLOWS_DIR: &str = "tests/workflows";
76
77	/// The whole test tree: the harness and the load runner are as much a part of the
78	/// suite's contract as the workflow modules are.
79	const TESTS_DIR: &str = "tests";
80
81	#[test]
82	fn loose_status_assertions_do_not_increase() {
83	let mut per_file: Vec<(String, usize)> = Vec::new();
84	for path in rs_files(Path::new(TESTS_DIR)) {
85	if file_name(&path) == "test_hygiene.rs" {
86	continue;
87	}
88	let text = fs::read_to_string(&path).expect("read test module");
89	let n = text.matches(".status.is_success()").count()
90	+ text.matches(".status.is_client_error()").count();
91	if n > 0 {
92	per_file.push((file_name(&path), n));
93	}
94	}
95	let total: usize = per_file.iter().map(\|(_, n)\| n).sum();
96
97	if total > LOOSE_STATUS_HIGH_WATER {
98	per_file.sort_by_key(\|(_, n)\| std::cmp::Reverse(*n));
99	panic!(
100	"loose status assertions rose from {LOOSE_STATUS_HIGH_WATER} to {total}.\n\
101	Assert the exact code the handler contracts: assert_eq!(resp.status, 403).\n\
102	Heaviest files: {:?}",
103	&per_file[..per_file.len().min(5)],
104	);
105	}
106	assert_eq!(
107	total, LOOSE_STATUS_HIGH_WATER,
108	"loose status assertions fell to {total}. Lower LOOSE_STATUS_HIGH_WATER to {total}.",
109	);
110	}
111
112	#[test]
113	fn prefixed_test_names_do_not_increase() {
114	let mut found: Vec<String> = Vec::new();
115	for dir in ["src", "tests"] {
116	for path in rs_files(Path::new(dir)) {
117	let text = fs::read_to_string(&path).expect("read rust file");
118	for line in text.lines() {
119	let line = line.trim_start();
120	if line.starts_with("fn test_") \|\| line.starts_with("async fn test_") {
121	found.push(format!("{}: {line}", path.display()));
122	}
123	}
124	}
125	}
126	let total = found.len();
127
128	assert!(
129	total <= TEST_PREFIX_HIGH_WATER,
130	"`test_`-prefixed test names rose from {TEST_PREFIX_HIGH_WATER} to {total}.\n\
131	Name the behavior instead: `returns_none_when_absent`, not `test_lookup`.\n\
132	Examples: {:?}",
133	&found[..found.len().min(5)],
134	);
135	assert_eq!(
136	total, TEST_PREFIX_HIGH_WATER,
137	"`test_` prefixes fell to {total}. Lower TEST_PREFIX_HIGH_WATER to {total}.",
138	);
139	}
140
141	#[test]
142	fn oversized_workflow_modules_do_not_increase() {
143	let mut over: Vec<(String, usize)> = rs_files(Path::new(WORKFLOWS_DIR))
144	.into_iter()
145	.map(\|p\| {
146	let lines = fs::read_to_string(&p)
147	.expect("read workflow module")
148	.lines()
149	.count();
150	(file_name(&p), lines)
151	})
152	.filter(\|(_, lines)\| *lines > MAX_MODULE_LINES)
153	.collect();
154	let total = over.len();
155
156	if total > OVERSIZED_MODULE_HIGH_WATER {
157	over.sort_by_key(\|(_, n)\| std::cmp::Reverse(*n));
158	panic!(
159	"workflow modules over {MAX_MODULE_LINES} lines rose from \
160	{OVERSIZED_MODULE_HIGH_WATER} to {total}.\n\
161	Split the module by domain rather than growing it: {over:?}",
162	);
163	}
164	assert_eq!(
165	total, OVERSIZED_MODULE_HIGH_WATER,
166	"oversized modules fell to {total}. Lower OVERSIZED_MODULE_HIGH_WATER to {total}.",
167	);
168	}
169
170	/// Every test module states what surface it covers. A module with no `//!` header
171	/// is one whose reason for existing lives only in whoever wrote it.
172	#[test]
173	fn every_workflow_module_has_a_doc_header() {
174	let missing: Vec<String> = rs_files(Path::new(WORKFLOWS_DIR))
175	.into_iter()
176	.filter(\|p\| file_name(p) != "mod.rs")
177	.filter(\|p\| {
178	let text = fs::read_to_string(p).expect("read workflow module");
179	!text.lines().next().is_some_and(\|l\| l.starts_with("//!"))
180	})
181	.map(\|p\| file_name(&p))
182	.collect();
183
184	assert!(
185	missing.is_empty(),
186	"workflow modules without a `//!` header: {missing:?}\n\
187	Say what surface the module covers and what would break if it were deleted.",
188	);
189	}
190
191	/// `#[ignore]` without a reason is coverage that silently stopped running while
192	/// still costing compile time. The attribute takes a string; use it to say why,
193	/// and how to run the test instead.
194	#[test]
195	fn every_ignored_test_states_a_reason() {
196	let mut bare: Vec<String> = Vec::new();
197	for dir in ["src", "tests"] {
198	for path in rs_files(Path::new(dir)) {
199	let text = fs::read_to_string(&path).expect("read rust file");
200	for (i, line) in text.lines().enumerate() {
201	if line.trim() == "#[ignore]" {
202	bare.push(format!("{}:{}", path.display(), i + 1));
203	}
204	}
205	}
206	}
207	assert!(
208	bare.is_empty(),
209	"`#[ignore]` with no reason string: {bare:?}\n\
210	Write `#[ignore = \"why, and how to run it\"]`.",
211	);
212	}
213
214	/// Every `.rs` file under `dir`, recursively, in a stable order.
215	fn rs_files(dir: &Path) -> Vec<PathBuf> {
216	let mut out = Vec::new();
217	let Ok(entries) = fs::read_dir(dir) else {
218	return out;
219	};
220	for entry in entries.flatten() {
221	let path = entry.path();
222	if path.is_dir() {
223	out.extend(rs_files(&path));
224	} else if path.extension().is_some_and(\|e\| e == "rs") {
225	out.push(path);
226	}
227	}
228	out.sort();
229	out
230	}
231
232	fn file_name(path: &Path) -> String {
233	path.file_name()
234	.expect("workflow path has a file name")
235	.to_string_lossy()
236	.into_owned()
237	}
238