test: fix temp-fixture directory collisions in parser test suites - #399
Conversation
`TempLogFixture::new` (and the shared `TempBenchFile`) derived their temp directory name from `SystemTime::now().as_nanos()`. Clock granularity is coarser than a nanosecond, so two fixtures constructed in the same observed tick got the same path. Because each impl'd `Drop` as `remove_dir_all(&dir)`, whichever test finished first deleted the other's fixture file; the victim then parsed zero entries and panicked indexing `entries[0]`. The harness runs each test binary's tests on parallel threads, so this made `cargo test` intermittently fail (roughly 1 run in 5 on an M-series Mac). Switch all four fixtures to `tempfile::TempDir` (already a dev-dependency), which creates the directory with a genuinely unique name and removes it on drop, so the hand-rolled `Drop` impls go away too. Verified with 12 consecutive clean `cargo test --locked` runs plus one `--all-features` run; `cargo clippy --all-targets -- -D warnings` is clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughTest fixtures across the parser test suites now use ChangesTemporary fixture cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes intermittent, parallel-test fixture collisions in the src-tauri parser test suites by switching from time-derived temp directory names (which can collide on coarse-granularity clocks) to tempfile::TempDir-managed unique directories. This stabilizes integration tests that create and clean up temp fixtures while the Rust test harness runs tests concurrently.
Changes:
- Replaced
SystemTime::now().as_nanos()-derived temp directory naming withTempDir::with_prefix(...)across affected test/bench fixtures. - Removed custom
Dropimplementations that manuallyremove_dir_all’d shared temp paths, relying onTempDir’s cleanup instead. - Updated fixture structs to retain the
TempDirvia a_dir: TempDirfield so the directory outlives any paths derived from it.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src-tauri/tests/cmtlog_parser.rs | Uses TempDir to create per-test unique fixture directories; removes manual cleanup Drop. |
| src-tauri/tests/parser_regression_corpus.rs | Uses TempDir for unique temp fixtures to avoid cross-test directory collisions. |
| src-tauri/tests/parser_expanded_corpus.rs | Migrates temp fixture directory creation to TempDir and removes manual directory deletion. |
| src-tauri/tests/common/mod.rs | Updates shared bench/test fixture helper to use TempDir for safe unique directories and cleanup. |
Problem
src-tauri/tests/cmtlog_parser.rshad a race that made the whole test binary intermittently fail:TempLogFixture::newderived its temp directory name fromSystemTime::now().duration_since(UNIX_EPOCH).as_nanos().as_nanos()does not guarantee distinct values between calls; macOS clock granularity is coarser than a nanosecond, so two fixtures constructed within the same observed tick got the same directory path.Each fixture also implemented
Dropasfs::remove_dir_all(&self.dir). The test harness runs a binary's tests on parallel threads, so whichever test finished first deleted the other test's fixture file. The victim then read an empty/missing file, parsed zero entries, and panicked onresult.entries[0].This reproduced roughly 1 run in 5 of the full
cargo test --lockedsuite on an M-series Mac. Running the single binary alone rarely reproduced it, since there is far less scheduling pressure.Scope
The same time-derived pattern had been copied into four places, all with the same
remove_dir_alldestructor:src-tauri/tests/cmtlog_parser.rscmtrace-open-cmtlog-test-src-tauri/tests/parser_regression_corpus.rscmtrace-open-parser-regression-src-tauri/tests/parser_expanded_corpus.rscmtrace-open-expanded-src-tauri/tests/common/mod.rs(TempBenchFile)cmtrace-open-bench-tests/common/mod.rsis also pulled intobenches/intune_pipeline.rsvia#[path = "../tests/common/mod.rs"], so the benchmark fixture carried the defect too.Fix
tempfile = "3"was already in[dev-dependencies], so each fixture now holds aTempDirbuilt withTempDir::with_prefix(...). That takes uniqueness from the OS (random suffix plusO_EXCLretry) rather than from the clock, andTempDir's ownDrophandles cleanup, so all four hand-rolledDropimpls are removed. Net 34 lines deleted.The fixture structs keep the directory alive via a
_dir: TempDirfield; dropping it early would delete the directory out from under the still-livepath.Verification
cargo test --lockedruns, 785 tests each, zero failurescargo test --locked --all-featuresrun, adding the feature-gated binaries includingesp_diagnostics_sources, zero failurescargo clippy --all-targets -- -D warningscleanNote
cargo fmtreformatted 13 unrelated files on this branch that were already non-conformant (12 undersrc-tauri/src/, plustests/esp_diagnostics_sources.rs). That collateral was reverted to keep this diff minimal. There is nocargo fmt --checkgate in.github/workflows/, so the drift is pre-existing and unenforced; worth cleaning up separately if desired.Found while working #395, but entirely unrelated to it, so it lands on its own.
🤖 Generated with Claude Code
Summary by CodeRabbit