Harden WaitsFor timeout test

[thomhurst]

Summary

• Replace the flaky stopwatch lower-bound check in WaitsFor_Fails_When_Timeout_Expires with assertions on the timeout failure diagnostics.
• Avoid asserting retry counts or elapsed time, so the test is not sensitive to OS timer resolution or constrained runner scheduling.
• Use a slightly larger named timeout and assert that the diagnostic reports that timeout value.

Tests

• dotnet run --project TUnit.Assertions.Tests\TUnit.Assertions.Tests.csproj --configuration Release --framework net10.0 --no-build --treenode-filter "/*/*/WaitsForAssertionTests/*" passed before the final simplification.
• Final local rebuild was blocked by local GitVersion/file-lock failures before reaching this test project; CI should verify the pushed branch.

[claude]

Code Review

Overall: Solid, targeted fix. Replacing a wall-clock lower-bound assertion with a polling-count check is the right approach for removing timer-resolution flakiness.

What works well

• Switching from Stopwatch.Elapsed >= 100ms to attempts > 1 removes the OS timer-resolution sensitivity that can fire early on Windows/slow CI environments.
• Interlocked.Increment is a good habit even if WaitsFor always polls on the same thread — it's defensive and self-documenting.
• Extracting timeout as a named variable and interpolating it into the message assertion ($"...{timeout.TotalMilliseconds:F0}ms") is strictly better than the hardcoded "100ms" — changing the timeout won't silently break the message check.

One thing to consider

attempts > 1 is a very weak lower bound. It proves at least two polls happened, but a hypothetical regression that fires the timeout after only 20 ms (2 polls) would still pass. With timeout = 250ms and pollingInterval = 10ms the expected poll count is ~25.

A CI-safe but tighter alternative that doesn't reintroduce wall-clock fragility:

// Expect at least half the theoretical polls — tolerates slow CI while catching early-exit bugs
var minExpectedAttempts = (int)(timeout.TotalMilliseconds / pollingInterval.TotalMilliseconds) / 2;
await Assert.That(attempts).IsGreaterThanOrEqualTo(minExpectedAttempts);

This would assert >= 12 here, which is still very achievable on any real hardware while giving meaningful coverage.

That said, even > 1 combined with the AssertionException throw and the message content check gives reasonable coverage of the intent, so this is not a blocker — just worth thinking about if the goal is to prove the full timeout was exercised, not just that polling started.

Nit

The jump from 100 ms to 250 ms isn't explained in the PR description or a comment. Since the new assertion doesn't rely on wall-clock duration, the 100 ms value would still work. If there's a reason for 250 ms (e.g., making the > 1 assertion more reliable on particularly slow runners), a one-liner comment would be useful.

---

No blocking issues. The fix is correct and the direction is right.

[claude]

Code Review

Overall: Solid, targeted fix. Replacing a wall-clock lower-bound assertion with a polling-count check is the right approach for removing timer-resolution flakiness.

What works well

• Switching from Stopwatch.Elapsed >= 100ms to attempts > 1 removes the OS timer-resolution sensitivity that can fire early on Windows/slow CI environments.
• Interlocked.Increment is a good habit even if WaitsFor always polls on the same thread — it is defensive and self-documenting.
• Extracting timeout as a named variable and interpolating it into the message assertion ($"...{timeout.TotalMilliseconds:F0}ms") is strictly better than the hardcoded "100ms" — changing the timeout value won't silently break the message check.

One thing to consider

attempts > 1 is a very weak lower bound. It proves at least two polls happened, but a hypothetical regression that fires the timeout after only 20 ms (2 polls) would still pass. With timeout = 250ms and pollingInterval = 10ms the expected poll count is ~25.

A CI-safe but tighter alternative that doesn't reintroduce wall-clock fragility:

// Expect at least half the theoretical polls — tolerates slow CI while catching early-exit bugs
var minExpectedAttempts = (int)(timeout.TotalMilliseconds / pollingInterval.TotalMilliseconds) / 2;
await Assert.That(attempts).IsGreaterThanOrEqualTo(minExpectedAttempts);

This would assert >= 12 here, which is still very achievable on any real hardware while giving meaningful coverage of the full timeout duration.

That said, even > 1 combined with the AssertionException throw and the message content check gives reasonable coverage of the intent, so this is not a blocker — just worth thinking about if the goal is to prove the full timeout was exercised.

Nit

The jump from 100 ms to 250 ms isn't explained in the PR description or a comment. Since the new assertion doesn't rely on wall-clock duration, the 100 ms value would still work. If there's a reason for 250 ms (e.g., extra breathing room for slow runners), a one-liner comment would help.

---

No blocking issues. The fix is correct and the direction is right.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity

Metric	Results
Complexity	0

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[claude]

Updated Review (second commit: 3de91710)

Following up on my earlier review (which covered the first commit that used Interlocked.Increment + attempts > 1): the second commit simplifies further by dropping the poll-count assertion entirely and reverting to a plain value = 1. Here is my assessment of the current state.

What changed and why it matters

The first commit replaced the flaky Stopwatch.Elapsed >= 100ms check with a counter-based proof of polling. My review flagged that attempts > 1 was a weak lower bound. The author's response was to remove the poll-count assertion altogether rather than tighten it — a valid and defensible call.

Current assertions:

1. An AssertionException is thrown → proves the timeout mechanism fires
2. Message contains "assertion did not pass within 250ms" → proves diagnostics report the correct value (and this matches the implementation's {_timeout.TotalMilliseconds:F0}ms format on line 92 of WaitsForAssertion.cs exactly)
3. Message contains "Last error:" → proves the last assertion error is surfaced

Remaining observation from my prior review

The jump from 100 ms to 250 ms is still unexplained. Since the test no longer relies on wall-clock duration at all, the original 100 ms would have worked just as well. If the intent is to give slow CI runners more slack for other reasons (e.g., the exception-construction path), a brief comment would help future readers. Not a blocker.

Overall

The current state is clean, deterministic, and directly tied to observable behavior rather than implementation internals. The test is strictly better than what it replaces. Approved.