Repository navigation
test: prove the extra shots' budget on a mock clock, not the runner's wall time (#720) - #835
Conversation
… wall time (#720) The browser-run budget tests timed captureShots with performance.now() against real timers, so a loaded runner overran them (CI: 768 ms for a 600 ms budget, on a change that touched no browser file). They now run on node:test's mock clock: setTimeout and Date pinned, performance.now (captureShots' monotonic clock) reading the pinned Date, and the clock moved 1 ms at a time once everything due has run. The stand-ins also block the event loop for 40 ms of real time per command, a slow runner the mock clock never sees. The checks are tighter, not looser: the extra shots must take at most their budget (was budget + 250 ms and budget + 120 ms of slack), and the 50 ms-budget test now checks its time too. No src change. Test (fails without): on a simulated slow runner (an --import that blocks the event loop) the old tests fail and the new ones pass; the new ones fail against run.js mutants with the budget's race removed, with the budget checked per view only, and with the pause after the frame wait not cut to what is left. Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
…te.now minus a start (#720) The new mockClock helper measured the extra shots' time as Date.now() - t0. That is the shape test/no-wall-clock-durations.test.ts (#582) counts, so browser-run.test.ts went to 2 against its allowance of 1, and shard 4/4 failed on Node 22 and 24 (run 39328722273). It now reads performance.now(), the clock captureShots itself reads, which the helper already points at the mock clock. The value is the same: every bound stays as tight as it was (<= 1100, <= 600, <= 50), and the allowance is not raised. Test (fails without): no-wall-clock-durations reports "test/browser-run.test.ts: 2, allowed 1" on the previous head and passes on this one. Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
Evaluator verdict: CHANGES NEEDED (round 1/2)Reviewed head f121552 against #720. I also checked e7b2de0, the first head. Base: 6e0d7d8, which is the merge base and current main (measured). Every claim below is labelled measured, inferred or guess. All local runs used a scratch HOME and state dirs, plus Acceptance criteria (#720)
Rule 10: are the bounds weaker?
Fail-withoutSlow runner: a preload that blocks the event loop for B ms every E ms, using real
Run.js mutants, applied by a load hook (each one was confirmed applied). All results measured:
What the new tests prove, and what they no longer prove
Can it hang in CI?
Protection check
Scope, hygiene, CI
Required changes
Not required (named for the record)
|
…per test) (#720) On the mock clock the budget tests advance one mock ms per event-loop turn, so a regression that never settles was stopped only by the 60 000-turn guard, which a starved runner could stretch to hours: the required shards set no --test-timeout. Both tests now carry node:test's per-test { timeout: 60_000 }, which fires on the real clock while t.mock.timers holds setTimeout, and the loop stops once the test's signal is aborted. 60 s is about 30 times the 2 s both took on a runner blocked 200 ms in every 250 ms, and what this file gives its real-Chrome tests. It only adds a failure path; no bound is loosened. Test (fails without): with a run.js stand-in whose extra commands never settle, on a runner blocked 300 ms in every 100 ms, the previous head had finished no test after 150 s; this one stops both with "test timed out after 60000ms". Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
|
Round 2 is pushed: head c461a29. Each required change is addressed:
Re-checked on this head (measured):
Filed from your notes:
|
Evaluator verdict: CHANGES NEEDED (round 2/2), PR body onlyReviewed head c461a29 (round 2) against #720 and my round-1 verdict. Every claim below is labelled measured, inferred or guess. All runs were local, with a scratch HOME and state dirs and The code is done. The one required change is in the PR body: two lines still describe f121552's behaviour at the extreme load as this head's. Once the body is corrected, this is PASS with no further code review needed (inferred: the delta would be text only). Round-1 required changes
The timeout, reproduced with my own stand-in
Rule 10: bounds and allowance
Re-checks on c461a29 (all measured)
Commit and body
CI on c461a29
Required change
For the owner (a trade-off, not a required change)
Host tools in the builder's 75-file run (static read, not rerun)The files selected for this PR are Files that set their own PATH reaching
Files where the system PATH is only a string or a parsed value, and no spawn uses it:
Files that use the inherited PATH for host tools:
Net (inferred):
Issues #848 and #849
|
Intent: the browser-run budget tests never fail because a CI runner is slow, and they still prove that the dark and phone shots share one budget and that every extra command is cut off at what remains. They now run on node:test's mock clock instead of the wall clock. The production cap in
src/browser/run.tsis unchanged, and no src file changes.Risk class: logic (test-only). The only changed path is
test/browser-run.test.ts. It is not in.worklane/review.yaml's engine / engine-guards / release-signing lists and is touched by none of the open PRs checked (#769, #770, #788, #789, #803, #816, #818, #819, #820, #825), measured withgh pr diff --name-only.Closes #720
What changed
mockClock(t)helper.t.mock.timersforsetTimeoutandDate(from 0), and mocksperformance.nowto read the pinned Date.performance.nowis captureShots' monotonic clock.run(work), which moves the clock 1 ms at a time once everything due by then has run (setImmediatestays real). It returns the result and the mock time taken.stall()in the stand-ins. Every stand-in command blocks the event loop for 40 ms of real time (Atomics.wait). This is a slow runner built into the test, as the issue's acceptance asks. The mock clock never sees it.Every changed assertion, and why it is no weaker
took < 1100 + 250(wall clock)took <= 1100(mock clock)lateTook < 600 + 120(wall clock)lateTook <= 600(mock clock)ms <= 50Every other assertion in these tests is unchanged, including views, messages, files, the verdict, the sent commands and their per-command timeout.
git diffshows no otherassertline touched (measured).Scope: what these tests prove, and what they don't
performance.now). The dark and phone views share one budget. Each extra command is raced against what remains and refused once nothing remains. The pause after the frame wait is cut to what remains. On that clock, the total is at most the budget. This is measured with the mutants below.sync-stallmutant (a 300 ms synchronous block per extra view) failed the old tests and passes these (measured by the reviewer).Dateandperformance.nowread the same clock.Round 2: a real-time bound on the budget tests
{ timeout: 60_000 }(BUDGET_TEST_TIMEOUT_MS). It fires on the real clock even whilet.mock.timersholdssetTimeout.t.signalis aborted, so a timed-out test stops moving the mock clock (it exits rather than spinning on).timeout 150/timeout 400:test timed out after 60000ms(60.4 s and 60.4 s; cancelled 2). The run exits 1 after 121 s on its own.On a normal runner the same mutant fails at the turn guard in 0.66 s for the whole run ("the extra shots still ran after a minute of mock time"), as before (measured).
no-race2 fail,per-view-only2 fail,full-pause1 fail (840 ms > 600).affected.mjs --event pull_request, thenshard.mjs s/4for s = 1–4): 76 files. I ran 75 of them;mcp-broker-unitwas excluded because this machine must never run the escape fixture, and CI runs it. Result: 999 tests, 985 pass, 0 fail, 0 cancelled, 14 skipped (the real-Chrome and similar tests, since this machine has no Chrome).Correction (round 1)
This body first said every check was "measured", but the first head (e7b2de0) failed a required check that I had not run.
test/no-wall-clock-durations.test.ts(Clocks: dashboard listing caches use a monotonic injectable clock, so a wall-clock step cannot keep or cut a cache window; lint new Date.now() durations in tests (v0.5.3) #582), withtest/browser-run.test.ts: 2, allowed 1.mockClockhelper measured time asDate.now() - t0, which that lint counts.performance.now(), the clock captureShots reads, which the helper already points at the mock clock. The allowance is not raised, and every bound is unchanged (<= 1100,<= 600,<= 50).no-wall-clock-durationsandno-clock-helpers: 4/4 pass.scripts/ci/affected.mjs --event pull_requestthenscripts/ci/shard.mjs 4/4, 19 files): 217 tests, 217 pass, 0 fail. No mcp-* or plugin-fetch file was in the list.no-race2 fail,per-view-only2 fail,full-pause1 fail.How it was verified
Scratch tools live only in the scratchpad; no repo file was edited to make a mutant.
--importpreload that blocks the event loop for SLOW_BLOCK_MS every SLOW_EVERY_MS of real time.loadhook that rewritesdist/src/browser/run.jsas it is loaded:no-race: each extra command waits for its full answer.per-view-only: extra commands are neither refused nor cut, which is the bug the test's comment describes.full-pause: the pause after the frame wait is not cut to what is left.Run:
node --import ./dist/test/isolate.js [--import <preload>] --test --test-name-pattern budget dist/test/browser-run.test.js, with HOME and the state dirs pointed at a scratch dir.Old tests (origin/main 6e0d7d8), measured:
dark: skipped… / phone: not tried…no-raceper-view-onlyfull-pausethe extra shots took 843 ms, past their 600 ms budget)New tests (this head), measured:
no-raceper-view-onlyfull-pausethe extra shots took 840 ms, past their 600 ms budgetdist/test/browser-run.test.js: 17 tests, 12 pass, 0 fail, 5 skipped. The 5 skips are the real-Chrome tests, because this machine has no Chrome (measured).test/no-clock-helpers(the Tests: no result depends on today's date; five intake tests go red on main ~2026-11-02 (fix before 2026-11-01) #687 lint on clock-reading helpers): 2/2 pass (measured).npm run typecheck: exit 0 (measured).npm run check:denylist:denylist: 1309 files clean(measured).npm pack --dry-run --ignore-scriptslists notest/file except examples/basic/test/price.test.js, and the packagefilesfield ships only dist/src, dist/web, templates, examples/basic and the notices.Adversarial pass
performance.now()and armssetTimeout, and both are what the mock drives, so the production code path runs unmodified (inferred). All three mutants of that code fail the new tests (measured).<= budget(measured: the mutants fail).--test-timeout". That was false. The required shards set no--test-timeoutand no jobtimeout-minutes(measured by the round-1 review); only the weekly clock-shift job sets one.{ timeout: 60_000 }, added in round 2 (measured, see "Round 2").t.mockis restored when each test ends (node:test documents this). The rest of the file passes in the same process (measured: 12/12 non-Chrome tests).<=budget, no slack); no assertion was removed; the production cap is untouched.src/browser/run.ts, which is by design and unchanged. This PR only makes its test deterministic; it adds no lint.New dependencies
None.