Repository navigation
test: run suites without --isolate and remove three flaky tests - #8336
Merged
Merged
Conversation
The cross-file leaks that --isolate hid are fixed (#8314, #8315, #8316, #8319), so each package's suite now runs in one process. The desktop suite takes about 38 s without the flag and about 73 s with it. apps/relay still had one leak: host-tunnel.test.ts replaced partyserver with a stub that has no getServerByName, which broke index.test.ts in some file orders. The test passes against the real module, so the stub is gone. Removed as flaky: - workspace-fs: FsWatcherManager resumes a subscription after the root is deleted and recreated - i18n: locale changes preserve state and focus (provider-state) - agent-setup: Pi completion notifications, which poll up to 1 s for a detached script
Contributor
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
📒 Files selected for processing (33)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
No installed development build can load this pull request as an update: its native fingerprint is |
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Contributor
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "clean": "git clean -xdf .cache .turbo dist node_modules", | ||
| "typecheck": "tsc --noEmit --emitDeclarationOnly false", | ||
| "test": "bun test --isolate" | ||
| "test": "bun test" |
Contributor
There was a problem hiding this comment.
If PAGE_WATCH_TEST_DATABASE_URL is set and a file such as trpc.test.ts runs first, removing --isolate makes the watch test reuse the database client created by that earlier file. watch-ownership.postgres.test.ts changes DATABASE_URL, but importing the cached dbWs does not change its connection. The test queries the earlier database, which defaults to localhost on a clean checkout. It can then fail or write its fixtures to the wrong database.
Run this test in a separate process, or give it a client created from PAGE_WATCH_TEST_DATABASE_URL.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/trpc/package.json
Line: 143
Comment:
**Watch tests use wrong database**
If `PAGE_WATCH_TEST_DATABASE_URL` is set and a file such as `trpc.test.ts` runs first, removing `--isolate` makes the watch test reuse the database client created by that earlier file. `watch-ownership.postgres.test.ts` changes `DATABASE_URL`, but importing the cached `dbWs` does not change its connection. The test queries the earlier database, which defaults to localhost on a clean checkout. It can then fail or write its fixtures to the wrong database.
Run this test in a separate process, or give it a client created from `PAGE_WATCH_TEST_DATABASE_URL`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.It sets DATABASE_URL and then imports the db client. In one process the client may already exist, so the test would write its fixtures to the database the first file connected to. As an .integration.ts file it runs only when named, in its own process, like the other opt-in suites.
pgilmore
added a commit
to patiently/superset
that referenced
this pull request
Oct 11, 2026
Merges upstream main into the watcher build. Upstream now runs each test suite in one process and bans module mocks in the desktop suite (ParadevAI#8319, ParadevAI#8336), so the hover-card test drives the real hooks against a fake host instead of mocking them. Tests run on the bun canary upstream's CI uses, because bun 1.4.2 can hang in a synchronous spawn. Claude-Session: https://claude.ai/code/session_01CZBCrRCZ2rEBHv4DH3Wh3k
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Drop
--isolateThe cross-file leaks that
--isolatehid are fixed in #8314, #8315, #8316 and #8319. This removes the flag from all 26 test scripts, so each package's suite runs in one process.packages/host-servicekeeps--parallel. The one line inAGENTS.mdthat named the flag is corrected.The desktop suite takes about 38 s without the flag and about 73 s with it.
One more leak, found by the sweep
apps/relay/src/host-tunnel.test.tsreplacedpartyserverwith{ Server: class {} }. In some file ordersindex.test.tsthen failed to load withExport named 'getServerByName' not found. The test passes against the real module, so the stub is removed.Flaky tests removed
workspace-fs: "FsWatcherManager root deletion recovery > resumes the same subscription after the root is deleted and recreated"mainand on PRs, 2026-10-07/08i18n: "locale changes preserve state and focus while updating context formatters", with its fixtureagent-setup: "Pi completion notifications" (3 cases)The second
workspace-fsrecovery test ("does not hang teardown after root deletion") stays. The Pi deletion removes all coverage of Pi completion notifications.Verification
bun 1.4.2,
bun test --randomize --seed Nwithout--isolate: every one of the 26 packages passes on seeds 31, 32 and 33;apps/relayalso on 34-36 after its fix. Biome passes onappsandpackages; typecheck passes inapps/relayandpackages/workspace-fs.Summary by cubic
Drops
--isolatefrom all 26 test scripts now that the cross-file leaks it hid are fixed, so each package's suite runs in one process. The desktop suite drops from about 73 s to about 38 s;packages/host-servicekeeps--parallel.Removes the
partyserverstub inapps/relay/src/host-tunnel.test.ts, which lackedgetServerByNameand brokeindex.test.tsin some file orders.Also removes three flaky tests: the
workspace-fsroot-deletion recovery test, thei18nlocale-state fixture, and theagent-setupPi completion notification tests (also removing the only coverage of that feature).watch-ownership.postgres.integration.tsso it no longer runs in the shared suite; it setsDATABASE_URLand imports the db client, which in one process could write fixtures to whichever database the first file connected to. It now runs only when named, in its own process, like the other opt-in suites.Written for commit e147dbf. Summary will update on new commits.
Summary by CodeRabbit