Skip to content

test(net): run moq-net's tests on a simulated clock instead of tokio - #4466

Open
kixelated wants to merge 8 commits into
quest/m1/rs2ts/sans-io/READMEfrom
quest/m1/rs2ts/mock-clock
Open

kixelated wants to merge 8 commits into
quest/m1/rs2ts/sans-io/READMEfrom
quest/m1/rs2ts/mock-clock

Conversation

@kixelated

@kixelated kixelated commented Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator

Stacks on #4458 (quest/m1/rs2ts/sans-io/model). Its diff shows here until #4458 merges.

Problem

quest/m1/rs2ts/mock-clock.md: moq-net's tests ran on tokio. 630 #[tokio::test]s, 96 of them paused, used tokio only for its advanceable clock. That kept two test-only time hooks in production code:

  • time::Clock::tokio, plus #[cfg(test)] branches in Deadline::new, set, poll and Clock::now that swapped in a tokio sleep.
  • The model::clock test clock: a frozen thread-local now(), advance(), and a registry that Pool::new joined under #[cfg(test)] so advancing dated every pool on the thread.

Approach

  • moq-net-sim (new, publish = false, at rs/moq-net/sim): a single-threaded executor with simulated time, about 490 lines (half of them docs) on futures::executor::LocalPool. Time only moves once every task stalls. It then jumps to the earliest armed timer, the same as tokio's paused clock. A run with every task parked and nothing armed panics as a deadlock instead of hanging. It offers run, spawn/JoinHandle, sleep, timeout, advance, yield_now and now, plus two hooks for caller-driven time:
    • drive(poll) polls a time::Driver with the simulated instant.
    • attach(source) keeps a time::Clock in step with simulated time.
  • #[moq_net_sim::test] (a proc macro in moq-net-sim-macros, with no dependencies) runs an async fn on it. Swapping it for #[tokio::test] is a one-line change per test.
  • Mechanical port. 233 tests had no .await and are now plain #[test] fn. The other 397 use #[moq_net_sim::test]. tokio::time::*, tokio::spawn, tokio::task::* map one-to-one onto the sim functions. tokio::join!, select!, pin!, oneshot and mpsc map to futures/std. No assertion changed.
  • Hooks retired. Clock::tokio and TokioSleep are gone, and so is every #[cfg(test)] branch in Clock/Deadline. Tests build a Clock::sim() that the executor advances through attach. The model's test clock and pool registry are gone too. model::clock::now() is now the real clock in every build. The ~40 cache-expiry tests that used model::clock::advance(d) now call pool.step(d) on the pool they test. step is a #[cfg(test)] helper on Pool that dates accesses without collecting, which is exactly what the registry did.
  • Integration tests. tests/support/harness.rs spawns drivers with moq_net_sim::drive. The session bench shares that file and still runs on tokio, so harness::spawn/now pick tokio when no sim run is active.
  • moq-net's tokio dev-dependency is only for the benches now. macros, io-util and sync are dropped, so a new #[tokio::test] in moq-net no longer compiles.
  • Guidance. rs/moq-net/AGENTS.md says that moq-net tests use plain #[test] with explicit instants, or #[moq_net_sim::test], and never tokio or the wall clock. rs/AGENTS.md keeps tokio::time::pause() for the other crates and points to it. The user approved both edits.
  • Quest. The quest is deleted and its references are removed from quest/m1/rs2ts/README.md and lite.md. The user had moved it under sans-io/ first, so the net effect is the same.

The Sans-IO lite and IETF quests each gained a Plan line: as each session is rewritten, turn its async test bodies into synchronous poll_* tests with explicit instants.

The Timestamp anchor and Timestamp::now() are unchanged, per the user's decision. No test reads a Timestamp::now() value beyond !is_zero().

Impact

  • Public API: none. Everything changed is #[cfg(test)], and the new crates are unpublished path-only dev-dependencies, which cargo publish strips.
  • Wire: none.
  • Production code: unchanged in a non-test build. The removed lines were all #[cfg(test)].

Public API tradeoffs

Nothing is exported. The shape still matters because rs2ts and the TypeScript tests will copy it.

Executor and harness shape

Option Pros Cons
Private moq-net-sim crate, tokio-shaped API, #[moq_net_sim::test] (this PR) One-line port per test. Shared by unit tests, integration tests and the bench harness. No moq-net dependency, so no dev-dep cycle. A deadlock panics instead of hanging. Two new workspace crates (the proc macro must be its own crate). Async test bodies remain async, so they translate only after the sans-IO sessions land.
#[cfg(test)] mod inside moq-net, #[path]-included by tests/support No new crates. Integration tests can't reach crate:: paths, so it needs #[path] tricks. Still needs a proc-macro crate or a body re-indent (~20k-line diff).
Put the executor in kio behind a feature Other kio users could use it. Adds public API to a published crate. Nobody else needs it yet.
Rewrite every test as a synchronous poll_* loop, with no executor Every test translates directly. Not mechanical: 397 bodies change shape, and assertions move with them. Most of these tests exercise async internals (IETF/lite sessions) that the sans-IO quests are about to replace anyway.

Recommendation: the private crate. The async bodies left over belong to the sessions that sans-io/lite.md and sans-io/ietf.md rewrite, and their tests become synchronous with them.

