Conversation
|
Status: reworked to the scheduling change alone, waiting for CI on 607aa3d. The first version (pooled views plus rewritten assertions) measured 9.79s in the parallel bucket on the macOS 15 VM and 3.4s on x64 (build 109335), against 72 to 123s and 15s on main, but two of the assertions it added were wrong on WebKit and a key-input test flaked under concurrency. This version keeps every test body as it is on main and changes only how the tests are scheduled: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. WalkthroughChangesThe WebView test suite now uses lane-aware concurrency, structured constructor error checks, concurrent lifetime stress coverage, serialized native input tests, and child-process isolation for process-sensitive scenarios. WebView Test Suite
Merge Risk: ⚪ Minimal · up to The PR makes the WebKit webview tests run concurrently while preserving serialized native-input tests and adds cross-platform constructor validation; it should reduce CI time without changing product behavior, and no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the problem, implementation, verification results, performance context, and test-scope constraints. It does not use the exact template headings, but it contains the required information and is sufficiently complete. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…cles on 7 lanes webview.test.ts opened about 110 WKWebViews one after the other, one WebContent process launch each. In the Tart macOS VMs that run the macOS 15 and 26 CI lanes the file took 72 to 123 seconds. Every macOS test now runs with test.concurrent, through a lane that caps how many tests of a kind are in flight: own-view tests four at a time, native input tests one at a time, child process tests unlaned. The 70 lifetime create/navigate/close cycles run on seven lanes; the cap they guard against is on lifetime instances, not concurrent ones. The constructor validation tests are a table with exact error shapes and run on every platform.
robobun
changed the title
54a665c to
607aa3d
Compare
There was a problem hiding this comment.
LGTM — both earlier points are addressed: skip now goes through test.concurrent.skip so the group stays contiguous, and the concurrent-screenshot() assertion is gone with the rest of the assertion rewrite.
Checked the lanes() semaphore (hand-off keeps busy counted, no over-admission), the 7-lane lifetime test (shared ++next is JS-atomic; created === 70 still proves the 65th lifetime instance navigated; watchdog is a rejecting deadline, not a sleep), and that the plain test/test.each validation cases sit before the first test.concurrent so they don't split the group. test.todo at EOF has nothing after it to split.
Extended reasoning...
Overview
Test-only refactor of test/js/bun/webview/webview.test.ts to run the macOS WebView tests concurrently instead of sequentially. Adds a lanes(width) counting-semaphore helper and tiered wrappers (it capped at 4 concurrent view-opening tests, itInput serialized for native key/mouse, itRendering todo-on-CI, itInChild/itConcurrent uncapped). The 70-cycle lifetime regression test is parallelized across 7 internal lanes with a shared next counter. Constructor validation is lifted into a platform-agnostic test.each with exact {name, code, message} assertions via a new thrown() helper. No production code touched.
Security risks
None. Test-only change; no auth, crypto, network egress, or untrusted input handling introduced. The only server usage remains local Bun.serve({ port: 0 }).
Level of scrutiny
Low-to-moderate. This is a test-file scheduling refactor with no runtime code changes. The concurrency helper is a textbook semaphore (hand off directly to a waiter or decrement busy). The lifetime test's invariant — that the 65th+ lifetime WKWebView can navigate — is preserved: ++next is atomic in single-threaded JS, each lane closes its view before pulling the next number, and expect(created).toBe(70) guards against a lane bailing early. The 30s watchdog is a rejecting race deadline (commented as such), not a sleep, and the strengthened expect(view.url).toBe(url) is an improvement over toStartWith.
Other factors
This is a re-review after a force-push. The prior version's two flagged issues are both resolved: the Promise.all of two screenshot() calls on one view was dropped along with the rest of the assertion rewrite (confirmed absent from the file), and skip is now test.concurrent.skip, so itPersistentDataStore on macOS < 15.2 no longer splits the concurrent group. The plain test / test.each validation cases are positioned before the first it(...), so the concurrent group starts fresh after them; the trailing test.todo at EOF has nothing to split. No CODEOWNERS entry covers this path. Exit reason was dry_streak.
This branch has not been deployed
1 participant
Problem
test/js/bun/webview/webview.test.tsis one of the slowest files in CI: 72 to 83s on the macOS 15 lane and 115 to 123s on the macOS 26 lane (both Tart VMs), for example 76.6s in build #109310. The physical macOS boxes take 9s (arm64) and 15s (x64).Fix
test.concurrent, through a lane that caps how many tests of a kind are in flight: tests that open their own views 4 at a time, tests that drive native input 1 at a time, child-process tests unlaned. A skipped test stays on the concurrent chain so it does not split the group.width,height,headless) are one table with exactname,codeandmessage, and run on every platform: validation throws before a backend is touched. Test bodies, view sizes and the other assertions are unchanged, so the open PRs that touch this file (webview: sample click(selector) stability from distinct rAF callbacks #35173, webview: add mouse.down/up/move primitives for drag automation #29817, blob: report the size of Blobs created by the native bindings to the GC #37697, webview: let a dead transport release its browser, and route closeAll() through it #40176) still apply.bun bd test test/js/bun/webview/webview.test.tson Linux (6 pass, 58 skip, 1 todo, 2.06s and 2.07s; before: 2 pass, 60 skip, 2.10s and 2.17s). The macOS numbers come from CI: see the notes. Self-reviewed: the first version of this PR also pooled views and rewrote the assertions. The review found that the scheduling change carries most of the win and that the rewrite is what conflicts with the open PRs, and CI agreed (the two red tests were assertions the rewrite added). This version is the scheduling change alone.Background
Bun.WebViewon macOS is one host subprocess per bun process (the bun binary re-executed withBUN_INTERNAL_WEBVIEW_HOST). Eachnew Bun.WebView()is a WKWebView plus an off-screen NSWindow in that host; its firstnavigate()launches a WebContent process. Operations on different views run in parallel, so concurrent tests overlap the launches.bun testruns consecutivetest.concurrenttests as one group, up to--max-concurrency(default 20). The plain validation tests at the top of the file run first, then the whole macOS group. The lanes inside the file bound the number of live WebContent processes independently of that default.press("Escape")arrive in the page as a stream of repeated keydowns (5 on x64, 610 on the VM), which is why those tests are serialized.todoon CI, as before: the CI runners have no display, so CVDisplayLink never fires.Notes
Timings of
test/js/bun/webview/webview.test.tson main, from the Buildkite job logs (the file runs in the parallel bucket, 4 files at a time):Per-test timings are not in the logs (only the file total), and the JUnit artifacts are on S3, which this environment cannot reach, so the breakdown is derived from the view count: 76.6s / 110 views is about 0.7s per view in the VM, 9s / 110 about 85ms on the physical arm64 box.
The first version of this PR (pooled views, 7 lifetime lanes, assertions rewritten) measured in build #109335: 9.79s in the parallel bucket on darwin-arm64-focaccia-tart-15, then 6.29s, 3.44s and 3.25s when the runner re-ran the file alone; 3.42s, 2.98s, 3.01s and 3.03s on darwin-x64-14.8.9. Two tests were red on every attempt, both from assertions that version added (two
screenshot()calls in flight on one view, which the per-view slot guard rejects; anavigate()right afteronNavigationFailed, which fires before the failed navigation's slot is released). A third test,press dispatches virtual keys, flaked once per lane with repeatedEscapekeydowns. The numbers for this version come from its own CI run.The
lanes()helper is the same as the one in #39078 (the Chrome sibling file); the two can share a module once either lands.Linux runs,
bun bd test test/js/bun/webview/webview.test.ts: before 2.10s and 2.17s (2 pass, 60 skip, 1 todo), after 2.06s and 2.07s (6 pass, 58 skip, 1 todo). Nothing but the constructor validation runs off macOS.