Repository navigation
test(browser): prepare fixture dependencies once per run - #1126
Conversation
59d6a9b to
2dcc6e2
Compare
Every Vite fixture server started with an empty optimizer cache, so it scanned and bundled the app dependencies inside the test's 15 s navigation budget. On a busy runner that ran past the budget (messages.spec failed all seven tests in one CI job). Playwright's globalSetup now scans every fixture page and bundles the dependencies once. Each fixture server starts from its own copy of that cache, so servers stay isolated. The helper fixes the settings in Vite's config hash (root, plugin names, optimizeDeps) so the copy is valid for every fixture: fixture plugins that only add middleware or rewrite index HTML run as one stable plugin, and fixtures can no longer set optimizeDeps. Co-authored-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> Signed-off-by: Logan Johnson <loganj@squareup.com>
2dcc6e2 to
7896b81
Compare
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Thanks for this. The approach is sound: pinning the root, plugin names and optimizeDeps so one prepared cache matches every fixture server, keeping a private copy per server, and failing loudly on plugins or configs that would change the bundle. One gap keeps it from fully doing what it sets out to do.
Blocking: a vite build earlier in the same worker still forces a full re-optimization inside the test.
In the CI run at the head commit 7896b81 (run 38670801149), two Browser journeys shards still re-bundle inside tests: chromium 116216855958 and webkit 116216856307. Each logs Re-optimizing dependencies because vite config has changed five times, followed by scanning dependencies… / bundling dependencies…, for the terminal-motion and terminal-renderer servers. That is the same cold scan and bundle inside the 15 s navigation budget that this PR is meant to remove. It also means the description's claim that no shard log has a scanning or bundling line doesn't hold at this head.
Cause: Vite's dependency config hash includes process.env.NODE_ENV || config.mode (getConfigHash in Vite 8.3.0). Setup bundles the seed with NODE_ENV=development. In both shards, the worker that ran the terminal specs had just run reminders-layout.spec or sidebar-reveal.spec, which call vite build. When NODE_ENV is unset, build sets it to production for the rest of the worker process. Every fixture server created on that worker afterward hashes differently, throws away the copied cache, and optimizes from scratch. tests/browser/build.mjs (the built-app fixture) and channel-members-focus.spec also call build, so any worker that runs a built-app spec before a fixture spec can hit this. Which shards are affected depends on test order.
This is the same kind of NODE_ENV leak the setup already guards against, just in the other direction and inside the worker. Two possible fixes:
- Have the four in-worker
buildcall sites restoreNODE_ENVthe same wayprepareFixtureDependenciesdoes, perhaps through one shared helper. - Or make the hash independent of the worker's leftover state in
createServer, for example by pinningNODE_ENVwhile the server initializes its optimizer and then restoring it.
Either fix should come with a regression test: a build followed by createServer still reuses the prepared cache. That test would catch this case, which the current marker test doesn't.
Minor
src/bundled/workflows/workflows.playwright.config.mjs, the focused offline config for the workflow editor, has noglobalSetup. Running it directly now hits the new "Fixture dependencies are not prepared" error increateServer, becauseworkflows.journey.mjsuses the helper. Either addglobalSetupto that config, or point the error message at both configs.- Relatedly, the scan entries cover
index.htmlandtests/**/*.html, butsrc/bundled/workflows/fixture.htmlandsession-fixture.htmlare also served through the helper. Their dependencies are currently reachable fromindex.html(CI shows no runtime discovery for the workflow journeys), but listing them explicitly would keep that from being an accident. The docs line "every HTML page undertests/" would need the same update.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
* origin/main: perf(css): replace wide :has() anchors with owner-set data attributes (#1180) fix(goose): share an MCP server pool and harden session lifecycle (#1142) test(browser): hold reading focus until the unread baseline is observed (#1182) Refine the profile menu and open status editing immediately (#1172) unify session naming, workspace and lifetime settings (#1079) perf(messages): stop forcing a layout on every action bar position frame (#1184) test(browser): prepare fixture dependencies once per run (#1126) test: replace fragile string assertions with precise checks (#1162) test: keep thread fixture errors out of http responses (#1163) feat: upgrade tauri and native plugins (#1169) test: pass native menu scripts to eval as arguments (#1160) test: cover shared link url policy (#1165) test: preserve literal text across markdown comment projection (#1164) test: pin html escaping of clipboard link labels (#1166) Preserve linked text pasted from other apps (#1069) fix(messages): keep the reader's message in place when a thread reply and layout change land together (#1130) Signed-off-by: John Tennant <jtennant@squareup.com> # Conflicts: # src/bundled/channels/ChannelBody.tsx # src/bundled/channels/ConversationTab.tsx
* commit 'f2595b8df20c9e6e91f79e77b52b700137063a49': (25 commits) Add community leave and device removal settings (#954) perf(css): replace wide :has() anchors with owner-set data attributes (#1180) fix(goose): share an MCP server pool and harden session lifecycle (#1142) test(browser): hold reading focus until the unread baseline is observed (#1182) Refine the profile menu and open status editing immediately (#1172) unify session naming, workspace and lifetime settings (#1079) perf(messages): stop forcing a layout on every action bar position frame (#1184) test(browser): prepare fixture dependencies once per run (#1126) test: replace fragile string assertions with precise checks (#1162) test: keep thread fixture errors out of http responses (#1163) feat: upgrade tauri and native plugins (#1169) test: pass native menu scripts to eval as arguments (#1160) test: cover shared link url policy (#1165) test: preserve literal text across markdown comment projection (#1164) test: pin html escaping of clipboard link labels (#1166) Preserve linked text pasted from other apps (#1069) fix(messages): keep the reader's message in place when a thread reply and layout change land together (#1130) separate session sharing from explicit channel conversion (#1070) feat(goose): bundle goose-acp with the app (#1110) feat(agents): answer unmentioned messages in one-to-one DMs (#1173) ... Signed-off-by: Sol <49aa1f65411fd096d2e2ec144f1e7aa36fdc76d1b907cfdf7be000c66f9d3b8e@buzz.block.builderlab.xyz>
🤖
Summary
Browser tests that start their own Vite dev server sometimes fail with
page.goto: Timeout 15000ms exceeded. The cause is not a slow page. Every fixture server started with an empty dependency cache. So on the first page load, Vite scanned the fixture pages and bundled all of the app's npm dependencies inside the test's 15 s navigation budget. On a busy CI runner that work can take longer than 15 s. In one CI job (116031456316), Vite loggedscanning dependencies… bundling dependencies…before each test, and all 7messages.spectests failed in a row.After this change, the browser test run scans and bundles the dependencies once, in Playwright's
globalSetup, before any test starts. Each fixture server starts from its own copy of that cache. No fixture server scans or bundles dependencies during a test.Details
optimizeDeps, and those values varied from spec to spec.tests/browser/vite-server.mjsnow sets all three:optimizeDepsis owned by the helper, so a fixture can no longer set it. Its scan coversindex.htmland every HTML page undertests/.@jsquash/webp/encode.js, which one test imports only frompage.evaluate, is ininclude.configureServer) or rewrite index HTML (transformIndexHtml). Neither changes the bundled dependencies, so the helper runs them all as one plugin with a fixed name. A fixture plugin with any other hook is rejected with an error.close()still deletes it. One server cannot invalidate another server's imports. The copy uses copy-on-write where the filesystem supports it.optimizeDepsfrommentions,channel-completionandcomposer-attachments.conversation.specmoves from a directviteserver onto the helper.messages,channel-completion,mentions,composer-attachmentsand the other specs with custom middleware), and it makes the first, cold server on each worker fast too.emoji.specstill has its own direct cache here, because test(browser): serve stateless dev-server specs from the shared source server #1109 moves it to the shared server. Expected overlap: one line each indocs/browser-testing.mdandtests/integration/vite-fixture.test.mjs.NODE_ENV=developmentwhen it is unset. Setup runs in the Playwright runner, and workers inherit its environment, so a leaked value would make the built-app specs' production build reportimport.meta.env.DEVas true (developer settings then appear and fetch/api/relay/stats). Setup restoresNODE_ENVto its original value.optimizeDeps(), which is deprecated. If a later Vite release removes it, setup will fail with an error before any test runs. It will not fall back to a cold cache.Evidence
scanning dependencies,bundling dependenciesornew dependencies optimizedline, and none hasTimeout 15000ms exceeded.messages+channel-completionChromium run (9 tests), withDEBUG=vite:deps:main: 9 dependency scans and 9 bundles, one inside each test.messages,channel-completion,mentions,composer-attachments,image-copy-focus,window-chrome,custom-emoji-authoring,mocked-native-ipcandavatar-shapes(sharedsourceOriginserver). 58 passed. That run did one bundle (in setup) and had 58 cache reuses. It had no scans and no runtime re-optimization.tests/integration/vite-fixture.test.mjsadds three tests:define) all start from the prepared cache. Both possible regressions make this test fail: removing the plugin merge, and removing the copy.NODE_ENVunchanged. This test fails without the restore.appearance,account-profile,gifs,external-links) withmessagesandchannel-completion, Chromium: 28 passed, no/api/relay/statspage errors.