Repository navigation
fix(tests): make dotnet_trx mtime-dependent tests deterministic - #4048
Conversation
|
|
📊 Automated PR Analysis
SummaryFixes two flaky tests in dotnet_trx.rs that relied on thread::sleep and small mtime deltas to establish file ordering, which could fail on filesystems with coarse mtime granularity. Replaces this with explicit mtime setting via the filetime crate using wide 10-second gaps, added as a new dev-dependency. Review Checklist
Linked issues: #4037 Analyzed automatically by wshm · This is an automated analysis, not a human review. |
|
@KuSh Could you please review it once ? |
5c7dfb4 to
e174518
Compare
test_parse_trx_files_in_dir_since_ignores_older_files flaked when
filesystem mtime granularity quantized new.trx's mtime at or below
the 'since' threshold, leaving zero summaries and panicking on
.expect("merged summary") (fixes rtk-ai#4037).
Set explicit mtimes (now-10s / now+10s) via the filetime dev crate
instead of deriving them from write order, and apply the same fix to
test_find_recent_trx_in_dir_picks_newest_trx, which relied on a 5ms
sleep plus max_by_key over unspecified read_dir order.
e174518 to
9082031
Compare
std::fs::File::set_modified has covered this since Rust 1.75, well under the crate's 1.91 rust-version, so the filetime dev-dependency buys nothing. Dropping it also drops the Cargo.toml hunk, which added a second [dev-dependencies] table and made the manifest a duplicate key once develop grew its own. Extend the same treatment to the remaining sleep-ordered mtime test, merge_test_summary_from_trx_ignores_stale_fallback_file, whose 5 ms sleep left the stale/fresh distinction to filesystem mtime granularity. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
KuSh
left a comment
KuSh
left a comment
There was a problem hiding this comment.
Round 1 of 3 — APPROVED
Claim: make the two mtime-dependent dotnet_trx tests deterministic by setting mtimes explicitly instead of relying on thread::sleep plus filesystem granularity (Fixes #4037).
Scope: accept. The diagnosis in #4037 is right, and it is fixed at the right level — the ordering guarantee moves from "whatever the filesystem records" to an explicit value.
Ran: two implementation mutations; 40× determinism loop; cargo test --all against the PR merged into current develop; cargo check on that same merge before the fixup.
CI @1281273e: all 11 green, Windows included.
Blocks merge — resolved
Cargo.toml: merged with currentdevelop, the tree did not build. The PR added a[dev-dependencies]table afterwhich = "8";developgrew its own (temp-env) in the edition-2024 series. Git merges both without a conflict marker, and cargo then rejects the manifest outright:
error: duplicate key
--> Cargo.toml:60:2
|
60 | [dev-dependencies]
Your CI was green and still is — it just could not have seen this. Your run finished 2026-09-14 11:05; #3782 landed on develop at 20:21 the same day. Green on the branch, unbuildable after merge.
Fixup pushed — 1281273e
Rather than reshuffle the manifest, the fixup removes the need for one. std::fs::File::set_modified has done this in std since Rust 1.75, comfortably under the crate's rust-version = "1.91", so filetime was not buying anything. Dropping it takes the Cargo.toml and Cargo.lock hunks with it, and that is what makes the merge clean.
It also extends the same treatment to test_merge_test_summary_from_trx_ignores_stale_fallback_file in dotnet_cmd.rs, the one remaining test where a 5 ms sleep was the only thing separating "stale" from "fresh". Forcing that fixture's mtime 1 s forward makes it fail, so the mechanism was live, just with a narrower trigger than the two you found. There are now no sleep-ordered mtime tests left in src/cmds/dotnet/.
Verified on the fixup merged into current develop: one [dev-dependencies] table, cargo fmt --check and cargo clippy --all-targets clean, cargo test --all green (3613 unit + all integration), 40/40 on the two tests.
Checked and correct
- Your fix does not hollow out either assertion. Flipping
max_by_keytomin_by_keyinfind_recent_trx_in_dirstill failstest_find_recent_trx_in_dir_picks_newest_trx; disabling thesincefilter still failstest_parse_trx_files_in_dir_since_ignores_older_files(left: 5, right: 3). That was the thing worth checking — a determinism fix that quietly stops testing anything is the usual way this goes wrong, and yours does not. - The future (
now + 10s) mtimes are deliberate and harmless: they widen the gap on both sides ofsince, and the production path only ever reads mtimes.
Your questions
- "Could you please review it once?" → Done. Good catch and a correct diagnosis; the only thing between it and merge was a manifest collision that appeared on
developnine hours after your CI ran.
Next
Nothing — approving. CI is green on 1281273e.
Rounds: 1/3. Threads: 0 open, 0 resolved.
3 participants
Fixes #4037
Problem: Three tests were flaky because they relied on filesystem mtime granularity and write ordering:
dotnet_trx::tests::test_parse_trx_files_in_dir_since_ignores_older_filesdotnet_trx::tests::test_find_recent_trx_in_dir_picks_newest_trxdotnet_cmd::tests::test_merge_test_summary_from_trx_ignores_stale_fallback_fileAll three used
std::thread::sleep(5-10 ms) to create a time gap between file writes, then compared mtimes against a threshold. On filesystems with coarse mtime granularity (1 s on some CI filesystems, FAT 2 s buckets) the mtimes become indistinguishable, so "older" and "newer" can no longer be told apart and the assertion flips.Solution: set mtimes explicitly with wide gaps (±10 seconds), which is immune to any realistic filesystem granularity.
now - 10s, new files tonow + 10s, with the threshold atnowstd::fs::File::set_modified(stable since Rust 1.75, and the crate requires 1.91), so no new dependency is neededNote: the first draft of this PR added the
filetimecrate as a dev-dependency. That introduced a second[dev-dependencies]table inCargo.toml, which collided with the onedevelopgained in the edition-2024 series (#3782) and made the merged manifest fail witherror: duplicate key. The maintainer fixup in 1281273 switches to the std equivalent, which drops theCargo.toml/Cargo.lockchanges entirely, and extends the same treatment to the third test above.