Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a test-quality review document describing reliability/coverage gaps across the workspace test suite, and includes a small change intended to improve adversary-detector digest test isolation under parallel Tokio execution.
Changes:
- Add a new documentation review covering flaky/ignored tests, Loom entry points, CI test tier semantics, and fixed-port CI risks.
- Update adversary-detector digest tests to generate per-test temp paths using an atomic monotonic suffix (instead of a timestamp) to avoid same-run collisions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| docs/test-quality-review-2026-04-25.md | New test-quality review document capturing reliability and CI/test-tier recommendations. |
| crates/adversary-detector/src/digest.rs | Adjusts test temp-path generation to avoid concurrent test collisions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn tmp_path() -> PathBuf { | ||
| // Use a unique path in the system temp dir | ||
| // Tokio runs these tests concurrently; use a monotonic suffix so parallel | ||
| // tests cannot accidentally share a digest store. | ||
| let dir = std::env::temp_dir(); | ||
| let name = format!( | ||
| "digest-test-{}-{}", | ||
| std::process::id(), | ||
| std::time::SystemTime::now() | ||
| .duration_since(std::time::UNIX_EPOCH) | ||
| .unwrap() | ||
| .as_nanos() | ||
| NEXT_TMP_PATH_ID.fetch_add(1, Ordering::Relaxed) | ||
| ); | ||
| dir.join(name) |
There was a problem hiding this comment.
tmp_path() now uses only (pid, monotonic counter). Because test files are not cleaned up, a reused PID on the same machine can cause a later test run to reuse an existing digest file and pick up stale state. Consider using tempfile (already in dev-dependencies) to create an auto-cleaned temp file/dir, or include an additional randomness/time component and explicitly remove the file after each test.
There was a problem hiding this comment.
Codex integration sweep: addressed in #38 without editing this PR branch. Digest tests now allocate paths under tempfile-created directories rather than pid/counter-derived names.
|
Acknowledged. Test-quality reviews from both this PR and #37 (Round 1+2+3 audit) are complementary — different files audited. Will fold the Round-3 findings (real API token in config.rs, validators-with-zero-decision-tree-tests, etc.) into the next test-coverage pass. |
Summary
Why
The suite is broad and useful, but several tests can produce false confidence or flake under concurrency. The digest race reproduced twice during branch pushes, so this PR includes the minimal reliability fix alongside the review.
Validation
Draft until the recommendations are triaged and the duplicated digest fix is reconciled with PR #30 if needed.