From b9d6ed05ffb17366f42515d7f093e2cee945d679 Mon Sep 17 00:00:00 2001 From: SaschaBa <18143567+SaschaOnTour@users.noreply.github.com> Date: Sat, 30 May 2026 10:07:23 +0200 Subject: [PATCH] fix(test-detection)route test recognition through one cfg-test set (framework attrs + per-crate tests/), fix DEAD_CODE on test entry points, remove unsound --diff mode (v1.3.0) --- CHANGELOG.md | 103 ++++++ Cargo.lock | 2 +- Cargo.toml | 2 +- README.md | 2 +- book/ai-coding-workflow.md | 4 +- book/ci-integration.md | 13 +- book/code-reuse.md | 2 +- book/function-quality.md | 2 +- book/getting-started.md | 1 - book/reference-cli.md | 4 - book/reference-output-formats.md | 6 - book/reference-rules.md | 2 +- src/adapters/analyzers/dry/fragments.rs | 10 +- src/adapters/analyzers/dry/functions.rs | 15 +- src/adapters/analyzers/dry/match_patterns.rs | 11 +- src/adapters/analyzers/dry/mod.rs | 4 +- src/adapters/analyzers/dry/tests/dead_code.rs | 31 ++ src/adapters/analyzers/dry/tests/fragments.rs | 32 ++ src/adapters/analyzers/dry/tests/functions.rs | 49 +++ .../analyzers/dry/tests/match_patterns.rs | 27 ++ src/adapters/analyzers/dry/tests/wildcards.rs | 40 +++ src/adapters/analyzers/dry/wildcards.rs | 25 +- src/adapters/analyzers/structural/btc.rs | 8 + src/adapters/analyzers/structural/deh.rs | 8 +- src/adapters/analyzers/structural/iet.rs | 6 + src/adapters/analyzers/structural/mod.rs | 29 +- src/adapters/analyzers/structural/nms.rs | 8 + src/adapters/analyzers/structural/slm.rs | 8 + .../analyzers/structural/tests/btc.rs | 26 +- .../analyzers/structural/tests/deh.rs | 22 +- .../analyzers/structural/tests/iet.rs | 25 +- .../analyzers/structural/tests/nms.rs | 18 +- src/adapters/analyzers/structural/tests/oi.rs | 26 +- .../analyzers/structural/tests/root.rs | 10 +- .../analyzers/structural/tests/sit.rs | 20 +- .../analyzers/structural/tests/slm.rs | 16 +- src/adapters/analyzers/tq/coverage.rs | 10 +- src/adapters/analyzers/tq/tests/coverage.rs | 50 ++- src/adapters/shared/cfg_test.rs | 15 +- src/adapters/shared/cfg_test_files.rs | 102 +++++- src/adapters/shared/tests/cfg_test.rs | 59 ++++ src/adapters/shared/tests/cfg_test_files.rs | 313 ++++++++++++++++++ src/adapters/shared/tests/mod.rs | 1 + src/adapters/source/filesystem.rs | 64 ---- src/adapters/source/tests/filesystem.rs | 70 ---- src/app/tests/run.rs | 20 -- src/cli/mod.rs | 5 - src/lib.rs | 18 - 48 files changed, 1072 insertions(+), 272 deletions(-) create mode 100644 src/adapters/shared/tests/cfg_test.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index ad1c6f2d..9fd07110 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,109 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [1.3.0] - 2026-05-29 + +Minor release (one breaking removal — see below): **test-entry-point +recognition routed through one authoritative cfg-test file set** — +framework test attributes and per-crate integration-test directories are +now recognised everywhere, fixing a `DEAD_CODE` false-positive on +`tests/**` integration-test entry points. Package roots are identified by their crate-root file +(`src/lib.rs` / `src/main.rs` / autobinary `src/bin/.rs`) derived +from the parsed `.rs` set — a `tests/` directory only counts when its +owning directory is such a root — and the DRY collectors consume the +shared set instead of their own path strings. +Failing-first regression tests in `src/adapters/shared/tests/cfg_test.rs`, +`src/adapters/shared/tests/cfg_test_files.rs`, +`src/adapters/analyzers/dry/tests/dead_code.rs`, +`src/adapters/analyzers/dry/tests/functions.rs`, and +`src/adapters/analyzers/dry/tests/wildcards.rs`. + +### Fixed (test detection) + +- **`DEAD_CODE` no longer false-flags integration-test entry points + under a crate's `tests/` directory.** Two narrow heuristics stacked + up: (1) `has_test_attr` only recognised the bare `#[test]`, so + `#[tokio::test]` / `#[rstest]` / `#[test_case]` functions were not + seen as test roots; (2) the cfg-test file detector classified only + workspace-root `tests/**`, missing per-crate + `crates//tests/**` integration binaries. A `#[tokio::test]` + entry point in `crates/sv-utility-retry/tests/integration.rs` was + reported as dead code while the same function in + `src/**/*_tests.rs` was not. Both narrownesses are removed. + +### Changed (unified test recognition) + +- **`has_test_attr` (`adapters/shared/cfg_test.rs`) now recognises + framework test attributes.** Matches any attribute whose path ends in + `test` (`#[test]`, `#[tokio::test]`, `#[async_std::test]`, + `#[googletest::test]`, …) plus the renamed macros `#[rstest]` and + `#[test_case]`. Shared by all seven dimensions, so the broadened + recognition applies uniformly (IOSP/complexity/DRY/SRP/coupling/TQ/ + architecture leniency for test functions). +- **One authoritative cfg-test file set; the directory rule feeds it and + nothing else, and is derived from real package roots.** + `cfg_test_files` first computes the package roots present in the parsed + tree by their **crate-root file** — `src/lib.rs`, `src/main.rs`, or an + autobinary `src/bin/.rs` (`""` for the analysis-root crate). This + is Cargo's defining marker of a package, taken from the `.rs` set with + no manifest/filesystem access. `cfg_test_files::is_integration_test_path` + accepts a `tests/` file when the owner of **any** `tests/` component in + its path is one of those roots (so a package nested under a directory + literally named `tests`, `fixtures/tests/retry/tests/it.rs`, is matched at + its real root). This matches `tests/**` and per-crate + `crates//tests/**`, but NOT a `tests/` directory whose owner has + no crate root (`fixtures/tests/**`, `tools/shared/tests/**`, a + coincidental `src/`+`tests/` pair) nor one nested under a package's + `src/` (`src/foo/tests/bar.rs`, a unit-test submodule reached via + `#[cfg(test)] mod`). All four DRY file collectors — `FunctionCollector` + (duplicate hashing, DRY-001), `FragmentCollector` (repeated fragments, + DRY-004), `MatchPatternCollector` (repeated matches, DRY-005), and + `WildcardCollector` (wildcard imports) — no longer apply their own + `/tests/` path strings; they consult the shared `cfg_test_files` set + (integration dirs + `#![cfg(test)]` + `#[cfg(test)] mod` chains), so a + production directory merely *named* `tests` is never blanket-classified + as test-only, and `ignore_tests` now skips package integration-test and + `#![cfg(test)]` companion files for every DRY check. Addresses review + findings: neither `contains("/tests/")` nor a "non-`src` `/tests/`" + guard actually identified package roots. + *Known limitation:* a crate using a non-default `[lib] path = …` / + `[[bin]] path = …` (no file at a conventional `src/` crate-root) is not + detected as a package root from paths alone — that would require parsing + `Cargo.toml`. +- **Test Quality coverage checks (TQ-004/TQ-005) key off the + analyzer-computed `is_test` flag, not a `test_` name prefix.** + `tq::coverage` previously skipped any function *named* `test_*` via + an isolated heuristic that both missed framework-attributed tests and + wrongly exempted production functions named like tests (e.g. + `test_connection`). It now uses `FunctionAnalysis::is_test`, + consistent with every other dimension. +- **All structural detectors (BTC, SLM, NMS, IET, OI, SIT, DEH) skip + whole test files.** They previously skipped only inline `#[cfg(test)] + mod` blocks (BTC/IET/DEH and the SLM/NMS inherent-method walk) or + nothing at all — so a stub mock impl, selfless/needless-`&mut`-self + helper, inconsistent error types, orphaned impl, single-impl trait, or + `downcast` used in a package integration-test file or `#![cfg(test)]` + companion was falsely flagged (SRP/Coupling). The visitor-based + detectors now skip files in `cfg_test_files`; OI/SIT are fixed at the + source — `collect_metadata` no longer records types/traits/impls from + test files. Brings the Structural dimension in line with the rest of + the tool's test-aware behaviour. + +### Removed (breaking) + +- **`--diff [REF]` changed-files mode removed.** It parsed only the + git-changed files, so every cross-file analysis ran on a partial set + and produced false positives/negatives: dead-code reachability (a fn + used only from an unchanged file looked uncalled), cross-file DRY + duplicates (one side unchanged → missed), coupling instability (partial + module graph), architecture call-parity (missing adapters/targets), and + the cfg-test/package-root detection above. The flag's promise of "the + same analysis, only reporting changed files" was not what it did — it + analysed only changed files. Rather than ship a mode that is unsound for + a whole-program analyzer, it is removed along with `get_git_changed_files` + / `filter_to_changed`. Run rustqual over the full tree and use + `--format github` / `sarif` for PR-scoped annotations. + ## [1.2.6] - 2026-05-27 Patch release: **dyn-compatibility precision in `must_be_object_safe`** — diff --git a/Cargo.lock b/Cargo.lock index 14638c20..f04e281c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -578,7 +578,7 @@ dependencies = [ [[package]] name = "rustqual" -version = "1.2.6" +version = "1.3.0" dependencies = [ "clap", "clap_complete", diff --git a/Cargo.toml b/Cargo.toml index f7186ffe..82d58071 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "rustqual" -version = "1.2.6" +version = "1.3.0" edition = "2021" description = "Comprehensive Rust code quality analyzer — seven dimensions: IOSP, Complexity, DRY, SRP, Coupling, Test Quality, Architecture" license = "MIT" diff --git a/README.md b/README.md index 7cfb9750..75825814 100644 --- a/README.md +++ b/README.md @@ -145,7 +145,7 @@ With coverage and PR annotations: ```yaml - run: cargo install rustqual cargo-llvm-cov - run: cargo llvm-cov --lcov --output-path lcov.info -- run: rustqual --diff origin/main --coverage lcov.info --format github +- run: rustqual --coverage lcov.info --format github ``` For codebases that aren't yet at 100% but want to prevent regression: diff --git a/book/ai-coding-workflow.md b/book/ai-coding-workflow.md index f3276064..08bc15b9 100644 --- a/book/ai-coding-workflow.md +++ b/book/ai-coding-workflow.md @@ -63,10 +63,10 @@ jobs: - uses: dtolnay/rust-toolchain@stable - run: cargo install rustqual cargo-llvm-cov - run: cargo llvm-cov --lcov --output-path lcov.info - - run: rustqual --diff HEAD~1 --coverage lcov.info --format github + - run: rustqual --coverage lcov.info --format github ``` -`--format github` produces inline annotations on the PR diff — exactly where the issue is, what rule fired, why it matters. `--diff HEAD~1` restricts analysis to the changed files so PRs stay fast even on large codebases. +`--format github` produces inline annotations on the PR diff — exactly where the issue is, what rule fired, why it matters. ## Pattern 4: baseline tracking for AI-velocity codebases diff --git a/book/ci-integration.md b/book/ci-integration.md index 41aa30f1..aefc3757 100644 --- a/book/ci-integration.md +++ b/book/ci-integration.md @@ -28,16 +28,6 @@ That's all. If any dimension flags a finding, the job fails. `--format github` emits `::error::` and `::warning::` annotations that GitHub renders inline on the PR diff. `--min-quality-score 90` enforces a hard floor on the overall quality score. -## GitHub Actions — changed files only - -For large codebases, restrict analysis to files changed in the PR: - -```yaml -- run: rustqual --diff origin/main --format github -``` - -`--diff ` runs the same analysis but only reports findings in files that differ from ``. Cuts CI time for large codebases without losing PR-level signal. - ## GitHub Actions — with coverage The Test Quality dimension can use LCOV coverage data to detect untested logic: @@ -123,7 +113,6 @@ The flags compose. A typical "production-grade" CI step: ```yaml - run: | rustqual \ - --diff origin/main \ --coverage lcov.info \ --min-quality-score 90 \ --fail-on-warnings \ @@ -131,7 +120,7 @@ The flags compose. A typical "production-grade" CI step: ``` This: -- analyses only files changed vs `main`, +- analyses the full source tree, - includes coverage-based test-quality checks, - requires the overall quality score to be at least 90%, - treats suppression-ratio overruns as hard errors, diff --git a/book/code-reuse.md b/book/code-reuse.md index 736d8364..a44de3ae 100644 --- a/book/code-reuse.md +++ b/book/code-reuse.md @@ -72,7 +72,7 @@ pub fn assert_in_range(actual: f64, expected: f64, tol: f64) { /* … */ } `qual:api` and `qual:test_helper` exclude the function from `DRY-002` *and* from `TQ-003` (untested), without counting against `max_suppression_ratio`. Use them on functions that are exported to consumers outside the crate or used only by integration tests. -By default, the dead-code analysis treats workspace-root `tests/**` files as call-sites — so a function used only from integration tests is not dead. +By default, the dead-code analysis treats a package's `tests/**` files as call-sites — both the analysis-root crate's `tests/**` and each member's `crates/*/tests/**` — so a function used only from integration tests is not dead. ## Code fragments and repeated matches diff --git a/book/function-quality.md b/book/function-quality.md index ef8af76e..696e25d3 100644 --- a/book/function-quality.md +++ b/book/function-quality.md @@ -81,7 +81,7 @@ allow_expect = false # set true to permit .expect() but still flag .unwrap() ### Test-aware -`A20` and `CX-004` skip `#[test]` functions and files under workspace-root `tests/**`. Asserting on `.unwrap()` in a test is fine; it's panicking in production that matters. +`A20` and `CX-004` skip test functions (`#[test]`, `#[tokio::test]`, `#[rstest]`, `#[test_case]`, any `…::test`) and files in a package's `tests/**` directory — the analysis-root crate's `tests/**` and each member's `crates/*/tests/**` alike. Asserting on `.unwrap()` in a test is fine; it's panicking in production that matters. ## Method-shape checks diff --git a/book/getting-started.md b/book/getting-started.md index 3f47e7e3..c2821991 100644 --- a/book/getting-started.md +++ b/book/getting-started.md @@ -89,7 +89,6 @@ This writes a `rustqual.toml` next to `Cargo.toml`, with thresholds calibrated t | `--no-fail` | Local exploration; don't exit non-zero | | `--verbose` | Show every function, not just findings | | `--findings` | One finding per line: `file:line category in fn_name` | -| `--diff [REF]` | Only analyse files changed vs a git ref | | `--coverage ` | Include coverage-based test-quality checks | | `--init` | Generate a config tailored to your codebase | | `--watch` | Re-analyse on file changes | diff --git a/book/reference-cli.md b/book/reference-cli.md index 1462de62..f2cc3541 100644 --- a/book/reference-cli.md +++ b/book/reference-cli.md @@ -21,7 +21,6 @@ rustqual [OPTIONS] [PATH] | Flag | Description | |---|---| | `-c`, `--config ` | Path to config. Default: `rustqual.toml` in the target directory. | -| `--diff [REF]` | Only analyse files changed vs a git ref (default: `HEAD`). Conflicts with `--watch`. | | `--coverage ` | Path to LCOV coverage file. Enables TQ-004 / TQ-005. | | `--explain ` | Diagnostic mode: explain architecture-rule classification for one file. | | `--watch` | Watch for file changes and re-analyse continuously. | @@ -91,9 +90,6 @@ rustqual --no-fail --verbose # CI hard gate with coverage and PR annotations rustqual --coverage lcov.info --min-quality-score 90 --fail-on-warnings --format github -# PR-only analysis -rustqual --diff origin/main --format github - # Baseline-based regression gate rustqual --compare baseline.json --fail-on-regression --format github diff --git a/book/reference-output-formats.md b/book/reference-output-formats.md index 4c5f0c92..50e375f3 100644 --- a/book/reference-output-formats.md +++ b/book/reference-output-formats.md @@ -110,12 +110,6 @@ summary annotation (`::error::Quality analysis: N finding(s)…` or of the run. For per-rule filtering in CI, use `--format sarif` and upload to Code Scanning instead. -Combine with `--diff origin/main` for PR-only analysis: - -```yaml -- run: rustqual --diff origin/main --format github -``` - ## `sarif` SARIF v2.1.0. Designed for GitHub Code Scanning, but consumed by Azure DevOps, Sonatype, and any SARIF tool. diff --git a/book/reference-rules.md b/book/reference-rules.md index 63aa87f4..3b3b0bf1 100644 --- a/book/reference-rules.md +++ b/book/reference-rules.md @@ -40,7 +40,7 @@ For dimension intent and refactor patterns, see the use-case guides linked at th | `CX-006` | Unsafe block detected | `detect_unsafe = true` | | `A20` | Error-handling issue (`unwrap`/`expect`/`panic!`/`todo!`) | `detect_error_handling = true` | -`A20` and `CX-004` skip `#[test]` functions and workspace-root `tests/**` files. +`A20` and `CX-004` skip test functions (`#[test]`, `#[tokio::test]`, `#[rstest]`, `#[test_case]`, any `…::test`) and files in a package's `tests/**` directory (the analysis-root crate and each `crates/*/tests/**`). ## DRY diff --git a/src/adapters/analyzers/dry/fragments.rs b/src/adapters/analyzers/dry/fragments.rs index b3f993f8..6dd5457a 100644 --- a/src/adapters/analyzers/dry/fragments.rs +++ b/src/adapters/analyzers/dry/fragments.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use syn::spanned::Spanned; use syn::visit::Visit; @@ -75,8 +75,11 @@ fn collect_all_windows( parsed: &[(String, String, syn::File)], config: &DuplicatesConfig, ) -> (Vec, Vec) { + let cfg_test_files = + crate::adapters::shared::cfg_test_files::collect_cfg_test_file_paths(parsed); let mut collector = FragmentCollector { config, + cfg_test_files: &cfg_test_files, file: String::new(), fn_infos: Vec::new(), windows: Vec::new(), @@ -221,6 +224,7 @@ pub(crate) fn merge_into_fragments( /// AST visitor that collects statement windows from all function bodies. struct FragmentCollector<'a> { config: &'a DuplicatesConfig, + cfg_test_files: &'a HashSet, file: String, fn_infos: Vec, windows: Vec, @@ -232,7 +236,9 @@ struct FragmentCollector<'a> { impl super::FileVisitor for FragmentCollector<'_> { fn reset_for_file(&mut self, file_path: &str) { self.file = file_path.to_string(); - self.in_test = false; + // Whole-file test classification from the authoritative cfg-test + // set; inline `#[cfg(test)] mod` is still handled per-item below. + self.in_test = self.cfg_test_files.contains(file_path); self.parent_type = None; self.is_trait_impl = false; } diff --git a/src/adapters/analyzers/dry/functions.rs b/src/adapters/analyzers/dry/functions.rs index 676bf3f3..c56c306b 100644 --- a/src/adapters/analyzers/dry/functions.rs +++ b/src/adapters/analyzers/dry/functions.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use syn::spanned::Spanned; use syn::visit::Visit; @@ -11,6 +11,7 @@ use crate::config::sections::DuplicatesConfig; /// AST visitor that collects function bodies and computes their normalized hashes. pub(crate) struct FunctionCollector<'a> { pub(crate) config: &'a DuplicatesConfig, + pub(crate) cfg_test_files: &'a HashSet, pub(crate) file: String, pub(crate) entries: Vec, in_test: bool, @@ -19,9 +20,10 @@ pub(crate) struct FunctionCollector<'a> { } impl<'a> FunctionCollector<'a> { - pub(crate) fn new(config: &'a DuplicatesConfig) -> Self { + pub(crate) fn new(config: &'a DuplicatesConfig, cfg_test_files: &'a HashSet) -> Self { Self { config, + cfg_test_files, file: String::new(), entries: Vec::new(), in_test: false, @@ -34,10 +36,11 @@ impl<'a> FunctionCollector<'a> { impl FileVisitor for FunctionCollector<'_> { fn reset_for_file(&mut self, file_path: &str) { self.file = file_path.to_string(); - // Files under a `tests/` subdir are test companions loaded via a - // parent's `#[cfg(test)] mod tests;` — treat them as test code even - // though their AST has no visible #[cfg(test)] attribute. - self.in_test = file_path.contains("/tests/"); + // Whole-file test classification comes from the authoritative + // cfg-test file set (integration-test dirs + `#![cfg(test)]` + + // `#[cfg(test)] mod` chains), not a path heuristic. Inline + // `#[cfg(test)] mod` blocks are still handled per-item below. + self.in_test = self.cfg_test_files.contains(file_path); self.parent_type = None; self.is_trait_impl = false; } diff --git a/src/adapters/analyzers/dry/match_patterns.rs b/src/adapters/analyzers/dry/match_patterns.rs index 1c0828e8..af016e75 100644 --- a/src/adapters/analyzers/dry/match_patterns.rs +++ b/src/adapters/analyzers/dry/match_patterns.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use syn::visit::Visit; @@ -39,6 +39,7 @@ pub(crate) struct CollectedMatch { /// AST visitor that collects match expressions for repeated pattern detection. struct MatchPatternCollector<'a> { config: &'a crate::config::sections::DuplicatesConfig, + cfg_test_files: &'a HashSet, file: String, collected: Vec, in_test: bool, @@ -57,10 +58,13 @@ struct MatchPatternCollector<'a> { impl FileVisitor for MatchPatternCollector<'_> { fn reset_for_file(&mut self, file_path: &str) { + // Whole-file test classification from the authoritative cfg-test + // set (checked against the raw path, matching its keys). Inline + // `#[cfg(test)] mod` is still handled per-item below. + self.in_test = self.cfg_test_files.contains(file_path); // Normalise separators for deterministic findings across OSes, // matching the other DRY collectors (e.g. wildcards.rs). self.file = file_path.replace('\\', "/"); - self.in_test = false; self.current_fn = String::new(); self.current_impl_type = String::new(); self.module_stack.clear(); @@ -177,8 +181,11 @@ pub fn detect_repeated_matches( parsed: &[(String, String, syn::File)], config: &crate::config::sections::DuplicatesConfig, ) -> Vec { + let cfg_test_files = + crate::adapters::shared::cfg_test_files::collect_cfg_test_file_paths(parsed); let mut collector = MatchPatternCollector { config, + cfg_test_files: &cfg_test_files, file: String::new(), collected: Vec::new(), in_test: false, diff --git a/src/adapters/analyzers/dry/mod.rs b/src/adapters/analyzers/dry/mod.rs index f5d611bd..6609cdc3 100644 --- a/src/adapters/analyzers/dry/mod.rs +++ b/src/adapters/analyzers/dry/mod.rs @@ -74,7 +74,9 @@ pub(crate) fn collect_function_hashes( parsed: &[(String, String, syn::File)], config: &crate::config::sections::DuplicatesConfig, ) -> Vec { - let mut collector = functions::FunctionCollector::new(config); + let cfg_test_files = + crate::adapters::shared::cfg_test_files::collect_cfg_test_file_paths(parsed); + let mut collector = functions::FunctionCollector::new(config, &cfg_test_files); visit_all_files(parsed, &mut collector); collector.entries } diff --git a/src/adapters/analyzers/dry/tests/dead_code.rs b/src/adapters/analyzers/dry/tests/dead_code.rs index bddcdb0b..4bf1cc6d 100644 --- a/src/adapters/analyzers/dry/tests/dead_code.rs +++ b/src/adapters/analyzers/dry/tests/dead_code.rs @@ -491,6 +491,37 @@ fn test_private_use_does_not_count_as_reexport() { ); } +#[test] +fn integration_test_entry_point_in_crate_tests_dir_not_dead_code() { + // A `#[tokio::test]` entry point under a workspace crate's `tests/` + // directory is an integration-test root that cargo compiles and + // runs. It has no in-crate callers but must NOT be flagged as dead + // code — the same treatment already applied to `#[test]` fns. + let code = r#" + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn five_oh_two_twice_then_success_under_default_policy() { let x = 1; } + "#; + let parsed = vec![( + "crates/sv-utility-retry/tests/integration.rs".to_string(), + code.to_string(), + syn::parse_file(code).expect("parse failed"), + )]; + let config = Config::default(); + let cfg_test_files = + crate::adapters::shared::cfg_test_files::collect_cfg_test_file_paths(&parsed); + let warnings = detect_dead_code( + &parsed, + &config, + &std::collections::HashMap::new(), + &std::collections::HashMap::new(), + &cfg_test_files, + ); + assert!( + warnings.is_empty(), + "integration-test entry point must not be flagged as dead code: {warnings:?}" + ); +} + #[test] fn test_cfg_test_mod_file_not_flagged() { // Parent file declares `#[cfg(test)] mod helpers;` (external module) diff --git a/src/adapters/analyzers/dry/tests/fragments.rs b/src/adapters/analyzers/dry/tests/fragments.rs index fafc320a..75409190 100644 --- a/src/adapters/analyzers/dry/tests/fragments.rs +++ b/src/adapters/analyzers/dry/tests/fragments.rs @@ -31,6 +31,38 @@ fn low_threshold_config() -> DuplicatesConfig { } } +#[test] +fn fragments_in_cfg_test_companion_file_skipped() { + // DRY-004 must skip repeated statement windows in test code. A + // `#![cfg(test)]` companion file (no `tests/` path segment) is test + // code; the fragment collector recognises it via the authoritative + // cfg-test file set, not a path heuristic. + let code = r#" + #![cfg(test)] + fn helper_a() { + let x = 1; + let y = x + 2; + let z = y * x; + } + fn helper_b() { + let x = 1; + let y = x + 2; + let z = y * x; + } + "#; + let parsed = vec![( + "src/foo_tests.rs".to_string(), + code.to_string(), + syn::parse_file(code).expect("parse failed"), + )]; + let config = low_threshold_config(); // ignore_tests = true + let groups = detect_fragments(&parsed, &config); + assert!( + groups.is_empty(), + "fragments in a #![cfg(test)] file must be skipped: {groups:?}" + ); +} + #[test] fn test_detect_fragments_empty() { let parsed = parse(""); diff --git a/src/adapters/analyzers/dry/tests/functions.rs b/src/adapters/analyzers/dry/tests/functions.rs index 017bd9e8..f874e927 100644 --- a/src/adapters/analyzers/dry/tests/functions.rs +++ b/src/adapters/analyzers/dry/tests/functions.rs @@ -151,6 +151,55 @@ fn test_detect_duplicates_test_functions_included() { ); } +#[test] +fn duplicates_in_cfg_test_companion_file_skipped() { + // A `#![cfg(test)]` companion file is test code even though its path + // has no `tests/` segment. Duplicate detection must skip it by + // consulting the authoritative cfg-test file set, not a `tests/` + // path heuristic. + let code = r#" + #![cfg(test)] + fn helper_one() { let x = 1; let y = x + 2; let z = y * x; } + fn helper_two() { let a = 1; let b = a + 2; let c = b * a; } + "#; + let parsed = vec![( + "src/foo_tests.rs".to_string(), + code.to_string(), + syn::parse_file(code).expect("parse failed"), + )]; + let config = low_threshold_config(); // ignore_tests = true + let groups = detect_duplicates(&parsed, &config); + assert!( + groups.is_empty(), + "duplicates in a #![cfg(test)] file must be skipped: {groups:?}" + ); +} + +#[test] +fn duplicates_in_production_file_under_nested_tests_dir_still_flagged() { + // Inverse of the review fix: a `src/**/tests/` directory that is NOT + // reached via `#[cfg(test)] mod` is production code and its + // duplicates must still be flagged (the old broad path heuristic + // wrongly skipped anything containing `/tests/`). + let parsed = parse_multi(&[ + ( + "src/database/tests/conn_a.rs", + "pub fn conn_a() { let x = 1; let y = x + 2; let z = y * x; }", + ), + ( + "src/database/tests/conn_b.rs", + "pub fn conn_b() { let a = 1; let b = a + 2; let c = b * a; }", + ), + ]); + let config = low_threshold_config(); + let groups = detect_duplicates(&parsed, &config); + assert_eq!( + groups.len(), + 1, + "duplicates in a non-cfg-test src/**/tests/ file must be flagged: {groups:?}" + ); +} + #[test] fn test_detect_duplicates_three_way() { let parsed = parse_multi(&[ diff --git a/src/adapters/analyzers/dry/tests/match_patterns.rs b/src/adapters/analyzers/dry/tests/match_patterns.rs index 4134abfd..188424ff 100644 --- a/src/adapters/analyzers/dry/tests/match_patterns.rs +++ b/src/adapters/analyzers/dry/tests/match_patterns.rs @@ -18,6 +18,33 @@ fn test_detect_empty() { assert!(result.is_empty()); } +#[test] +fn repeated_matches_in_cfg_test_companion_file_skipped() { + // DRY-005 must skip repeated match patterns in test code. A + // `#![cfg(test)]` companion file (no `tests/` path segment) is test + // code; the collector recognises it via the authoritative cfg-test + // file set, not a path heuristic. + let code = r#" + #![cfg(test)] + enum E { A, B, C } + fn f1(e: E) -> i32 { match e { E::A => 1, E::B => 2, E::C => 3 } } + fn f2(e: E) -> i32 { match e { E::A => 1, E::B => 2, E::C => 3 } } + fn f3(e: E) -> i32 { match e { E::A => 1, E::B => 2, E::C => 3 } } + "#; + let parsed = vec![( + "src/foo_tests.rs".to_string(), + code.to_string(), + syn::parse_file(code).expect("parse failed"), + )]; + let config = DuplicatesConfig::default(); // ignore_tests = true + let result = detect_repeated_matches(&parsed, &config); + assert!( + result.is_empty(), + "repeated matches in a #![cfg(test)] file must be skipped: {} group(s)", + result.len() + ); +} + #[test] fn test_detect_single_match_not_flagged() { let code = r#" diff --git a/src/adapters/analyzers/dry/tests/wildcards.rs b/src/adapters/analyzers/dry/tests/wildcards.rs index 93c19b8f..05c941b5 100644 --- a/src/adapters/analyzers/dry/tests/wildcards.rs +++ b/src/adapters/analyzers/dry/tests/wildcards.rs @@ -95,6 +95,46 @@ fn test_external_glob_detected() { assert_eq!(warnings[0].module_path, "std::collections::*"); } +#[test] +fn wildcard_in_cfg_test_companion_file_skipped() { + // Wildcard imports in a test file are a common, accepted pattern. + // A `#![cfg(test)]` companion file (no `tests/` path segment) is test + // code, so its glob imports must be skipped — recognised via the + // authoritative cfg-test file set, not a `tests/` path heuristic. + let code = r#" + #![cfg(test)] + use crate::foo::*; + "#; + let parsed = vec![( + "src/bar_tests.rs".to_string(), + code.to_string(), + syn::parse_file(code).expect("parse failed"), + )]; + let warnings = detect_wildcard_imports(&parsed); + assert!( + warnings.is_empty(), + "wildcard import in a #![cfg(test)] file must be skipped: {warnings:?}" + ); +} + +#[test] +fn wildcard_in_production_file_under_nested_tests_dir_still_flagged() { + // A `src/**/tests/` file not reached via `#[cfg(test)] mod` is + // production code; its wildcard imports must still be flagged. + let code = "use crate::foo::*;"; + let parsed = vec![( + "src/database/tests/util.rs".to_string(), + code.to_string(), + syn::parse_file(code).expect("parse failed"), + )]; + let warnings = detect_wildcard_imports(&parsed); + assert_eq!( + warnings.len(), + 1, + "wildcard in a non-cfg-test src/**/tests/ file must be flagged: {warnings:?}" + ); +} + #[test] fn test_multiple_globs_in_file() { let code = "use crate::a::*;\nuse crate::b::*;\nuse crate::c::Foo;"; diff --git a/src/adapters/analyzers/dry/wildcards.rs b/src/adapters/analyzers/dry/wildcards.rs index 1dfb0fcb..809bc587 100644 --- a/src/adapters/analyzers/dry/wildcards.rs +++ b/src/adapters/analyzers/dry/wildcards.rs @@ -1,3 +1,5 @@ +use std::collections::HashSet; + use syn::spanned::Spanned; use syn::visit::Visit; @@ -21,25 +23,34 @@ pub struct WildcardImportWarning { pub fn detect_wildcard_imports( parsed: &[(String, String, syn::File)], ) -> Vec { + let cfg_test_files = + crate::adapters::shared::cfg_test_files::collect_cfg_test_file_paths(parsed); let mut collector = WildcardCollector { + cfg_test_files, file: String::new(), warnings: Vec::new(), in_test: false, + file_is_test: false, }; super::visit_all_files(parsed, &mut collector); collector.warnings } struct WildcardCollector { + cfg_test_files: HashSet, file: String, warnings: Vec, in_test: bool, + file_is_test: bool, } impl FileVisitor for WildcardCollector { fn reset_for_file(&mut self, file_path: &str) { - // Normalise separators once so downstream checks (e.g. "/tests/" - // companion-file detection) work on Windows paths too. + // Whole-file test classification comes from the authoritative + // cfg-test file set (checked against the raw path, matching the + // set's keys). `self.file` is separately separator-normalised so + // the emitted warning path is stable on Windows. + self.file_is_test = self.cfg_test_files.contains(file_path); self.file = file_path.replace('\\', "/"); self.in_test = false; } @@ -68,11 +79,11 @@ impl<'ast> Visit<'ast> for WildcardCollector { if self.in_test && prefix.as_slice() == ["super"] { continue; } - // Skip wildcard imports in files under any `tests/` - // directory: companion test subtrees inside `src/**/tests/` - // AND workspace-root `tests/**` integration-test binaries. - // `reset_for_file` already normalised `\` → `/`. - if self.file.starts_with("tests/") || self.file.contains("/tests/") { + // Skip wildcard imports in test files (integration-test + // binaries and `#[cfg(test)]`/companion files alike). + // Classification comes from the authoritative cfg-test + // file set, computed in `detect_wildcard_imports`. + if self.file_is_test { continue; } // Skip any prelude wildcard: matches the bare diff --git a/src/adapters/analyzers/structural/btc.rs b/src/adapters/analyzers/structural/btc.rs index 195085d5..7730b6d6 100644 --- a/src/adapters/analyzers/structural/btc.rs +++ b/src/adapters/analyzers/structural/btc.rs @@ -1,19 +1,27 @@ +use std::collections::HashSet; + use crate::config::StructuralConfig; use crate::findings::Dimension; use super::{StructuralWarning, StructuralWarningKind}; /// Detect broken trait contracts: impl Trait where methods are only stubs (TQ-like binary check). +/// Whole test files (in `cfg_test_files`) are skipped; inline +/// `#[cfg(test)] mod` blocks are already skipped during recursion. /// Operation: iterates parsed files, inspects impl blocks, no own calls. pub(crate) fn detect_btc( warnings: &mut Vec, parsed: &[(String, String, syn::File)], config: &StructuralConfig, + cfg_test_files: &HashSet, ) { if !config.check_btc { return; } parsed.iter().for_each(|(path, _, syntax)| { + if cfg_test_files.contains(path) { + return; + } syntax.items.iter().for_each(|item| { check_item(item, path, warnings); }); diff --git a/src/adapters/analyzers/structural/deh.rs b/src/adapters/analyzers/structural/deh.rs index 94d940a1..f5b453d2 100644 --- a/src/adapters/analyzers/structural/deh.rs +++ b/src/adapters/analyzers/structural/deh.rs @@ -1,3 +1,5 @@ +use std::collections::HashSet; + use syn::visit::Visit; use crate::config::StructuralConfig; @@ -9,11 +11,15 @@ use super::{has_cfg_test_attr, StructuralWarning, StructuralWarningKind}; const DOWNCAST_METHODS: &[&str] = &["downcast_ref", "downcast_mut", "downcast"]; /// Detect downcast escape hatches: use of Any::downcast_*. +/// Test code is skipped: a file in `cfg_test_files` (integration-test dir, +/// `#![cfg(test)]`, or `#[cfg(test)] mod` chain) starts `in_test = true`; +/// inline `#[cfg(test)] mod` blocks are handled per-item. /// Operation: iterates parsed files, walks expressions for downcast calls. pub(crate) fn detect_deh( warnings: &mut Vec, parsed: &[(String, String, syn::File)], config: &StructuralConfig, + cfg_test_files: &HashSet, ) { if !config.check_deh { return; @@ -22,7 +28,7 @@ pub(crate) fn detect_deh( let mut visitor = DowncastVisitor { file: path.clone(), warnings, - in_test: false, + in_test: cfg_test_files.contains(path), }; visitor.visit_file(syntax); }); diff --git a/src/adapters/analyzers/structural/iet.rs b/src/adapters/analyzers/structural/iet.rs index 91093bc6..897da9ff 100644 --- a/src/adapters/analyzers/structural/iet.rs +++ b/src/adapters/analyzers/structural/iet.rs @@ -6,16 +6,22 @@ use crate::findings::Dimension; use super::{StructuralWarning, StructuralWarningKind}; /// Detect inconsistent error types: pub fns in same module return different Result<_, E>. +/// Whole test files (in `cfg_test_files`) are skipped; inline +/// `#[cfg(test)] mod` blocks are already skipped during recursion. /// Operation: collects return types per file, flags inconsistencies. pub(crate) fn detect_iet( warnings: &mut Vec, parsed: &[(String, String, syn::File)], config: &StructuralConfig, + cfg_test_files: &HashSet, ) { if !config.check_iet { return; } parsed.iter().for_each(|(path, _, syntax)| { + if cfg_test_files.contains(path) { + return; + } let error_types = collect_pub_error_types(syntax); if error_types.len() >= 2 { let mut types: Vec = error_types.into_iter().collect(); diff --git a/src/adapters/analyzers/structural/mod.rs b/src/adapters/analyzers/structural/mod.rs index 7aa36023..53805e3f 100644 --- a/src/adapters/analyzers/structural/mod.rs +++ b/src/adapters/analyzers/structural/mod.rs @@ -7,7 +7,7 @@ pub(crate) mod oi; pub(crate) mod sit; pub(crate) mod slm; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use syn::spanned::Spanned; use crate::config::StructuralConfig; @@ -103,9 +103,15 @@ pub(crate) struct TraitInfo { pub method_count: usize, } -/// Collect structural metadata from all parsed files. +/// Collect structural metadata from all parsed files. Whole test files +/// (in `cfg_test_files`) are skipped so OI/SIT — which key off this +/// metadata — never flag types/traits/impls that live in test code; +/// inline `#[cfg(test)] mod` blocks are already skipped per-item. /// Operation: iterates files and visits AST nodes, no own calls. -pub(crate) fn collect_metadata(parsed: &[(String, String, syn::File)]) -> StructuralMetadata { +pub(crate) fn collect_metadata( + parsed: &[(String, String, syn::File)], + cfg_test_files: &HashSet, +) -> StructuralMetadata { let mut meta = StructuralMetadata { enum_defs: HashMap::new(), type_defs: HashMap::new(), @@ -114,6 +120,9 @@ pub(crate) fn collect_metadata(parsed: &[(String, String, syn::File)]) -> Struct inherent_impls: Vec::new(), }; parsed.iter().for_each(|(path, _, syntax)| { + if cfg_test_files.contains(path) { + return; + } syntax.items.iter().for_each(|item| { collect_item_metadata(item, path, &mut meta); }); @@ -246,16 +255,18 @@ pub(crate) fn analyze_structural( parsed: &[(String, String, syn::File)], config: &StructuralConfig, ) -> StructuralAnalysis { - let meta = collect_metadata(parsed); + let cfg_test_files = + crate::adapters::shared::cfg_test_files::collect_cfg_test_file_paths(parsed); + let meta = collect_metadata(parsed, &cfg_test_files); let mut warnings = Vec::new(); - btc::detect_btc(&mut warnings, parsed, config); - slm::detect_slm(&mut warnings, parsed, config); - nms::detect_nms(&mut warnings, parsed, config); - deh::detect_deh(&mut warnings, parsed, config); + btc::detect_btc(&mut warnings, parsed, config, &cfg_test_files); + slm::detect_slm(&mut warnings, parsed, config, &cfg_test_files); + nms::detect_nms(&mut warnings, parsed, config, &cfg_test_files); + deh::detect_deh(&mut warnings, parsed, config, &cfg_test_files); oi::detect_oi(&mut warnings, &meta, config); sit::detect_sit(&mut warnings, &meta, config); - iet::detect_iet(&mut warnings, parsed, config); + iet::detect_iet(&mut warnings, parsed, config, &cfg_test_files); StructuralAnalysis { warnings } } diff --git a/src/adapters/analyzers/structural/nms.rs b/src/adapters/analyzers/structural/nms.rs index 9db8a2bd..6e7acb07 100644 --- a/src/adapters/analyzers/structural/nms.rs +++ b/src/adapters/analyzers/structural/nms.rs @@ -1,3 +1,5 @@ +use std::collections::HashSet; + use syn::visit::Visit; use crate::config::StructuralConfig; @@ -6,16 +8,22 @@ use crate::findings::Dimension; use super::{StructuralWarning, StructuralWarningKind}; /// Detect needless &mut self: method takes &mut self but never writes to self. +/// Whole test files (in `cfg_test_files`) are skipped; inline +/// `#[cfg(test)] mod` blocks are already skipped by `visit_inherent_methods`. /// Operation: iterates parsed files via shared visitor, no own calls. pub(crate) fn detect_nms( warnings: &mut Vec, parsed: &[(String, String, syn::File)], config: &StructuralConfig, + cfg_test_files: &HashSet, ) { if !config.check_nms { return; } super::visit_inherent_methods(parsed, |method, path| { + if cfg_test_files.contains(path) { + return; + } check_method(method, path, warnings); }); } diff --git a/src/adapters/analyzers/structural/slm.rs b/src/adapters/analyzers/structural/slm.rs index 2bab5788..15d93f31 100644 --- a/src/adapters/analyzers/structural/slm.rs +++ b/src/adapters/analyzers/structural/slm.rs @@ -1,3 +1,5 @@ +use std::collections::HashSet; + use syn::visit::Visit; use crate::config::StructuralConfig; @@ -6,16 +8,22 @@ use crate::findings::Dimension; use super::{StructuralWarning, StructuralWarningKind}; /// Detect self-less methods: &self/&mut self param but self never referenced in body. +/// Whole test files (in `cfg_test_files`) are skipped; inline +/// `#[cfg(test)] mod` blocks are already skipped by `visit_inherent_methods`. /// Operation: iterates parsed files via shared visitor, no own calls. pub(crate) fn detect_slm( warnings: &mut Vec, parsed: &[(String, String, syn::File)], config: &StructuralConfig, + cfg_test_files: &HashSet, ) { if !config.check_slm { return; } super::visit_inherent_methods(parsed, |method, path| { + if cfg_test_files.contains(path) { + return; + } check_method(method, path, warnings); }); } diff --git a/src/adapters/analyzers/structural/tests/btc.rs b/src/adapters/analyzers/structural/tests/btc.rs index c40382c0..f2fd6f7f 100644 --- a/src/adapters/analyzers/structural/tests/btc.rs +++ b/src/adapters/analyzers/structural/tests/btc.rs @@ -5,11 +5,28 @@ use crate::config::StructuralConfig; fn detect_in(source: &str) -> Vec { let parsed = super::parse_single(source); let config = StructuralConfig::default(); + let cfg_test_files = + crate::adapters::shared::cfg_test_files::collect_cfg_test_file_paths(&parsed); let mut warnings = Vec::new(); - detect_btc(&mut warnings, &parsed, &config); + detect_btc(&mut warnings, &parsed, &config, &cfg_test_files); warnings } +#[test] +fn stub_impl_in_cfg_test_file_excluded() { + // BTC must skip whole test files (here a `#![cfg(test)]` companion), + // not only inline `#[cfg(test)] mod` — mock impls with `todo!()` + // stubs are legitimate in tests. + let w = detect_in( + "#![cfg(test)]\ntrait Foo { fn bar(&self); } impl Foo for MyType { fn bar(&self) { todo!() } } struct MyType;", + ); + assert!( + w.is_empty(), + "stub impl in a #![cfg(test)] file must be excluded: {} warning(s)", + w.len() + ); +} + #[test] fn test_all_stub_methods_flagged() { let w = detect_in("trait Foo { fn bar(&self); } impl Foo for MyType { fn bar(&self) { todo!() } } struct MyType;"); @@ -69,6 +86,11 @@ fn test_disabled_check() { ..StructuralConfig::default() }; let mut warnings = Vec::new(); - detect_btc(&mut warnings, &parsed, &config); + detect_btc( + &mut warnings, + &parsed, + &config, + &std::collections::HashSet::new(), + ); assert!(warnings.is_empty()); } diff --git a/src/adapters/analyzers/structural/tests/deh.rs b/src/adapters/analyzers/structural/tests/deh.rs index 659595a2..6c50b373 100644 --- a/src/adapters/analyzers/structural/tests/deh.rs +++ b/src/adapters/analyzers/structural/tests/deh.rs @@ -5,8 +5,10 @@ use crate::config::StructuralConfig; fn detect_in(source: &str) -> Vec { let parsed = super::parse_single(source); let config = StructuralConfig::default(); + let cfg_test_files = + crate::adapters::shared::cfg_test_files::collect_cfg_test_file_paths(&parsed); let mut warnings = Vec::new(); - detect_deh(&mut warnings, &parsed, &config); + detect_deh(&mut warnings, &parsed, &config, &cfg_test_files); warnings } @@ -40,6 +42,21 @@ fn test_test_code_excluded() { assert!(w.is_empty()); } +#[test] +fn downcast_in_cfg_test_companion_file_excluded() { + // DEH (Coupling) must skip test code identified by the authoritative + // cfg-test set, not only inline `#[cfg(test)] mod`. A `#![cfg(test)]` + // whole-file companion (and package integration-test files) count as + // test code even without an inline cfg(test) module or `#[test]` fn. + let w = + detect_in("#![cfg(test)]\nfn helper(a: &dyn std::any::Any) { a.downcast_ref::(); }"); + assert!( + w.is_empty(), + "downcast in a #![cfg(test)] file must be excluded: {} warning(s)", + w.len() + ); +} + #[test] fn test_disabled_check() { let syntax = syn::parse_file("fn foo(a: &dyn std::any::Any) { a.downcast_ref::(); }") @@ -49,7 +66,8 @@ fn test_disabled_check() { check_deh: false, ..StructuralConfig::default() }; + let cfg_test_files = std::collections::HashSet::new(); let mut warnings = Vec::new(); - detect_deh(&mut warnings, &parsed, &config); + detect_deh(&mut warnings, &parsed, &config, &cfg_test_files); assert!(warnings.is_empty()); } diff --git a/src/adapters/analyzers/structural/tests/iet.rs b/src/adapters/analyzers/structural/tests/iet.rs index f7cc969d..f3e2affd 100644 --- a/src/adapters/analyzers/structural/tests/iet.rs +++ b/src/adapters/analyzers/structural/tests/iet.rs @@ -5,11 +5,27 @@ use crate::config::StructuralConfig; fn detect_in(source: &str) -> Vec { let parsed = super::parse_single(source); let config = StructuralConfig::default(); + let cfg_test_files = + crate::adapters::shared::cfg_test_files::collect_cfg_test_file_paths(&parsed); let mut warnings = Vec::new(); - detect_iet(&mut warnings, &parsed, &config); + detect_iet(&mut warnings, &parsed, &config, &cfg_test_files); warnings } +#[test] +fn inconsistent_error_types_in_cfg_test_file_excluded() { + // IET must skip whole test files (here a `#![cfg(test)]` companion), + // not only inline `#[cfg(test)] mod`. + let w = detect_in( + "#![cfg(test)]\npub fn a() -> Result<(), String> { Ok(()) } pub fn b() -> Result { todo!() }", + ); + assert!( + w.is_empty(), + "inconsistent error types in a #![cfg(test)] file must be excluded: {} warning(s)", + w.len() + ); +} + #[test] fn test_consistent_error_types_not_flagged() { let w = detect_in( @@ -65,7 +81,12 @@ fn test_disabled_check() { ..StructuralConfig::default() }; let mut warnings = Vec::new(); - detect_iet(&mut warnings, &parsed, &config); + detect_iet( + &mut warnings, + &parsed, + &config, + &std::collections::HashSet::new(), + ); assert!(warnings.is_empty()); } diff --git a/src/adapters/analyzers/structural/tests/nms.rs b/src/adapters/analyzers/structural/tests/nms.rs index 4b371d71..4c13b943 100644 --- a/src/adapters/analyzers/structural/tests/nms.rs +++ b/src/adapters/analyzers/structural/tests/nms.rs @@ -5,11 +5,27 @@ use crate::config::StructuralConfig; fn detect_in(source: &str) -> Vec { let parsed = super::parse_single(source); let config = StructuralConfig::default(); + let cfg_test_files = + crate::adapters::shared::cfg_test_files::collect_cfg_test_file_paths(&parsed); let mut warnings = Vec::new(); - detect_nms(&mut warnings, &parsed, &config); + detect_nms(&mut warnings, &parsed, &config, &cfg_test_files); warnings } +#[test] +fn needless_mut_self_in_cfg_test_file_excluded() { + // NMS must skip whole test files (here a `#![cfg(test)]` companion), + // not only inline `#[cfg(test)] mod`. + let w = detect_in( + "#![cfg(test)]\nstruct S { x: i32 } impl S { fn foo(&mut self) -> i32 { self.x } }", + ); + assert!( + w.is_empty(), + "needless &mut self in a #![cfg(test)] file must be excluded: {} warning(s)", + w.len() + ); +} + #[test] fn test_needless_mut_self_flagged() { let w = detect_in("struct S { x: i32 } impl S { fn foo(&mut self) -> i32 { self.x } }"); diff --git a/src/adapters/analyzers/structural/tests/oi.rs b/src/adapters/analyzers/structural/tests/oi.rs index e33371ea..03e16406 100644 --- a/src/adapters/analyzers/structural/tests/oi.rs +++ b/src/adapters/analyzers/structural/tests/oi.rs @@ -5,13 +5,35 @@ use crate::config::StructuralConfig; fn detect_multi(sources: &[(&str, &str)]) -> Vec { let parsed = super::parse_multi(sources); - let meta = collect_metadata(&parsed); + let cfg_test_files = + crate::adapters::shared::cfg_test_files::collect_cfg_test_file_paths(&parsed); + let meta = collect_metadata(&parsed, &cfg_test_files); let config = StructuralConfig::default(); let mut warnings = Vec::new(); detect_oi(&mut warnings, &meta, &config); warnings } +#[test] +fn orphaned_impl_across_cfg_test_files_excluded() { + // OI must skip test code: a type + inherent impl split across + // `#![cfg(test)]` companion files are test fixtures, not a production + // orphaned impl. (Fixed at the metadata source: test files are not + // collected into StructuralMetadata.) + let w = detect_multi(&[ + ("src/a_tests.rs", "#![cfg(test)]\nstruct W;"), + ( + "src/b_tests.rs", + "#![cfg(test)]\nimpl W { fn go(&self) {} }", + ), + ]); + assert!( + w.is_empty(), + "orphaned impl across #![cfg(test)] files must be excluded: {} warning(s)", + w.len() + ); +} + #[test] fn test_same_file_not_flagged() { let w = detect_multi(&[("lib.rs", "struct Foo {} impl Foo { fn bar() {} }")]); @@ -92,7 +114,7 @@ fn test_disabled_check() { syn::parse_file("impl Foo { fn bar() {} }").expect("test"), ), ]; - let meta = collect_metadata(&parsed); + let meta = collect_metadata(&parsed, &std::collections::HashSet::new()); let config = StructuralConfig { check_oi: false, ..StructuralConfig::default() diff --git a/src/adapters/analyzers/structural/tests/root.rs b/src/adapters/analyzers/structural/tests/root.rs index bdc70336..55c1c352 100644 --- a/src/adapters/analyzers/structural/tests/root.rs +++ b/src/adapters/analyzers/structural/tests/root.rs @@ -9,7 +9,7 @@ fn test_structural_analysis_default_empty() { #[test] fn test_collect_metadata_empty() { let parsed: Vec<(String, String, syn::File)> = vec![]; - let meta = collect_metadata(&parsed); + let meta = collect_metadata(&parsed, &std::collections::HashSet::new()); assert!(meta.enum_defs.is_empty()); assert!(meta.type_defs.is_empty()); assert!(meta.trait_defs.is_empty()); @@ -20,7 +20,7 @@ fn test_collect_metadata_enum() { let source = "pub enum Color { Red, Green, Blue }"; let syntax = syn::parse_file(source).expect("test source"); let parsed = vec![("lib.rs".to_string(), source.to_string(), syntax)]; - let meta = collect_metadata(&parsed); + let meta = collect_metadata(&parsed, &std::collections::HashSet::new()); assert!(meta.enum_defs.contains_key("Color")); let (file, variants) = &meta.enum_defs["Color"]; assert_eq!(file, "lib.rs"); @@ -32,7 +32,7 @@ fn test_collect_metadata_struct_and_impl() { let source = "struct Foo {} impl Foo { fn bar() {} }"; let syntax = syn::parse_file(source).expect("test source"); let parsed = vec![("lib.rs".to_string(), source.to_string(), syntax)]; - let meta = collect_metadata(&parsed); + let meta = collect_metadata(&parsed, &std::collections::HashSet::new()); assert_eq!(meta.type_defs.get("Foo"), Some(&"lib.rs".to_string())); assert_eq!(meta.inherent_impls.len(), 1); assert_eq!(meta.inherent_impls[0].0, "Foo"); @@ -43,7 +43,7 @@ fn test_collect_metadata_trait_and_impl() { let source = "trait Drawable { fn draw(&self); } struct Circle; impl Drawable for Circle { fn draw(&self) {} }"; let syntax = syn::parse_file(source).expect("test source"); let parsed = vec![("lib.rs".to_string(), source.to_string(), syntax)]; - let meta = collect_metadata(&parsed); + let meta = collect_metadata(&parsed, &std::collections::HashSet::new()); assert!(meta.trait_defs.contains_key("Drawable")); assert!(!meta.trait_defs["Drawable"].is_pub); assert_eq!(meta.trait_defs["Drawable"].method_count, 1); @@ -55,6 +55,6 @@ fn test_cfg_test_module_excluded() { let source = "#[cfg(test)] mod tests { struct TestOnly; }"; let syntax = syn::parse_file(source).expect("test source"); let parsed = vec![("lib.rs".to_string(), source.to_string(), syntax)]; - let meta = collect_metadata(&parsed); + let meta = collect_metadata(&parsed, &std::collections::HashSet::new()); assert!(!meta.type_defs.contains_key("TestOnly")); } diff --git a/src/adapters/analyzers/structural/tests/sit.rs b/src/adapters/analyzers/structural/tests/sit.rs index 5fe2a05c..71c7c925 100644 --- a/src/adapters/analyzers/structural/tests/sit.rs +++ b/src/adapters/analyzers/structural/tests/sit.rs @@ -5,13 +5,29 @@ use crate::config::StructuralConfig; fn detect_from(source: &str) -> Vec { let parsed = super::parse_single(source); - let meta = collect_metadata(&parsed); + let cfg_test_files = + crate::adapters::shared::cfg_test_files::collect_cfg_test_file_paths(&parsed); + let meta = collect_metadata(&parsed, &cfg_test_files); let config = StructuralConfig::default(); let mut warnings = Vec::new(); detect_sit(&mut warnings, &meta, &config); warnings } +#[test] +fn single_impl_trait_in_cfg_test_file_excluded() { + // SIT must skip test code: a non-pub trait with one impl inside a + // `#![cfg(test)]` file is a test mock, not a production over-abstraction. + let w = detect_from( + "#![cfg(test)]\ntrait Drawable { fn draw(&self); } struct Circle; impl Drawable for Circle { fn draw(&self) {} }", + ); + assert!( + w.is_empty(), + "single-impl trait in a #![cfg(test)] file must be excluded: {} warning(s)", + w.len() + ); +} + #[test] fn test_single_impl_flagged() { let w = detect_from( @@ -59,7 +75,7 @@ fn test_disabled_check() { syn::parse_file("trait D { fn d(&self); } struct C; impl D for C { fn d(&self) {} }") .expect("test source"); let parsed = vec![("lib.rs".to_string(), String::new(), syntax)]; - let meta = collect_metadata(&parsed); + let meta = collect_metadata(&parsed, &std::collections::HashSet::new()); let config = StructuralConfig { check_sit: false, ..StructuralConfig::default() diff --git a/src/adapters/analyzers/structural/tests/slm.rs b/src/adapters/analyzers/structural/tests/slm.rs index f3dca6ba..15e20b23 100644 --- a/src/adapters/analyzers/structural/tests/slm.rs +++ b/src/adapters/analyzers/structural/tests/slm.rs @@ -5,11 +5,25 @@ use crate::config::StructuralConfig; fn detect_in(source: &str) -> Vec { let parsed = super::parse_single(source); let config = StructuralConfig::default(); + let cfg_test_files = + crate::adapters::shared::cfg_test_files::collect_cfg_test_file_paths(&parsed); let mut warnings = Vec::new(); - detect_slm(&mut warnings, &parsed, &config); + detect_slm(&mut warnings, &parsed, &config, &cfg_test_files); warnings } +#[test] +fn selfless_method_in_cfg_test_file_excluded() { + // SLM must skip whole test files (here a `#![cfg(test)]` companion), + // not only inline `#[cfg(test)] mod`. + let w = detect_in("#![cfg(test)]\nstruct S; impl S { fn foo(&self) -> i32 { 42 } }"); + assert!( + w.is_empty(), + "selfless method in a #![cfg(test)] file must be excluded: {} warning(s)", + w.len() + ); +} + #[test] fn test_selfless_method_flagged() { let w = detect_in("struct S; impl S { fn foo(&self) -> i32 { 42 } }"); diff --git a/src/adapters/analyzers/tq/coverage.rs b/src/adapters/analyzers/tq/coverage.rs index cca3f477..28155e4c 100644 --- a/src/adapters/analyzers/tq/coverage.rs +++ b/src/adapters/analyzers/tq/coverage.rs @@ -13,7 +13,7 @@ pub(crate) fn detect_uncovered_functions( ) -> Vec { all_results .iter() - .filter(|fa| !fa.suppressed && !is_test_function(&fa.name)) + .filter(|fa| !fa.suppressed && !fa.is_test) .filter_map(|fa| { let file_data = lcov_data.get(&fa.file)?; let hit_count = file_data.function_hits.get(&fa.name)?; @@ -40,7 +40,7 @@ pub(crate) fn detect_untested_logic( ) -> Vec { all_results .iter() - .filter(|fa| !fa.suppressed && !is_test_function(&fa.name)) + .filter(|fa| !fa.suppressed && !fa.is_test) .filter_map(|fa| { let file_data = lcov_data.get(&fa.file)?; let complexity = fa.complexity.as_ref()?; @@ -75,9 +75,3 @@ pub(crate) fn detect_untested_logic( }) .collect() } - -/// Check if a function name looks like a test function. -/// Operation: string prefix check. -fn is_test_function(name: &str) -> bool { - name.starts_with("test_") -} diff --git a/src/adapters/analyzers/tq/tests/coverage.rs b/src/adapters/analyzers/tq/tests/coverage.rs index 49b6a788..657fff72 100644 --- a/src/adapters/analyzers/tq/tests/coverage.rs +++ b/src/adapters/analyzers/tq/tests/coverage.rs @@ -76,17 +76,38 @@ fn test_function_not_in_lcov_no_warning() { } #[test] -fn test_test_function_excluded() { - let results = vec![make_func("test_something", "src/lib.rs", 10)]; +fn test_function_excluded_by_is_test_flag() { + // Test entry points are excluded via the analyzer-computed `is_test` + // flag (attribute/cfg/path aware), not by a `test_` name prefix — + // so a `#[tokio::test]`-style fn with any name is excluded. + let mut func = make_func("five_oh_two_twice", "src/lib.rs", 10); + func.is_test = true; + let results = vec![func]; let mut lcov = HashMap::new(); lcov.insert( "src/lib.rs".to_string(), - make_lcov_data(&[("test_something", 0)], &[]), + make_lcov_data(&[("five_oh_two_twice", 0)], &[]), ); let warnings = detect_uncovered_functions(&results, &lcov); assert!(warnings.is_empty()); } +#[test] +fn production_function_named_like_test_is_still_checked() { + // A production function merely *named* `test_*` (no test attribute, + // is_test = false) is real production code and must still be + // coverage-checked — the old name-prefix heuristic wrongly skipped it. + let results = vec![make_func("test_connection", "src/lib.rs", 10)]; + let mut lcov = HashMap::new(); + lcov.insert( + "src/lib.rs".to_string(), + make_lcov_data(&[("test_connection", 0)], &[]), + ); + let warnings = detect_uncovered_functions(&results, &lcov); + assert_eq!(warnings.len(), 1); + assert_eq!(warnings[0].kind, TqWarningKind::Uncovered); +} + #[test] fn test_suppressed_function_excluded() { let mut func = make_func("process", "src/lib.rs", 10); @@ -103,6 +124,29 @@ fn test_suppressed_function_excluded() { // ── TQ-005 tests ──────────────────────────────────────── +#[test] +fn untested_logic_in_test_function_excluded() { + // TQ-005 must also key off the `is_test` flag: uncovered logic in a + // test function is not a coverage gap to report. + let mut func = make_func("verifies_retry", "src/lib.rs", 10); + func.is_test = true; + func.complexity = Some(ComplexityMetrics { + logic_occurrences: vec![LogicOccurrence { + kind: "if".to_string(), + line: 15, + }], + ..Default::default() + }); + let results = vec![func]; + let mut lcov = HashMap::new(); + lcov.insert("src/lib.rs".to_string(), make_lcov_data(&[], &[(15, 0)])); + let warnings = detect_untested_logic(&results, &lcov); + assert!( + warnings.is_empty(), + "uncovered logic in a test function must not be flagged: {warnings:?}" + ); +} + #[test] fn test_untested_logic_detected() { let mut func = make_func("process", "src/lib.rs", 10); diff --git a/src/adapters/shared/cfg_test.rs b/src/adapters/shared/cfg_test.rs index b0aac138..ba20060e 100644 --- a/src/adapters/shared/cfg_test.rs +++ b/src/adapters/shared/cfg_test.rs @@ -16,8 +16,19 @@ pub fn has_cfg_test(attrs: &[syn::Attribute]) -> bool { }) } -/// True if `attrs` contains `#[test]`. +/// True if `attrs` contains a test-entry-point attribute. +/// +/// Recognises the bare `#[test]` plus the common framework variants: +/// any attribute whose path ends in `test` (`#[tokio::test]`, +/// `#[async_std::test]`, `#[googletest::test]`, …) and the renamed +/// macros `#[rstest]` and `#[test_case]`. Matching on the last path +/// segment keeps both bare and fully-qualified forms (`#[rstest::rstest]`) +/// in scope without an exhaustive crate list. /// Operation: attribute inspection logic, no own calls. pub fn has_test_attr(attrs: &[syn::Attribute]) -> bool { - attrs.iter().any(|attr| attr.path().is_ident("test")) + attrs.iter().any(|attr| { + attr.path().segments.last().is_some_and(|seg| { + seg.ident == "test" || seg.ident == "rstest" || seg.ident == "test_case" + }) + }) } diff --git a/src/adapters/shared/cfg_test_files.rs b/src/adapters/shared/cfg_test_files.rs index 7f2031ce..2fad50e2 100644 --- a/src/adapters/shared/cfg_test_files.rs +++ b/src/adapters/shared/cfg_test_files.rs @@ -54,17 +54,107 @@ fn inner_cfg_test_files(parsed: &ParsedRefs<'_>) -> HashSet { .collect() } +/// Yields the owner directory of **every** `tests/` directory component in +/// `path` (`""` when `tests` is the leading component). A path may contain +/// more than one — e.g. a package nested under a directory literally named +/// `tests` (`fixtures/tests/retry/tests/it.rs`) has owners `fixtures` and +/// `fixtures/tests/retry`; the caller checks each against the package roots, +/// so the package's own `tests/` is matched at the right root. Boundary- +/// aware: `mytests/x.rs` does not match. +/// Operation: segment scan + slicing, own logic hidden in the closure. +fn tests_dir_owners(path: &str) -> impl Iterator + '_ { + path.char_indices().filter_map(move |(i, _)| { + let at_segment_start = i == 0 || path.as_bytes()[i - 1] == b'/'; + (at_segment_start && path[i..].starts_with("tests/")).then(|| { + if i == 0 { + "" + } else { + &path[..i - 1] + } + }) + }) +} + +/// If `path` is a crate-root file, return the owning package-root +/// directory (`""` for the analysis-root crate). Crate roots are Cargo's +/// defining markers of a package: the default `src/lib.rs` / `src/main.rs` +/// and autobinary `src/bin/.rs`. This identifies real package roots +/// from the parsed `.rs` set without reading any manifest. Custom +/// `[lib] path = …` / `[[bin]] path = …` layouts are not detectable from +/// paths alone (they would require parsing `Cargo.toml`). +/// Integration: combines the default-root and autobinary lookups. +fn crate_root_owner(path: &str) -> Option<&str> { + ["src/lib.rs", "src/main.rs"] + .into_iter() + .find_map(|tail| owner_with_tail(path, tail)) + .or_else(|| bin_crate_root_owner(path)) +} + +/// Owner directory of a path ending in `/{tail}` (`""` when the +/// path equals `tail`), or `None`. Boundary-aware via the trailing `/`. +/// Operation: suffix matching, no own calls. +fn owner_with_tail<'a>(path: &'a str, tail: &str) -> Option<&'a str> { + (path == tail) + .then_some("") + .or_else(|| path.strip_suffix(tail).and_then(|p| p.strip_suffix('/'))) +} + +/// Owner directory of an autobinary crate root `/src/bin/.rs` +/// (`""` for a top-level `src/bin/.rs`), or `None`. Only a file +/// directly inside `src/bin/` counts — deeper modules do not. +/// Operation: split + filter, no own calls. +fn bin_crate_root_owner(path: &str) -> Option<&str> { + path.strip_prefix("src/bin/") + .map(|name| ("", name)) + .or_else(|| path.split_once("/src/bin/")) + .filter(|(_, name)| name.ends_with(".rs") && !name.contains('/')) + .map(|(owner, _)| owner) +} + +/// Directory prefixes that are genuine Cargo package roots in the parsed +/// tree: every directory that holds a crate-root file (`src/lib.rs` or +/// `src/main.rs`). `""` denotes the analysis-root crate. Derived from the +/// actual `.rs` set — no filesystem or manifest access — so a directory +/// merely *containing* a `src/` subtree (without a crate root) does not +/// qualify. +/// Operation: filter_map over parsed paths, no own calls. +fn package_roots(parsed: &ParsedRefs<'_>) -> HashSet { + parsed + .iter() + .filter_map(|(path, _)| crate_root_owner(path).map(String::from)) + .collect() +} + +/// True for source paths Cargo compiles as integration-test binaries: +/// a file inside a `tests/` directory at a **package root** — the +/// directory owning that `tests/` must itself be a package root (i.e. be +/// in `package_roots`, derived from crate-root files). This matches +/// `tests/foo.rs` (analysis-root crate) and `crates//tests/foo.rs`, +/// but NOT a `tests/` directory without a sibling crate root +/// (`fixtures/tests/...`, `tools/shared/tests/...`, a coincidental +/// `src/`+`tests/` pair) nor one nested under a package's `src/` +/// (`src/foo/tests/bar.rs`, a unit-test submodule reached via +/// `#[cfg(test)] mod`). +/// +/// This is the single source of truth for the integration-test path +/// rule; it feeds only the cfg-test file set, which every dimension then +/// consults — so the rule cannot drift. Callers pass forward-slash +/// normalised paths. +/// Operation: owner lookup + set membership, no own calls. +pub(crate) fn is_integration_test_path(path: &str, package_roots: &HashSet) -> bool { + tests_dir_owners(path).any(|owner| package_roots.contains(owner)) +} + /// Files Cargo automatically treats as integration tests — everything -/// under the workspace-root `tests/` directory. Each is its own test -/// binary; no production code lives there. Companion test subtrees -/// under `src/**/tests/` are already reached via the `#[cfg(test)] mod` -/// detection above. -/// Operation: path-prefix filter, no own calls. +/// inside a package-root `tests/` directory. Each is its own test +/// binary; no production code lives there. +/// Operation: derive package roots, then filter, no logic. fn integration_test_files(parsed: &ParsedRefs<'_>) -> HashSet { + let roots = package_roots(parsed); parsed .iter() .map(|(path, _)| *path) - .filter(|p| p.starts_with("tests/")) + .filter(|p| is_integration_test_path(p, &roots)) .map(String::from) .collect() } diff --git a/src/adapters/shared/tests/cfg_test.rs b/src/adapters/shared/tests/cfg_test.rs new file mode 100644 index 00000000..f10181b7 --- /dev/null +++ b/src/adapters/shared/tests/cfg_test.rs @@ -0,0 +1,59 @@ +use crate::adapters::shared::cfg_test::{has_cfg_test, has_test_attr}; + +/// Parse a single free function and return its attributes. +fn fn_attrs(code: &str) -> Vec { + let file = syn::parse_file(code).expect("parse failed"); + match &file.items[0] { + syn::Item::Fn(f) => f.attrs.clone(), + _ => panic!("expected a free fn"), + } +} + +#[test] +fn bare_test_attribute_recognized() { + assert!(has_test_attr(&fn_attrs("#[test] fn t() {}"))); +} + +#[test] +fn framework_test_attributes_recognized() { + // Any attribute whose path ends in `test` is a test entry point: + // the async-runtime test macros all follow this shape. + assert!( + has_test_attr(&fn_attrs( + "#[tokio::test(flavor = \"multi_thread\")] async fn t() {}" + )), + "#[tokio::test] must be recognised" + ); + assert!( + has_test_attr(&fn_attrs("#[async_std::test] async fn t() {}")), + "#[async_std::test] must be recognised" + ); + assert!( + has_test_attr(&fn_attrs("#[googletest::test] fn t() {}")), + "#[googletest::test] must be recognised" + ); + // The renamed-macro frameworks that do not end in `test`: + assert!( + has_test_attr(&fn_attrs("#[rstest] fn t() {}")), + "#[rstest] must be recognised" + ); + assert!( + has_test_attr(&fn_attrs("#[test_case(1)] fn t() {}")), + "#[test_case] must be recognised" + ); +} + +#[test] +fn non_test_attributes_not_recognized() { + assert!(!has_test_attr(&fn_attrs("#[inline] fn t() {}"))); + assert!(!has_test_attr(&fn_attrs("#[allow(dead_code)] fn t() {}"))); + // `#[cfg(test)]` is a separate signal handled by has_cfg_test, not + // has_test_attr — keep the two predicates distinct. + assert!(!has_test_attr(&fn_attrs("#[cfg(test)] fn t() {}"))); +} + +#[test] +fn cfg_test_attribute_still_distinct() { + assert!(has_cfg_test(&fn_attrs("#[cfg(test)] fn t() {}"))); + assert!(!has_cfg_test(&fn_attrs("#[test] fn t() {}"))); +} diff --git a/src/adapters/shared/tests/cfg_test_files.rs b/src/adapters/shared/tests/cfg_test_files.rs index d25eac92..f878eecb 100644 --- a/src/adapters/shared/tests/cfg_test_files.rs +++ b/src/adapters/shared/tests/cfg_test_files.rs @@ -80,6 +80,319 @@ fn cfg_test_propagation_does_not_tag_unrelated_files() { ); } +#[test] +fn per_crate_tests_dir_recognized_as_integration_test() { + // Cargo compiles `/tests/*.rs` as integration-test + // binaries. In a workspace the package root is a sub-directory + // (`crates//`), so the integration-test files live at + // `crates//tests/*.rs` — not at the workspace-root `tests/`. + // These must be recognised as test-only just like the top-level + // layout, otherwise their test-attributed entry points are falsely + // reported as dead code. + let code = r#" + #[tokio::test] + async fn end_to_end() {} + "#; + let lib = "pub fn run() {}"; + let parsed = vec![ + ( + "crates/sv-utility-retry/src/lib.rs".to_string(), + lib.to_string(), + syn::parse_file(lib).unwrap(), + ), + ( + "crates/sv-utility-retry/tests/integration.rs".to_string(), + code.to_string(), + syn::parse_file(code).unwrap(), + ), + ]; + let result = collect_cfg_test_file_paths(&parsed); + assert!( + result.contains("crates/sv-utility-retry/tests/integration.rs"), + "per-crate tests/ integration file must be cfg-test: {result:?}" + ); +} + +#[test] +fn workspace_root_tests_dir_recognized_as_integration_test() { + // The single-crate layout: integration tests at the analysis-root + // `tests/` directory, with the crate's own `src/` present so the + // root ("") qualifies as a package root. + let code = "#[test] fn it_works() {}"; + let lib = "pub fn run() {}"; + let parsed = vec![ + ( + "src/lib.rs".to_string(), + lib.to_string(), + syn::parse_file(lib).unwrap(), + ), + ( + "tests/integration.rs".to_string(), + code.to_string(), + syn::parse_file(code).unwrap(), + ), + ]; + let result = collect_cfg_test_file_paths(&parsed); + assert!( + result.contains("tests/integration.rs"), + "analysis-root tests/ integration file must be cfg-test: {result:?}" + ); +} + +#[test] +fn tests_dir_only_classified_at_a_real_package_root() { + // Review: a `tests/` directory counts as Cargo integration tests only + // when its parent is a real package root — i.e. that directory also + // holds a `src/` tree in the parsed file set. A `tests/` directory + // anywhere else (fixtures, tooling, data) is production code and must + // NOT enter the shared cfg-test set, or findings get hidden across + // IOSP/SRP/architecture/DRY/TQ. + let crate_lib = "pub fn run() {}"; + let it = "#[test] fn it() {}"; + let fixture = "pub fn support() {}"; + let tool = "pub fn helper() {}"; + let parsed = vec![ + ( + "crates/x/src/lib.rs".to_string(), + crate_lib.to_string(), + syn::parse_file(crate_lib).unwrap(), + ), + ( + "crates/x/tests/it.rs".to_string(), + it.to_string(), + syn::parse_file(it).unwrap(), + ), + ( + "fixtures/tests/support.rs".to_string(), + fixture.to_string(), + syn::parse_file(fixture).unwrap(), + ), + ( + "tools/shared/tests/helpers.rs".to_string(), + tool.to_string(), + syn::parse_file(tool).unwrap(), + ), + ]; + let result = collect_cfg_test_file_paths(&parsed); + assert!( + result.contains("crates/x/tests/it.rs"), + "tests/ at a real package root (has sibling src/) must be cfg-test: {result:?}" + ); + assert!( + !result.contains("fixtures/tests/support.rs"), + "tests/ with no sibling src/ must NOT be cfg-test: {result:?}" + ); + assert!( + !result.contains("tools/shared/tests/helpers.rs"), + "nested non-package-root tests/ must NOT be cfg-test: {result:?}" + ); +} + +#[test] +fn package_nested_under_a_tests_dir_recognizes_its_own_tests() { + // Edge case: a real package whose path contains an earlier `tests` + // segment (`fixtures/tests/retry/`). Its OWN integration tests live at + // `fixtures/tests/retry/tests/…`; the owner of *that* `tests/` is the + // package root, even though an outer `tests` segment appears first. + // The outer `fixtures/tests/other.rs` (no package root there) must + // still NOT be classified. + let lib = "pub fn run() {}"; + let it = "#[test] fn it() {}"; + let outer = "pub fn outer() {}"; + let parsed = vec![ + ( + "fixtures/tests/retry/src/lib.rs".to_string(), + lib.to_string(), + syn::parse_file(lib).unwrap(), + ), + ( + "fixtures/tests/retry/tests/integration.rs".to_string(), + it.to_string(), + syn::parse_file(it).unwrap(), + ), + ( + "fixtures/tests/other.rs".to_string(), + outer.to_string(), + syn::parse_file(outer).unwrap(), + ), + ]; + let result = collect_cfg_test_file_paths(&parsed); + assert!( + result.contains("fixtures/tests/retry/tests/integration.rs"), + "the package's own tests/ (owner = package root) must be cfg-test: {result:?}" + ); + assert!( + !result.contains("fixtures/tests/other.rs"), + "an outer `tests` dir that is not a package root must NOT be cfg-test: {result:?}" + ); +} + +#[test] +fn tests_dir_next_to_src_without_crate_root_file_not_classified() { + // The package-root signal is Cargo's crate-root file (`src/lib.rs` or + // `src/main.rs`), derived from the parsed set — NOT the mere presence + // of a `src/` segment. A directory with a `src/` subtree but no + // crate-root file is not a package, so its sibling `tests/` is + // production code, not integration tests. Guards the case of a + // deeply nested coincidental `src/`+`tests/` pair. + let internal = "pub fn data() {}"; // under src/, but not a crate root + let support = "pub fn support() {}"; + let parsed = vec![ + ( + "vendor/widget/src/internal.rs".to_string(), + internal.to_string(), + syn::parse_file(internal).unwrap(), + ), + ( + "vendor/widget/tests/support.rs".to_string(), + support.to_string(), + syn::parse_file(support).unwrap(), + ), + ]; + let result = collect_cfg_test_file_paths(&parsed); + assert!( + !result.contains("vendor/widget/tests/support.rs"), + "tests/ next to a src/ without a crate-root file must NOT be cfg-test: {result:?}" + ); +} + +#[test] +fn tests_dir_next_to_crate_root_file_classified() { + // The flip side: a `src/lib.rs` (or `src/main.rs`) marks a real + // package root, so its sibling `tests/` is integration tests. + let lib = "pub fn run() {}"; + let it = "#[test] fn it() {}"; + let parsed = vec![ + ( + "vendor/widget/src/lib.rs".to_string(), + lib.to_string(), + syn::parse_file(lib).unwrap(), + ), + ( + "vendor/widget/tests/support.rs".to_string(), + it.to_string(), + syn::parse_file(it).unwrap(), + ), + ]; + let result = collect_cfg_test_file_paths(&parsed); + assert!( + result.contains("vendor/widget/tests/support.rs"), + "tests/ next to a crate-root file must be cfg-test: {result:?}" + ); +} + +#[test] +fn tests_dir_next_to_bin_only_crate_classified() { + // A binary-only package (autobins in `src/bin/`, no `lib.rs`/`main.rs`) + // is a real crate root, so its sibling `tests/` are integration tests. + let bin = "fn main() {}"; + let it = "#[test] fn it() {}"; + let parsed = vec![ + ( + "crates/cli/src/bin/tool.rs".to_string(), + bin.to_string(), + syn::parse_file(bin).unwrap(), + ), + ( + "crates/cli/tests/it.rs".to_string(), + it.to_string(), + syn::parse_file(it).unwrap(), + ), + ]; + let result = collect_cfg_test_file_paths(&parsed); + assert!( + result.contains("crates/cli/tests/it.rs"), + "tests/ next to a src/bin/ autobinary crate must be cfg-test: {result:?}" + ); +} + +#[test] +fn is_integration_test_path_matches_package_root_tests_only() { + use crate::adapters::shared::cfg_test_files::is_integration_test_path; + use std::collections::HashSet; + // Package roots derived from the file set: the analysis-root crate + // (""), a workspace member, and a member whose name contains "src". + let roots: HashSet = ["", "crates/x", "crates/my-src-tool"] + .iter() + .map(|s| s.to_string()) + .collect(); + // `tests/` directly under a package root → integration test. + assert!(is_integration_test_path("tests/it.rs", &roots)); + assert!(is_integration_test_path("crates/x/tests/it.rs", &roots)); + // A member whose name contains "src" is matched by its real root. + assert!(is_integration_test_path( + "crates/my-src-tool/tests/it.rs", + &roots + )); + // `tests/` whose owning directory is not a package root → not an + // integration test (fixtures, tooling, nested unit-test submodules). + assert!(!is_integration_test_path( + "fixtures/tests/support.rs", + &roots + )); + assert!(!is_integration_test_path( + "tools/shared/tests/helpers.rs", + &roots + )); + assert!(!is_integration_test_path("src/foo/tests/bar.rs", &roots)); + assert!(!is_integration_test_path( + "crates/x/src/foo/tests/bar.rs", + &roots + )); + // No `tests/` segment at all. + assert!(!is_integration_test_path("src/lib.rs", &roots)); +} + +#[test] +fn nested_src_tests_dir_not_blanket_classified_as_integration() { + // Regression guard for the review finding: `contains("/tests/")` was + // too broad and classified ANY nested `tests` directory as test-only. + // A plain production file under `src/**/tests/` that is NOT reached + // via `#[cfg(test)] mod` must not be auto-classified — otherwise real + // findings get hidden across dimensions. + let code = "pub fn connect() {}"; + let parsed = vec![( + "src/database/tests/connection.rs".to_string(), + code.to_string(), + syn::parse_file(code).unwrap(), + )]; + let result = collect_cfg_test_file_paths(&parsed); + assert!( + !result.contains("src/database/tests/connection.rs"), + "production file under src/**/tests/ must not be auto cfg-test: {result:?}" + ); +} + +#[test] +fn src_tests_dir_reached_via_cfg_test_mod_still_classified() { + // The flip side: when a `src/**/tests/` file IS reached through a + // `#[cfg(test)] mod tests;` chain it must still be cfg-test — that + // path comes from the mod-chain detector, not the integration-path + // heuristic, so tightening the heuristic must not regress it. + let parent_code = r#" + #[cfg(test)] + mod tests; + "#; + let child_code = "pub fn helper() -> u32 { 42 }"; + let parsed = vec![ + ( + "src/database.rs".to_string(), + parent_code.to_string(), + syn::parse_file(parent_code).unwrap(), + ), + ( + "src/database/tests.rs".to_string(), + child_code.to_string(), + syn::parse_file(child_code).unwrap(), + ), + ]; + let result = collect_cfg_test_file_paths(&parsed); + assert!( + result.contains("src/database/tests.rs"), + "src test file reached via #[cfg(test)] mod must still be cfg-test: {result:?}" + ); +} + #[test] fn collect_cfg_test_file_paths_basic() { let parent_code = r#" diff --git a/src/adapters/shared/tests/mod.rs b/src/adapters/shared/tests/mod.rs index 04fdb19a..bc1a3e8d 100644 --- a/src/adapters/shared/tests/mod.rs +++ b/src/adapters/shared/tests/mod.rs @@ -1,3 +1,4 @@ +mod cfg_test; mod cfg_test_files; mod file_to_module; mod normalize; diff --git a/src/adapters/source/filesystem.rs b/src/adapters/source/filesystem.rs index 15fa794c..7703522b 100644 --- a/src/adapters/source/filesystem.rs +++ b/src/adapters/source/filesystem.rs @@ -1,4 +1,3 @@ -use std::collections::HashSet; use std::path::{Path, PathBuf}; use walkdir::WalkDir; @@ -89,69 +88,6 @@ pub(crate) fn read_and_parse_files( .collect() } -/// Get Rust files changed vs a git ref. -/// Operation: shells out to git and parses output. -pub(crate) fn get_git_changed_files(path: &Path, git_ref: &str) -> Result, String> { - let dir = if path.is_file() { - path.parent().unwrap_or(path) - } else { - path - }; - - let root_output = std::process::Command::new("git") - .args(["rev-parse", "--show-toplevel"]) - .current_dir(dir) - .output() - .map_err(|e| format!("Failed to run git: {e}"))?; - if !root_output.status.success() { - return Err("Not a git repository".into()); - } - - let git_root = PathBuf::from(String::from_utf8_lossy(&root_output.stdout).trim()); - - let output = std::process::Command::new("git") - .args([ - "diff", - "--name-only", - "--diff-filter=ACMR", - git_ref, - "--", - "*.rs", - ]) - .current_dir(&git_root) - .output() - .map_err(|e| format!("Failed to run git diff: {e}"))?; - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - return Err(format!("git diff failed: {}", stderr.trim())); - } - - let files = String::from_utf8_lossy(&output.stdout) - .lines() - .filter(|l| !l.is_empty()) - .map(|l| git_root.join(l)) - .collect(); - - Ok(files) -} - -/// Filter file list to only those present in the changed set. -/// Operation: set-intersection logic using canonical paths. -pub(crate) fn filter_to_changed(all: Vec, changed: &[PathBuf]) -> Vec { - let changed_canonical: HashSet = changed - .iter() - .filter_map(|c| std::fs::canonicalize(c).ok()) - .collect(); - - all.into_iter() - .filter(|f| { - std::fs::canonicalize(f) - .map(|c| changed_canonical.contains(&c)) - .unwrap_or(false) - }) - .collect() -} - /// Scan source lines and collect per-file results via a closure. /// Trivial: generic iteration infrastructure, no own calls. fn collect_per_file( diff --git a/src/adapters/source/tests/filesystem.rs b/src/adapters/source/tests/filesystem.rs index 16001e6d..1b4f3ad0 100644 --- a/src/adapters/source/tests/filesystem.rs +++ b/src/adapters/source/tests/filesystem.rs @@ -1,74 +1,4 @@ use crate::adapters::source::filesystem::*; -use std::path::PathBuf; - -#[test] -fn test_filter_to_changed_matching() { - let dir = tempfile::Builder::new() - .prefix("rustqual_test_") - .tempdir() - .unwrap(); - let a = dir.path().join("a.rs"); - let b = dir.path().join("b.rs"); - let c = dir.path().join("c.rs"); - std::fs::write(&a, "").unwrap(); - std::fs::write(&b, "").unwrap(); - std::fs::write(&c, "").unwrap(); - - let all = vec![a.clone(), b, c.clone()]; - let changed = vec![a, c]; - let result = filter_to_changed(all, &changed); - assert_eq!(result.len(), 2); -} - -#[test] -fn test_filter_to_changed_none_matching() { - let dir = tempfile::Builder::new() - .prefix("rustqual_test_") - .tempdir() - .unwrap(); - let a = dir.path().join("a.rs"); - let d = dir.path().join("d.rs"); - std::fs::write(&a, "").unwrap(); - std::fs::write(&d, "").unwrap(); - - let all = vec![a]; - let changed = vec![d]; - let result = filter_to_changed(all, &changed); - assert!(result.is_empty()); -} - -#[test] -fn test_filter_to_changed_empty_changed() { - let dir = tempfile::Builder::new() - .prefix("rustqual_test_") - .tempdir() - .unwrap(); - let a = dir.path().join("a.rs"); - std::fs::write(&a, "").unwrap(); - - let all = vec![a]; - let changed: Vec = vec![]; - let result = filter_to_changed(all, &changed); - assert!(result.is_empty()); -} - -#[test] -fn test_filter_to_changed_empty_all() { - let all: Vec = vec![]; - let changed: Vec = vec![PathBuf::from("/tmp/x.rs")]; - let result = filter_to_changed(all, &changed); - assert!(result.is_empty()); -} - -#[test] -fn test_get_git_changed_files_not_git_repo() { - let dir = tempfile::Builder::new() - .prefix("rustqual_test_") - .tempdir() - .unwrap(); - let result = get_git_changed_files(dir.path(), "HEAD"); - assert!(result.is_err()); -} #[test] fn test_collect_rust_files_dot_prefix_path() { diff --git a/src/app/tests/run.rs b/src/app/tests/run.rs index b4c37abd..78d501f6 100644 --- a/src/app/tests/run.rs +++ b/src/app/tests/run.rs @@ -334,23 +334,3 @@ fn test_extract_init_metrics_with_complexity() { assert_eq!(m.max_nesting_depth, 3); assert_eq!(m.max_function_lines, 45); } - -// ── Diff CLI flag tests ────────────────────────────────────── - -#[test] -fn test_diff_cli_default_ref() { - let cli = Cli::parse_from(["test", "--diff"]); - assert_eq!(cli.diff.as_deref(), Some("HEAD")); -} - -#[test] -fn test_diff_cli_custom_ref() { - let cli = Cli::parse_from(["test", "--diff", "main"]); - assert_eq!(cli.diff.as_deref(), Some("main")); -} - -#[test] -fn test_diff_cli_not_set() { - let cli = Cli::parse_from(["test"]); - assert!(cli.diff.is_none()); -} diff --git a/src/cli/mod.rs b/src/cli/mod.rs index 7c9d93a9..289b1a7f 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -128,11 +128,6 @@ pub(crate) struct Cli { #[arg(long)] pub sort_by_effort: bool, - /// Analyze only files changed vs a git ref (default: HEAD). - /// Conflicts with --watch. - #[arg(long, value_name = "REF", num_args = 0..=1, default_missing_value = "HEAD", conflicts_with = "watch")] - pub diff: Option, - /// Path to an LCOV coverage file for test quality analysis (TQ-004, TQ-005). #[arg(long, value_name = "LCOV_FILE")] pub coverage: Option, diff --git a/src/lib.rs b/src/lib.rs index 4cfa696b..0bb1fa92 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -79,24 +79,6 @@ pub fn run() -> Result<(), i32> { } let files = adapters::source::filesystem::collect_filtered_files(&cli.path, &config); - let files = if let Some(ref git_ref) = cli.diff { - match adapters::source::filesystem::get_git_changed_files(&cli.path, git_ref) { - Ok(changed) => { - let filtered = adapters::source::filesystem::filter_to_changed(files, &changed); - eprintln!( - "[diff mode: {} changed file(s) vs {git_ref}]", - filtered.len() - ); - filtered - } - Err(e) => { - eprintln!("Warning: {e}. Analyzing all files."); - files - } - } - } else { - files - }; if files.is_empty() { eprintln!("No Rust source files found in {}", cli.path.display()); return Ok(());