Pre-merge "Tests touched" check ignores in-file Rust test modules #29

New issue

Closed

opened 2026-05-01 23:53:20 -07:00 by jwilger · 0 comments

jwilger commented

2026-05-01 23:53:20 -07:00

Owner

Symptom

The built-in pre-merge "Tests touched" check (crates/ar-review/src/pre_merge.rs) reports missing on PRs that do add or modify tests, when those tests live inside a #[cfg(test)] mod tests { ... } block in the same .rs file as the implementation.

Root cause

is_test_file (crates/ar-review/src/pre_merge.rs:224) decides "this is a test" purely from the file path:

last.contains("test") (filename basename)
/tests/, /__tests__/, /spec/ directory segments
.spec.ts / .spec.js suffixes

That heuristic matches crates/foo/tests/bar.rs and tests/integration.rs, but it does not match the project's documented convention (CLAUDE.md, "Development conventions"):

Pure parsing/formatting helpers go in #[cfg(test)] mod tests blocks alongside the code

For a diff that touches only crates/ar-review/src/pre_merge.rs and adds tests at the bottom of the same file, the check sees one changed file (pre_merge.rs) which is "source" but not "test" → the check fails even though tests were added.

Proposed fix

For .rs files where the path heuristic fails, inspect the diff hunks to see if any added/changed lines are inside (or introduce) a #[cfg(test)] / #[cfg(any(test, ...))] module — i.e. treat the file as having a test-touch when:

the diff adds a #[cfg(test)] attribute, or
the diff adds a #[test] / #[tokio::test] / #[rstest] etc. attribute, or
the changed lines fall inside an existing #[cfg(test)] mod ... block.

The simplest cut: scan added lines (+ lines, excluding +++ headers) for #[cfg(test)], #[test], or #[tokio::test] / #[rstest] / #[test_case] markers and treat that as a test-touch. False positives (commenting out a test) are tolerable; false negatives (today's behaviour) are the problem.

Cross-language note: the same shape exists in Python (test functions inside an implementation file via pytest) and Go (_test.go is enforced by the toolchain so this is a Rust-specific gap in practice). Scoping the fix to .rs files is fine for now.

Acceptance

A PR whose only change is adding #[test] functions inside an existing #[cfg(test)] mod tests block in src/foo.rs produces [x] Tests touched rather than [ ] Tests touched — missing.
A PR that touches src/foo.rs with no tests (no #[cfg(test)] / #[test] marker added) still fails the check.
Add a unit test in crates/ar-review/src/pre_merge.rs that feeds a synthetic diff containing only + #[test]\n+ fn ... lines on a src/*.rs path and asserts the check passes.

## Symptom The built-in pre-merge "Tests touched" check (`crates/ar-review/src/pre_merge.rs`) reports `missing` on PRs that *do* add or modify tests, when those tests live inside a `#[cfg(test)] mod tests { ... }` block in the same `.rs` file as the implementation. ## Root cause `is_test_file` (`crates/ar-review/src/pre_merge.rs:224`) decides "this is a test" purely from the file path: - `last.contains("test")` (filename basename) - `/tests/`, `/__tests__/`, `/spec/` directory segments - `.spec.ts` / `.spec.js` suffixes That heuristic matches `crates/foo/tests/bar.rs` and `tests/integration.rs`, but it does **not** match the project's documented convention (`CLAUDE.md`, "Development conventions"): > Pure parsing/formatting helpers go in `#[cfg(test)] mod tests` blocks alongside the code For a diff that touches only `crates/ar-review/src/pre_merge.rs` and adds tests at the bottom of the same file, the check sees one changed file (`pre_merge.rs`) which is "source" but not "test" → the check fails even though tests *were* added. ## Proposed fix For `.rs` files where the path heuristic fails, inspect the diff hunks to see if any added/changed lines are inside (or introduce) a `#[cfg(test)]` / `#[cfg(any(test, ...))]` module — i.e. treat the file as having a test-touch when: - the diff adds a `#[cfg(test)]` attribute, **or** - the diff adds a `#[test]` / `#[tokio::test]` / `#[rstest]` etc. attribute, **or** - the changed lines fall inside an existing `#[cfg(test)] mod ...` block. The simplest cut: scan added lines (`+` lines, excluding `+++` headers) for `#[cfg(test)]`, `#[test]`, or `#[tokio::test]` / `#[rstest]` / `#[test_case]` markers and treat that as a test-touch. False positives (commenting out a test) are tolerable; false negatives (today's behaviour) are the problem. Cross-language note: the same shape exists in Python (test functions inside an implementation file via `pytest`) and Go (`_test.go` is enforced by the toolchain so this is a Rust-specific gap in practice). Scoping the fix to `.rs` files is fine for now. ## Acceptance - A PR whose only change is adding `#[test]` functions inside an existing `#[cfg(test)] mod tests` block in `src/foo.rs` produces `[x] Tests touched` rather than `[ ] Tests touched — missing`. - A PR that touches `src/foo.rs` with no tests (no `#[cfg(test)]` / `#[test]` marker added) still fails the check. - Add a unit test in `crates/ar-review/src/pre_merge.rs` that feeds a synthetic diff containing only `+ #[test]\n+ fn ...` lines on a `src/*.rs` path and asserts the check passes.

jwilger referenced this issue from a pull request that will close it,

2026-05-02 12:28:39 -07:00

fix(review): detect inline Rust tests in pre-merge checks #36

jwilger closed this issue

2026-05-02 12:35:04 -07:00