Conversation
Add focused boundary and state-transition tests for clock, delay, periodic timer, fast instant, and timer internals. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds targeted tests across the tick crate to increase mutation coverage by exercising boundary conditions and state transitions in timer driving, clock configuration, and internal time sources.
Changes:
- Added new unit tests for timer ordering/discriminator behavior and timer-advancement edge cases.
- Added tests for clock-control auto-advance behavior, delay overflow normalization, and periodic timer exact-deadline firing.
- Introduced a small
#[cfg(test)]introspection helper onSimpleClockto validate fast-instant configuration in tests.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/tick/src/timers.rs | Adds tests for discriminator sequencing and a 1ns boundary in advance_timers. |
| crates/tick/src/state.rs | Adds a clone/uniqueness test for shared timer storage. |
| crates/tick/src/simple_clock.rs | Adds a test-only helper to check whether fast instant mode is active. |
| crates/tick/src/periodic_timer.rs | Adds a test asserting firing at an exact deadline when driven by ClockControl. |
| crates/tick/src/fast_instant.rs | Strengthens the platform clock read test (but currently in a potentially flaky way). |
| crates/tick/src/delay.rs | Adds a test intended to cover overflow behavior in timer registration. |
| crates/tick/src/clock.rs | Extends fast-instant configuration test assertions via SimpleClock helper. |
| crates/tick/src/clock_control.rs | Adds tests for auto-advance-timers evaluation and limit consumption. |
Suppressed comments (1)
crates/tick/src/timers.rs:175
- This test reaches into
timers.wakersand constructs aTimerKeywithdiscriminator = 0, which is not reachable viaTimers::register(it starts at 1). Using the public registration path keeps the test aligned with real behavior and reduces coupling to internals.
let next = now + Duration::from_nanos(1);
let key = TimerKey::new(next, 0);
timers.wakers.insert(key, Waker::noop().clone());
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #695 +/- ##
=======================================
Coverage 100.0% 100.0%
=======================================
Files 559 559
Lines 60774 60777 +3
=======================================
+ Hits 60774 60777 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
crates/tick/src/timers.rs:165
- This test hard-codes the initial discriminator values (1 and 2), which makes it brittle to internal changes that don’t affect the intended property (that discriminators progress sequentially). Prefer asserting the relationship between the two keys (wrapping increment) rather than exact starting values.
let first = timers.register(when, Waker::noop().clone());
let second = timers.register(when, Waker::noop().clone());
assert_eq!(first.discriminator, 1);
assert_eq!(second.discriminator, 2);
crates/tick/src/fast_instant.rs:126
platform_time()is not guaranteed to be non-zero (e.g., very early after boot it can legitimately return 0), so asserting!= Duration::ZEROcan be flaky. Consider waiting briefly and retrying a bounded number of times, then asserting the value became non-zero to keep the mutant-killing intent without relying on a single sample.
fn platform_time_can_be_read() {
assert_ne!(platform_time(), Duration::ZERO);
| let now = Instant::now(); | ||
| let next = now + Duration::from_nanos(1); | ||
| let key = TimerKey::new(next, 0); | ||
| timers.wakers.insert(key, Waker::noop().clone()); | ||
|
|
| #[test] | ||
| fn overflowing_reregistration_clears_existing_timer() { | ||
| let clock = Clock::new_system_frozen(); |
martintmk
left a comment
martintmk
left a comment
There was a problem hiding this comment.
Posted by an AI agent
Reviewed the complete pinned test-only change at 81f48351dede70d47962f3d59a36497ae99364b6 across all ten required areas. I found two new non-blocking gaps: the manual-advance test codifies behavior that conflicts with the public duration contract, and the ready-timer test does not verify that the task is actually woken. Existing discussions about discriminator coupling, synthetic delay re-registration, platform-time flakiness, and the delay-test name remain applicable and were not duplicated.
The targeted tick suite passed. A non-required Linux ARM pr-fast check remains red, but its log exposes no diagnostic beyond the failed action step.
| }; | ||
| let start = state.instant; | ||
| state.timers.register(start + Duration::from_secs(2), Waker::noop().clone()); | ||
|
|
There was a problem hiding this comment.
Posted by an AI agent · Non-blocking
ClockControl::advance can exceed its documented duration
Problem
The public docs say manual advancement moves by the specified duration and fires timers within that period, but this test codifies a one-second advance jumping two seconds when auto-advance timers are enabled.
Why this matters
Callers cannot tell whether manual advancement is bounded, and the test makes the undocumented overshoot part of the expected behavior.
Suggested fix
Clarify the interaction in the public method documentation, or change the behavior and test so manual advancement remains bounded.
| state.timers.register(state.instant + Duration::from_secs(1), Waker::noop().clone()); | ||
|
|
||
| state.advance_time(Duration::from_secs(1), TimeFlow::Forward); | ||
|
|
There was a problem hiding this comment.
Posted by an AI agent · Non-blocking
advance_time_fires_ready_timers does not verify task notification
Problem
The test registers Waker::noop() and only asserts that the timer entry is removed. It still passes if ready timers are deleted without waking their tasks.
Why this matters
Timer bookkeeping could report completion while the awaiting task remains permanently pending.
Suggested fix
Use a counting Wake implementation and assert one wake at the deadline and none before it.
martintmk
left a comment
martintmk
left a comment
There was a problem hiding this comment.
Posted by an AI agent
Reviewed the complete pinned change with all ten Review Lens areas and three independent correctness reviewers. I found two non-blocking test/documentation issues; the shipping public API and rustdoc surface are unchanged. Focused validation passed: cargo check -p tick --all-features and the specialist tick test runs.
| assert!(delay.current_timer.is_some()); | ||
|
|
||
| delay.duration = Duration::MAX; | ||
| assert_eq!(delay.register_timer(waker), Poll::Pending); |
There was a problem hiding this comment.
Posted by an AI agent · Non-blocking
Overflow re-registration leaves the prior timer registered
Problem
overflowing_reregistration_clears_existing_timer verifies only that current_timer becomes None. The exercised overflow branch drops the existing key without calling unregister_timer, so the clock still retains the timer and Drop can no longer remove it.
Why this matters
The test currently blesses an internally inconsistent state and would not protect a future re-arm/reset path from leaking a timer registration.
Suggested fix
Take and unregister the existing key before clearing the registration, then assert that the clock's timer count returns to zero.
| } | ||
|
|
||
| #[test] | ||
| fn auto_advanced_timer_consumes_limit() { |
There was a problem hiding this comment.
Posted by an AI agent · Non-blocking
auto_advance_limit docs exclude timer-driven advancement
Problem
This test establishes that timer-driven fast-forwarding consumes the same limit, but the public auto_advance_limit docs say the limit only has an effect after auto_advance is configured with a non-zero duration.
Why this matters
Users of auto_advance_timers can incorrectly conclude that the limit cannot bound timer-driven advancement, so the documented configuration contract disagrees with the covered behavior.
Suggested fix
Document that the limit caps advancement from both auto_advance and auto_advance_timers, and state that at least one automatic advancement mode must be enabled.
Add focused boundary and state-transition tests for clock, delay, periodic timer, fast instant, and timer internals.