Repository navigation
test(console): make the utc-offset log timestamp test fast again - #2841
Conversation
The UTC-offset timestamp test grew from 5 to 10 locale cases in #2819 and read the pane with seven browser round trips per width (220 widths), each case launching its own Chromium and signing in again: 4.4 s became 11.0 s in CI. It now shares one Chromium across the cases, reuses the first case's session, reads all per-width measurements in one evaluate, and gives each case its own Agent so the per-principal runtime-log rate limit (burst 10, 2/s) does not throttle the faster cases. Same locales, widths and assertions. Local p50 9.40 s -> 5.07 s (20 runs each, 2 CPUs).
… cell is absent Review nit: a missing row failed with a bare TypeError from the in-page read.
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
Median of the five green main runs ending at bef197b (82.3 s), less the 4.1 s this branch took off the UTC-offset test on its own CI (10.96 s main median to 6.88 s): 78.2 s.
GitHub Actions
Inactive
GitHub Actions
Failure
GitHub Actions
Inactive
clawsweeper
Bot
added
P3
|
Codex review: needs maintainer review before merge. What this changesThis PR shares Chromium and authentication across timestamp cases, batches layout measurements, and updates the CI duration estimate. Example: Run the timestamp layout test across 10 locale/time-zone cases and 22 viewport widths.
Review scores
ProductKind: Test only · Worth it: Yes · Fix scope: Complete Merge readiness✅ Ready for maintainer review Keep open: this PR is a useful, focused test optimization with no actionable findings; repository policy reserves closure for maintainer triage. Priority: P3 Before mergeNone. FindingsNone. Agent review detailsHow this fits togetherThe browser suite renders the production console against the controller application with an in-memory Compute Driver, reads timestamp geometry, and reports assertions to CI; the duration table controls shared-file scheduling. flowchart TD
A[CI browser lane] --> B[Timestamp test]
B --> C[Shared Chromium]
C --> D[Isolated locale contexts]
E[Controller fixture] --> F[Console log view]
D --> F
F --> G[Layout assertions]
Technical reviewBest possible solution: Retain the existing browser helper and complete timestamp matrix while collecting each width’s geometry in one synchronous snapshot. Do we have a high-confidence way to reproduce the issue? Source confirms repeated browser launches, sign-ins, and seven layout evaluations per width on main; the author reports repeated timing comparisons, but no tests were run during this read-only review. Is this the best way to solve the issue? The change addresses test overhead through existing lifecycle helpers, preserves independent browser contexts and assertions, and avoids changing application behavior. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 2ed2ce1f0136. Provenance checked
TestingProof path: in-process harness. SecurityNone. EvidenceWhat I checked:
Likely related people:
Review metrics
LabelsLabel changes:
Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment Reviewed October 18, 2026, 8:03 AM ET / 12:03 UTC. |
Problem
#2819 grew "log timestamps with their UTC offset stay on one line" (
tests/browser/console-agent-logs.test.mjs) from 5 to 10 locale cases and to seven browser round trips per viewport width (22 widths per case). Each case also launched its own Chromium and signed in again. In CI the test went from about 4.4 s to 11.0 s, which moved thechecks-browserlane p50 from 75 s to 82 s. The file'sfile-durations.jsonrow (74.0 s) was stale again against a p50 of 82.3 s.Change
Test-only. The locales, time zones, widths and assertions are the same as before.
fileBrowser()helper. Each case still gets its own context, with its own locale and time zone.evaluateper width returns everything the assertions need: each cell's line count and its left and right fit, the column starts for.log-time,.log-leveland.log-origin, each time cell's width and font size, overflow, and the message box. The assertions run on that result as before.file-durations.json's console-agent-logs row goes from 74.0 s to 78.2 s. That is the median of the five green main runs ending at bef197b (82.3 s, the method test(ci): refresh three stale file duration rows #2815 used), less the 4.1 s this PR took off the test on its own CI. Thesourcenote says so.Verification
taskset -c 10,11) with the Korean font used for fix(console): size the log time column to the locale's widest timestamp #2819. Before: p50 9.40 s (min 9.16, max 9.93). After: p50 5.07 s (min 4.98, max 5.19). 20/20 passed, and 20/20 again on the final test head (p50 5.11 s).fitLogTimeColumnand ran both the old and the new test against each.data-long-time: both fail the same way ("message 191px wide beside the time, ko-KR Asia/Seoul at 1300px").--log-time-width, or sampling one hour: both pass, so the new test is no weaker than the old one on these.node scripts/ci/run-tests.mjs auditreports no issues.Risks
Low. This is test-only. The per-width checks now read one synchronous layout snapshot instead of seven back-to-back reads. That is as strict as the old first read, which was already the line-and-fit check.