Sign up
View stargazers13Sign up

test(server): ban real-clock waits in tests#22564

merged3
tests/no-wall-clock
tests/no-real-time-wait
opened3d agobyJJulius Marminge
18 files changed
View on GitHubTwo-way sync enabledPR last imported 3d ago

Problem

it.effect runs on a TestClock, so Effect.sleep there only advances via TestClock.adjust; under it.live or plain it it is a real wait. Tests mixed the two, polled projections with 5 ms sleeps, and asserted "nothing happened" by sleeping 200 ms. These are the tunnel, AcpAdapterV2, OrchestratorMcpToolkit and PullRequestReviewGuideService flakes.

Fix

  • New rule t3code/no-real-time-wait-in-tests (non-browser test files): flags Effect.sleep/delay/Schedule.spaced under a real-clock tester, Effect.sleep under it.effect in a file that never adjusts the TestClock, and timer-in-Promise waits. Effect.timeout* safety nets are not flagged.
  • 31 files rewritten to await the event that marks the milestone (Deferred, latest projection sequence, a tracer span end, TestClock adjust). 7 suppressions outside live tests, each on a real-OS wait with no awaitable signal (pid files, process exit, a macrotask boundary); 7 in env-gated live probes.

Part of the anti-flake stack (#22563). Context: main was red in ~600 of ~2,900 runs since September; the genuinely flaky tests all waited on real time, scheduler budgets, or renderer frames. These rules make those tests fail lint instead of CI.

Made with Claude Opus 5.5 and Claude Fable 5.1 in Claude Code (T3 Code).

🤖 Generated with Claude Code

Checks

All checks passed
oxlint-plugin-t3code/rules/no-real-time-wait-in-tests.tsline 181Outdated · f45927fOpen thread
……@@ -0,0 +1,264 @@
178+ },
179+ create(context) {
180+ if (!TEST_FILE_PATTERN.test(context.filename)) return {};
181+ const drivesTestClock = TEST_CLOCK_DRIVER_PATTERN.test(context.sourceCode.text);
MmacroscopeappBoton line 1813d ago

🟢 Low rules/no-real-time-wait-in-tests.ts:181

A comment or string containing TestClock.adjust or TestClock.setTime sets drivesTestClock to true, suppressing the undriven-sleep diagnostic even when the test never drives the clock. Derive this flag from actual call expressions so documentation and fixture text cannot disable the check.

No longer relevant as of e90b635be35e5ff8b2814ec4ca8304f44bb44b93

JJulius Marminge3d ago

Fixed: drivesTestClock now comes from actual TestClock.adjust/TestClock.setTime call expressions, with the dependent reports deferred to Program:exit, and a test covers a comment-only mention.

MmacroscopeappBot3d ago

I still see drivesTestClock derived from context.sourceCode.text via the regex at line 181, so comment/string mentions can still suppress the diagnostic.

JJulius Marminge3d ago

Same: on 978d35a drivesTestClock is set only by TestClock.adjust/setTime CallExpressions found in the AST; the source-text regex is gone. Rule test "reports TestClock sleeps when TestClock.adjust appears only in a comment" covers it.

oxlint-plugin-t3code/rules/no-real-time-wait-in-tests.tslines 230–232Outdated · f45927fOpen thread
……@@ -0,0 +1,264 @@
229+ const tester = enclosingTester(node);
230+ if (Option.isNone(tester)) return;
231+ reportFor(
232+ tester.value === "effect"
MmacroscopeappBoton lines 230–2323d ago

🟠 High rules/no-real-time-wait-in-tests.ts:230

A timer hidden in a shared helper produces no diagnostic even when a test invokes that helper, so extracting setTimeout into sleep or waitUntil bypasses this rule. enclosingTester only finds lexical test callbacks, and this early return drops the timer call when none is found; report such timer waits as real-clock waits.

No longer relevant as of e90b635be35e5ff8b2814ec4ca8304f44bb44b93

JJulius Marminge3d ago

Fixed: Promise-wrapped and timers/promises timers are now reported at module level in test files too, and the existing setImmediate macrotask helpers (plus one subprocess timeout safety net) carry a reasoned disable comment.

MmacroscopeappBot3d ago

I still see the Option.isNone(tester) early return at line 230, so module-level/shared timer calls remain unreported.

JJulius Marminge3d ago

That re-review ran on 2551143, an intermediate head from the #22557 rebase. On the current head 978d35a a timer inside a new Promise executor or a timers/promises call at module level is reported (rule test "reports timers hidden in module-level helpers"); the Option.isNone(tester) branch now only defers an Effect.sleep helper until the file is known not to drive the TestClock.

Ggithub-actionsBot3d ago
Synced from GitHub

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

ℹ️ No successful main baseline artifact is available yet. This run establishes the initial measurement.

ProviderMetricMain baselineThis PRImpactPR ceiling
CodexTotal thread wire—5.0 KiB—6.8 KiB✅
CodexThread snapshot wire—3.8 KiB—4.9 KiB✅
CodexLive turn WebSocket wire—1.2 KiB—2.0 KiB✅
CodexLive turn WebSocket decoded—20.9 KiB—29.3 KiB✅
CodexLive turn messages—2—8✅
ClaudeTotal thread wire—5.0 KiB—6.8 KiB✅
ClaudeThread snapshot wire—3.8 KiB—4.9 KiB✅
ClaudeLive turn WebSocket wire—1.2 KiB—2.0 KiB✅
ClaudeLive turn WebSocket decoded—21.2 KiB—29.3 KiB✅
ClaudeLive turn messages—2—8✅

Baseline: unavailable · PR result: a9550e8 · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 108.5 KiB
  • Claude decoded thread snapshot: 108.8 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

MmacroscopeappBot3d ago
Synced from GitHub

Approvability

Verdict: Needs Human Approval

Macroscope's review found this PR requires human approval — This is primarily an anti-flake test refactor plus a substantial new oxlint rule, with no customer request-path behavior changes. It also changes lint enforcement and adds browser-test and line-level diagnostic exceptions, so the repository policy impact warrants review.

Notes:

  • All code in this push has already been reviewed. Approvability was decided on eligibility alone.

No code changes detected at a9550e8. Prior analysis still applies.

You can add or adjust custom eligibility rules. Learn more.

CcoderabbitaiBot3d ago
Synced from GitHub

Review in Change Stack →

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Team
  • Run ID: 5dc05cda-9dc7-4e07-93e0-3ad92064c810
📥 Commits

Reviewing files that changed from the base of the PR and between 77530729820bd594c49e8d3aa1e60469daee00d4 and a9550e8cf64dfa10a839f19573a6384017669bbe.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Team
  • Run ID: bbc5615e-6543-4229-843d-b2e24ff52577
📥 Commits

Reviewing files that changed from the base of the PR and between e90b635be35e5ff8b2814ec4ca8304f44bb44b93 and 978d35ae737aae906f80a37c15266bef6dce69d2.

📒 Files selected for processing: 6
  • apps/mobile/src/features/agent-awareness/remoteRegistration.test.ts
  • apps/server/src/preview/ServerBrowser.test.ts
  • apps/server/src/pullRequest/PullRequestReviewGuideService.test.ts
  • apps/server/src/terminal/Manager.test.ts
  • packages/provider-opencode/src/server/status.test.ts
  • packages/provider-pi/src/server/status.test.ts

Limit details: You’ve used all 10 included reviews currently available.

📝 Walkthrough

Adds and enables an Oxlint rule that detects real-time waits in test files. Tests replace timing-based polling with explicit signals or event-driven checks. Selected macrotask boundaries remain with targeted suppressions.

Changes: Test wait detection and synchronization

Layer / File(s)Summary
Add and enable real-time wait detection
oxlint-plugin-t3code/rules/no-real-time-wait-in-tests.*, oxlint-plugin-t3code/index.ts, vite.config.ts		Adds the rule, comprehensive rule tests, plugin registration, default enforcement, and a browser-test override.
Replace server polling with projection events
apps/server/src/orchestration-v2/...		Both waitForIdle helpers inspect the current projection and then reread it after event-sink events. They fail if the stream ends before idle state appears.
Use explicit signals for asynchronous tests
apps/desktop/..., apps/server/src/preview/..., apps/server/src/pullRequest/..., apps/server/src/terminal/..., apps/mobile/...		Tests replace scheduling delays and polling with ceremony, CDP, registration, tracer, queue, deferred, or event signals.
Retain intentional macrotask boundaries
apps/desktop/src/preview/CdpRelay.test.ts, apps/server/src/preview/ServerBrowserPage.test.ts, packages/shared/src/*.test.ts		Intentional waits retain targeted suppressions or explicit macrotask turns.
Remove artificial provider startup delays
packages/provider-opencode/src/server/status.test.ts, packages/provider-pi/src/server/status.test.ts		Provider test doubles no longer delay successful version responses or discovery startup.

Priority: ⬇️ Low

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Feature

Suggested reviewers: t3dotgg

Merge Risk: ⚪ Minimal · up to 978d3

No merge-blocking issue was identified in the changed test synchronization paths.

Pre-merge checks: Passed 3 · Failed 1

Failed Failed checks: 1 warning

Check nameStatusExplanationResolution
Description checkWarningThe description explains the problem and proposed fix, but it omits the required Scope and approval and Verification sections. It also uses "Fix" instead of the template's "Change" heading.Add a Scope and approval section with the related issue or explicit maintainer approval, or explain why the focused change qualifies for an exemption. Add a Verification section with the focused tests or checks run, observed results, and an…
Passed Passed checks: 3 passed
Check nameStatusExplanation
Title checkPassedThe title clearly and concisely describes the main change: banning real-clock waits in tests.
Linked Issues checkPassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes checkPassedCheck skipped because no linked issues were found for this pull request.
Full details: Description check

Resolution

Add a Scope and approval section with the related issue or explicit maintainer approval, or explain why the focused change qualifies for an exemption. Add a Verification section with the focused tests or checks run, observed results, and any limitations. Rename "Fix" to "Change" to match the repository template.

✨ Finishing Touches: 💡 1
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • Create a new PR
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

Added 3 commits

  • test(lint): no-real-time-wait catches helper timers and reads TestClock use from the AST0a4c565
  • test(server): ban real-clock waits in tests6767784
  • test: restore the real-clock wait rewrites dropped by the #22557 rebasea9550e8