Conversation
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>
|
Outcome: the quest is done in this PR. All 630 Open decisions for the maintainer:
(Written by Claude Opus 5.5) |
…ir tests synchronous Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 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".
| impl<T> Drop for JoinHandle<T> { | ||
| fn drop(&mut self) { | ||
| self.join.borrow_mut().detached = true; | ||
| } |
There was a problem hiding this comment.
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 👍 / 👎.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
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 inDeadline::new,set,pollandClock::nowthat swapped in a tokio sleep.model::clocktest clock: a frozen thread-localnow(),advance(), and a registry thatPool::newjoined under#[cfg(test)]so advancing dated every pool on the thread.Approach
moq-net-sim(new,publish = false, atrs/moq-net/sim): a single-threaded executor with simulated time, about 490 lines (half of them docs) onfutures::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 offersrun,spawn/JoinHandle,sleep,timeout,advance,yield_nowandnow, plus two hooks for caller-driven time:drive(poll)polls atime::Driverwith the simulated instant.attach(source)keeps atime::Clockin step with simulated time.#[moq_net_sim::test](a proc macro inmoq-net-sim-macros, with no dependencies) runs anasync fnon it. Swapping it for#[tokio::test]is a one-line change per test..awaitand 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!,oneshotandmpscmap tofutures/std. No assertion changed.Clock::tokioandTokioSleepare gone, and so is every#[cfg(test)]branch inClock/Deadline. Tests build aClock::sim()that the executor advances throughattach. 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 usedmodel::clock::advance(d)now callpool.step(d)on the pool they test.stepis a#[cfg(test)]helper onPoolthat dates accesses without collecting, which is exactly what the registry did.tests/support/harness.rsspawns drivers withmoq_net_sim::drive. The session bench shares that file and still runs on tokio, soharness::spawn/nowpick tokio when no sim run is active.macros,io-utilandsyncare dropped, so a new#[tokio::test]in moq-net no longer compiles.rs/moq-net/AGENTS.mdsays that moq-net tests use plain#[test]with explicit instants, or#[moq_net_sim::test], and never tokio or the wall clock.rs/AGENTS.mdkeepstokio::time::pause()for the other crates and points to it. The user approved both edits.quest/m1/rs2ts/README.mdandlite.md. The user had moved it undersans-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 aTimestamp::now()value beyond!is_zero().Impact
#[cfg(test)], and the new crates are unpublished path-only dev-dependencies, whichcargo publishstrips.#[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
moq-net-simcrate, tokio-shaped API,#[moq_net_sim::test](this PR)#[cfg(test)] modinside moq-net,#[path]-included bytests/supportcrate::paths, so it needs#[path]tricks. Still needs a proc-macro crate or a body re-indent (~20k-line diff).kiobehind a featurepoll_*loop, with no executorRecommendation: the private crate. The async bodies left over belong to the sessions that
sans-io/lite.mdandsans-io/ietf.mdrewrite, and their tests become synchronous with them.How tests control time (evaluated at the user's request)
nowtoDriver::poll, and tests step the one pool they hold (pool.step(d)). No global.driver.poll(now, waiter). The bun harness keeps its own fakenow, and there is no module state to reset between tests.Clock::now()/Clock::set()behindTimestamp::now())bun testshares module state within a file, so every test needsbeforeEachresets.std::thread, a multi-thread runtime).cfg(test)hides it, and then generated code has no setter at all.model::clockregistry#[cfg(test)]frozen clock plus a thread-local list of every pool, advanced together.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
nowper poll and pools take it pergc.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-netwall 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.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_attributein 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.Decisions (by the user)
moq-net-simcrate pair, as shipped.rs/AGENTS.mdandrs/moq-net/AGENTS.mdupdated in this PR.poll_*tests as the sans-IO lite and IETF sessions are rewritten. This is recorded in their quest plans.Follow-ups
asyncfeature withsans-io/async-feature.md.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code