Skip to content

Unify time mocking strategy across the codebase #306

@emlautarom1

Description

@emlautarom1

Context

Time mocking has been flagged as a recurring pain point across multiple PRs:

  • PR feat: add qbft module #18 (QBFT): Tests were flaky due to reliance on real wall-clock time. cargo nextest was introduced specifically for timeout/retry control. A custom FakeClock was built for QBFT to work around the problem. Several tests remain #[ignore]d with timing-related failures ("wrong decide round", "wrong prepared value").
  • PR feat: add deadline #276 (Deadliner): Utc::now() is called directly throughout deadline.rs, making the module untestable with tokio::time::pause()/advance(). Tests rely on real wall-clock time with short millisecond sleeps — fragile under CI load. The PR was merged with the agreement to revisit this as a separate issue.

Current state

The codebase has three unrelated time approaches coexisting:

Approach Where Mockable?
chrono::Utc::now() / chrono::Local::now() deadline.rs, retry.rs, peerinfo, cluster/definition.rs, p2p/k1.rs No (not controlled by tokio test-util)
std::time::Instant / tokio::time::Instant peerinfo/protocol.rs, p2p/relay.rs, QBFT tests Partially (tokio variant responds to pause()/advance())
FakeClock (custom, crossbeam-based) core/qbft/fake_clock.rs Yes, but ad-hoc and only for QBFT

Additionally, some modules use injectable time functions as a workaround:

  • retry.rs has with_time() accepting a Fn() -> DateTime<Utc> provider
  • deadline.rs has DeadlineFunc for injectable deadline calculation

These are local solutions — there is no unified abstraction.

Problem

  1. chrono::Utc::now() is invisible to tokio's test-util. tokio::time::pause() only controls tokio::time::Instant and tokio::time::sleep(). Any code using chrono::Utc::now() will see real wall-clock time in tests, leading to flaky or non-deterministic behavior.
  2. FakeClock is QBFT-specific. It uses crossbeam::channel and std::time::Instant — a completely different mechanism from tokio's async timers. It can't be reused for async code that uses tokio::select!.
  3. No consistent pattern for new code to follow. Each module has invented its own approach.

Proposed direction

Explore a unified time abstraction, potentially:

  • Option A: Lean into tokio::time throughout. Replace chrono::Utc::now() with tokio::time::Instant-based calculations where possible. Use tokio::time::pause()/advance() in tests. Keep chrono only for formatting/parsing, not as a clock source.
  • Option B: Create a small Clock trait (or crate-level abstraction as suggested in feat: add deadline #276) that wraps the time source. Production uses real time; tests inject a controllable clock. This is more flexible but adds a layer of indirection.
  • Option C: Hybrid — use tokio's built-in mocking for async timer code, and a simple injectable Fn() -> DateTime<Utc> for code that needs wall-clock timestamps.

Affected areas

Key files using chrono::Utc::now() or direct time that would benefit from unification:

  • crates/core/src/deadline.rs
  • crates/app/src/retry.rs
  • crates/peerinfo/src/protocol.rs, config.rs, behaviour.rs
  • crates/cluster/src/definition.rs
  • crates/p2p/src/k1.rs
  • crates/core/src/qbft/fake_clock.rs (custom mock)
  • crates/app/src/privkeylock.rs

References

Metadata

Metadata

Assignees

No one assigned

    Labels

    testingCreate, improve or modify tests

    Type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions