SRE-1049: Move the Brunch chat and runbook tests out of test:unit - #9643
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
1c24f19 to
3a7f490
Compare
PR SummaryLow Risk Overview
Moved tests update paths for the extra directory level ( Reviewed by Cursor Bugbot for commit 8603ff9. Bugbot is set up for automated code reviews on this repo. Configure here. |
1c8fdf8 to
d8f9cef
Compare
The Petrinaut chat test and the headless runbook test each spawn a child Node process that imports the built application bundle and opens a SQLite database. Both take longer than vitest's default 5000 ms timeout and fail under `test:unit`. Move them, the scripts they spawn, and the shared result types into `test/integration`, where `vitest.integration.config.ts` picks them up and `vitest.config.ts` already excludes them. Their assertions and their timeout are unchanged. `test:unit` was supplying the app build and the workspace library builds these tests read, so give `test:integration` the same dependencies in the package's `turbo.json` and regenerate `docs/task-dependencies.json`.
`@hashintel/brunch-agent` and `@hashintel/brunch-agent-plugin-sdcpn` are both dependencies of `@apps/brunch-agent`, so `^build` already covers them. Naming them again put each in the task graph twice. `turbo run test:integration --dry=json` resolves to the same eight dependencies and the same 83-task graph either way, and the regenerated `docs/task-dependencies.json` is unchanged.
A unit test should not need a build step. Three of the package's tests did: `prepared-workpiece.integration.test.ts` spawns a child process that imports `dist/app.mjs`, and `build-artifact.test.ts` reads the emitted bundles off disk and imports the built application. Move those two, the child script the first one spawns, and the `run-node-script` spawn helper, which now has no importer outside `test/integration`. Adjust the depth-relative paths in each. With nothing left in `test:unit` that reads `dist/`, drop `build` from its `dependsOn`. `codegen` and `^build` stay: the workspace libraries this package imports export only built JavaScript, so their builds are still required. `assets.test.ts` stays a unit test. It drives the asset route over a directory it writes in `tmpdir`, and never reads the build output.
`vitest.integration.config.ts` set no `testTimeout`, so the tests it runs had vitest's 5000 ms default — the same limit they exceeded under `test:unit`. A child-process boot that imports the built application measured 4525 ms on its own, and the Petrinaut chat test boots one twice. Use the values `tests/hash-backend-integration` uses: 60 s per test, 120 s per hook. `vitest.config.ts` keeps the default, so unit tests stay fast by construction.
d8f9cef to
8603ff9
Compare
Requested by Tim Diekmann · Slack thread
🌟 What is the purpose of this PR?
Before this change,
@apps/brunch-agentran two tests undertest:unitthat each spawn a child Node process, import the built application bundle fromdist, and open a SQLite database: the Petrinaut chat test and the headless runbook test. Both take longer than vitest's default 5000 ms timeout, so both fail. The most recent failure is on #9636.After it, four tests sit in
test/integration: the two above plusprepared-workpiece.integration.test.ts, which spawns the same kind of child, andbuild-artifact.test.ts, which reads the emitted bundles off disk. Those were the only files undertest:unitthat needed the package's own build output, sobuildcomes out oftest:unit'sdependsOnas well;codegenand^buildstay, because the workspace libraries this package imports export only built JavaScript. The integration config now sets a 60 s test timeout and a 120 s hook timeout, matchingtests/hash-backend-integration, since the 5000 ms default is what these tests were failing on in the first place.vitest.config.tskeeps the default, so a unit test that grows a build dependency or a child process fails there rather than passing slowly.🔗 Related links
🚫 Blocked by
🔍 What does this change?
petrinaut-chat.test.tsandrunbook-headless.test.tsintotest/integration, with the*.integration.tsscripts they spawn and the result types the chat pair shares.vitest.integration.config.tsmatches the two tests there, andvitest.config.tsalready excludes that directory.run-node-scriptimport, the imports of../src, the monorepo root passed to the spawn helper, and the workpiece fixture the runbook script reads.test:integrationin the package'sturbo.jsonwith the dependenciestest:unithad been supplying, since both tests importdist/app.mjsand read two built workspace libraries.apps/brunch-agent/docs/task-dependencies.json, which now lists the same builds fortest:integrationas fortest:unit.Pre-Merge Checklist 🚀
🚢 Has this modified a publishable library?
This PR:
📜 Does this require a change to the docs?
The changes in this PR:
🕸️ Does this require a change to the Turbo Graph?
The changes in this PR:
turbo.json's have been updated to reflect thistest:integrationgains the app build and the workspace library builds;docs/task-dependencies.jsonis regenerated with turbo 2.10.12, the version pinned in.config/mise/config.tomlassets.test.tsstays a unit test. Its CI name mentions build outputs, but it drives the asset route over a directory it writes intmpdirand never readsdist/.libs/@hashintel/brunch-agent/docsname the two tests at their old paths. They are mission records, so this PR leaves them alone.🐾 Next steps
🛡 What tests cover this?
test:integration, which CI runs for every affected package, and@apps/brunch-agenthas atest:integrationscript, so they keep running.❓ How to test this?
turbo run test:integration --filter @apps/brunch-agentand confirm all five test files intest/integrationrun and pass.turbo run test:unit --filter @apps/brunch-agentand confirm it runs 19 files, none of the four moved ones, and that it no longer builds the package first.