How tests control time (evaluated at the user's request)

Option What it looks like Parallel tests Can production misuse it Generated TypeScript
(a) Explicit instants (this PR) The harness passes now to Driver::poll, and tests step the one pool they hold (pool.step(d)). No global. Isolated by construction. No: there is nothing to set. Identical: driver.poll(now, waiter). The bun harness keeps its own fake now, and there is no module state to reset between tests.
(b) Global settable clock (Clock::now() / Clock::set() behind Timestamp::now()) One call moves every driver and pool. Tests share a process-wide clock, so any parallel test that sets it corrupts the others. The test runner has to serialize. Yes. It is a public mutator, so a library or app can skew every timestamp in the process. A mutable module-level singleton. bun test shares module state within a file, so every test needs beforeEach resets.
(c) Thread-local or per-test scoped (b) Same, with a guard that restores the clock on drop. Safe across Rust test threads, but broken by any code that hops threads (a std::thread, a multi-thread runtime). Less exposed, but still a public setter unless cfg(test) hides it, and then generated code has no setter at all. A thread-local becomes a plain module global in single-threaded JS. That is (b) again, with scoping enforced only by convention.
(d) The retired model::clock registry #[cfg(test)] frozen clock plus a thread-local list of every pool, advanced together. Thread-isolated. No, it is test-only. Untranslatable. rs2ts reads the non-test build, so the TS tests would have no way to move time or date pools. That is why it had to go.

Recommendation: (a). It is the only option that works the same on tokio, on the sim executor and in generated TypeScript without module state. It adds no public API. It is already what production does: drivers take now per poll and pools take it per gc. Timestamp::now() stays a public convenience that reads the real clock. Nothing in the tests depends on its value, so it needs no mock. (b) and (c) were not prototyped because (a) wins without them.

Benchmarks

cargo test -p moq-net wall time, prebuilt binaries, 5 interleaved rounds per side (base = quest/m1/rs2ts/sans-io/model). The machine was shared, at load average 3.6-4.4 on 32 cores.

Base Branch
Median total (incl. doctests) 5.44 s 5.40 s
Range 5.40-6.37 s 5.26-5.54 s
Lib unit-test binary 1.36 s 1.34 s

The run time is unchanged. Doctests dominate the total, and the paused tokio tests already ran on virtual time. The gain is determinism: the ~500 previously unpaused tests no longer use the wall clock for their timeouts and settles. No Criterion run was needed, since production code is unchanged in a non-test build. The session bench's harness change only touches driver setup.

Alternatives

  • macro_rules_attribute in place of the proc-macro crate. It is one external dependency, but every call site reads #[apply(moq_net_sim::test!)], and it still needs a macro somewhere.
  • Moving the session bench onto the sim executor, which would make the harness's tokio fallback unnecessary. Rejected because it changes what the bench measures, which would break the before and after comparison.

Decisions (by the user)

  1. Executor: the private moq-net-sim crate pair, as shipped.
  2. Time control: (a) explicit instants, with no global clock.
  3. Test guidance: rs/AGENTS.md and rs/moq-net/AGENTS.md updated in this PR.
  4. Async test bodies: become synchronous poll_* tests as the sans-IO lite and IETF sessions are rewritten. This is recorded in their quest plans.

Follow-ups

  • The async-helper tests move behind the async feature with sans-io/async-feature.md.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

kixelated and others added 7 commits September 28, 2026 19:08
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Drivers own a time::Clock advanced by the instants they are polled with, and
time::Deadline arms against it directly. The Timers/Timer traits, the unused
virtual runtime::Test, and runtime.rs are gone.

model/clock.rs is now the model's only local clock read: Timestamp::now and
the anchor that maps an Instant onto a Timestamp, with one conversion for
native and the browser instead of two.

Deletes the Sans-IO model quest.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…racks

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Outcome: the quest is done in this PR. All 630 #[tokio::test]s now run on moq-net-sim (233 as plain #[test], 397 as #[moq_net_sim::test]) with their assertions unchanged. time::Clock::tokio and the model::clock registry are removed. just check and just test interop pass. The PR stays a draft until #4458 merges.

Open decisions for the maintainer:

  1. Where the executor lives. Recommended: keep the private rs/moq-net/sim crate plus its macro crate. The alternatives are an in-crate #[path] module or a kio feature.
  2. How tests control time. Recommended: (a) explicit instants, as shipped. A global or thread-local settable clock is not recommended; see the tradeoffs table.
  3. rs/AGENTS.md still tells agents to use tokio::time::pause(). Recommended: point moq-net tests at #[moq_net_sim::test] in a prompted AGENTS.md edit.

(Written by Claude Opus 5.5)

…ir tests synchronous

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated
kixelated marked this pull request as ready for review September 29, 2026 16:33

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e550d138d2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread rs/moq-net/sim/src/lib.rs
Comment on lines +424 to +427
impl<T> Drop for JoinHandle<T> {
fn drop(&mut self) {
self.join.borrow_mut().detached = true;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Re-raise completed task panics when dropping the handle

When a spawned future panics before its still-owned JoinHandle is dropped, finish stores the panic in join.output, but this Drop implementation only sets detached; the task has already finished, so it never re-enters finish to propagate the panic. Consequently, tests that retain a background handle until the end, such as the _driver handles in the session tests, can complete successfully despite that task panicking, contrary to spawn's documented guarantee that dropping the handle propagates the panic. Inspect and re-raise a stored panic here so background failures cannot be silently ignored.

AGENTS.md reference: AGENTS.md:L17-L18

Useful? React with 👍 / 👎.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-29T16:37:25.522479Z e550d13 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant