test: stop depending on another test file's leaked indexedDB global - #1201
test: stop depending on another test file's leaked indexedDB global#1201njbrake wants to merge 3 commits into
Conversation
_Note: this PR description was drafted by Claude via back-and-forth with @njbrake. The reasoning and decisions are his; the prose is Claude's._ Removes a cross-file dependency that makes `sidebar-footer.test.tsx` fail depending on test order. Its "sync retry flow" block enables sync, which puts the encryption config on the render path. That reads the bare `indexedDB` global, which happy-dom does not implement, so the tests only passed when some earlier test *file* had already stubbed it. With `--randomize`, an ordering that runs this file first fails with `ReferenceError: indexedDB is not defined`. `use-app-initialization.test.tsx` was the accidental supplier, and its restore was itself wrong: it captured `globalThis.indexedDB` at module load (undefined, since happy-dom has none) and wrote that back in `afterAll`, leaving the property defined-but-undefined rather than absent. Adds `stubIndexedDb`/`restoreIndexedDb`, which delete the global when it did not exist before, and has both files opt in explicitly. The absence is load-bearing elsewhere, since the boot pipeline's storage pre-flight is supposed to report STORAGE_UNAVAILABLE without it, so this stays opt-in rather than going into the global preload. I saw this fail on a randomized run of `main`. I could not pin a seed that reproduces it on `main` itself, but it reproduces deterministically once any single extra test file changes the shuffle. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
8d9d8ff to
7b2dc27
Compare
|
Preview environment deployed 🚀
Stack: Auto-destroys on PR close/merge. Login via the bundled Keycloak realm — |
ital0
left a comment
ital0
left a comment
There was a problem hiding this comment.
I can see one concrete benefit here: use-app-initialization.test.tsx now restores a missing indexedDB global correctly. The old cleanup left the property defined with an undefined value.
I could not reproduce the reported failure. I ran the affected tests alone and together, then ran the full randomized suite five times on the parent commit and five times on this PR. Every run completed with 4,312 passing tests and no failures. That does not prove the flake cannot happen, but this PR has no regression test that fails before the change and passes after it.
My main concern is the sidebar test. It reaches key storage only when the E2EE state sends it there, but the test does not control that state. The new IndexedDB stub reports that no key exists, which makes the hook disable sync after render. The test can therefore assert the retry UI before the async migration check finishes.
The shared helper also presents itself as an IDBFactory that supports reads and writes, but several key-storage operations do not work with it. It can also leave the stub installed when calls are nested.
As written, the PR fixes one cleanup bug, but it does not demonstrate the reported failure or add coverage. It also introduces a shared fake that can leave global state behind and can make key-storage calls hang or throw.
| // put this file first failed with `ReferenceError: indexedDB is not defined`. | ||
| // Stubbing it here makes the block self-contained. | ||
| beforeEach(() => { | ||
| stubIndexedDb() |
There was a problem hiding this comment.
These tests do not control e2eeEnabled. In this path, the real needsSyncSetupWizard() only calls getCK() when encryption is enabled. This stub returns no key, so the hook disables sync after render. The assertions can therefore observe the retry UI before the async migration check finishes, instead of the stable state described by the test.
| afterAll(async () => { | ||
| await teardownTestDatabase() | ||
| Object.defineProperty(globalThis, 'indexedDB', { value: realIndexedDb, configurable: true, writable: true }) | ||
| restoreIndexedDb() |
There was a problem hiding this comment.
This cleanup fixes a real leak. The old code could replace a missing global with an indexedDB property whose value was undefined. However, no test asserts that this restore removes the property, so the same regression could return without any test failing.
| /** Present so `restoreIndexedDb` can tell "was absent" from "was something". */ | ||
| const absent = Symbol('absent') | ||
|
|
||
| let previous: IDBFactory | typeof absent = absent |
There was a problem hiding this comment.
previous is shared by every call. If stubIndexedDb() is called twice before restoring, the second call overwrites the first snapshot, and two restores still leave a stub installed. The helper can therefore create the same global leak it is meant to prevent.
| onblocked: null as (() => void) | null, | ||
| result: { | ||
| close: () => {}, | ||
| // Enough of a database for key-storage's read/write round trip to be |
There was a problem hiding this comment.
This says the fake supports a read/write round trip, but it does not store values, implement delete(), or fire transaction.oncomplete. I verified that storeCK() and getKeyPair() never settle, while clearCK() throws. The type assertion presents this incomplete object as a full IDBFactory, so future tests can hang or fail when they use the advertised operations.
- keep the fake to what it can honestly do: open() succeeds, but the database has no transaction, so key-storage round trips fail loudly instead of hanging on an event the fake never fires - make nested stub/restore pairs safe, so a restore can never leave a stub behind in place of the real global - pin the config store off in sidebar-footer's sync retry block: the encryption path was only reachable because another test file can leave that persisted module global switched on - cover the helper itself, including the delete-vs-undefined distinction the boot storage pre-flight depends on Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YPSDjepyzL3q9bZgDmtPm2
ital0
left a comment
ital0
left a comment
There was a problem hiding this comment.
Hey! Not sure if you're done with the fixes... But I took another look and the follow-up fixes several issues from the first review.
The main order dependency is still reproducible. Running pending-device-modal.test.tsx before sidebar-footer.test.tsx leaves a global @/db/encryption module mock in place. Resetting the Zustand config does not remove that mock. In that order, two sidebar retry tests fail with ReferenceError: indexedDB is not defined.
The full randomized suite passed with 4,316 tests, and the three directly affected files passed with 39 tests. The specific two-file order above still failed with 25 passing tests and 2 failures. This means the general run can miss the exact ordering problem the PR is meant to remove.
The remaining test-isolation and documentation problems are noted inline.
| // on, so pin it off here. That keeps sync enabled for the whole block and keeps | ||
| // the encryption path (and IndexedDB with it) off the render path entirely. | ||
| beforeEach(() => { | ||
| useConfigStore.setState({ config: {} }) |
There was a problem hiding this comment.
This reset does not isolate the test from leaked module mocks. pending-device-modal.test.tsx registers a global mock.module('@/db/encryption'). When that file runs first, this reset does not remove the mock, and the two sync retry tests fail with ReferenceError: indexedDB is not defined. The test still depends on file order.
| describe('stubIndexedDb / restoreIndexedDb', () => { | ||
| // happy-dom ships no IndexedDB, so "absent" is the real baseline — assert | ||
| // against it explicitly rather than trusting whatever ran before this file. | ||
| beforeEach(() => { |
There was a problem hiding this comment.
This deletes the global before each test, but cleanup still happens inside the individual test bodies. If an assertion fails before restoreIndexedDb(), both the global and the helper's private snapshot stay dirty and can affect later randomized tests.
| Reflect.deleteProperty(globalThis, 'indexedDB') | ||
| }) | ||
|
|
||
| it('installs an openable factory and removes it again', async () => { |
There was a problem hiding this comment.
This file never exercises a key-storage read or write. The old fake that hung on transaction events would still pass all four tests here, so the new claim that unsupported operations fail instead of hanging is not covered.
| expect(opened).toBe(true) | ||
| restoreIndexedDb() | ||
| // Deleted, not set to undefined: code reading the bare global must still | ||
| // see a ReferenceError, which is what the boot pre-flight relies on. |
There was a problem hiding this comment.
This comment is not accurate. The boot preflight reads globalThis.indexedDB and returns false both when the property is absent and when its value is undefined. It does not rely on a ReferenceError. That error comes from key storage reading the bare global.
| }) | ||
|
|
||
| it('restores a pre-existing global rather than deleting it', () => { | ||
| const original = { marker: 'real' } as unknown as IDBFactory |
There was a problem hiding this comment.
This double assertion bypasses the type system and conflicts with the repository rule against as unknown as. It also treats a marker object as a complete IDBFactory even though it does not implement that contract.
| * fail loudly rather than hanging on an event this fake never fires. Extend it | ||
| * here, with a test, if a case genuinely needs a backing store. | ||
| * | ||
| * Nested calls keep the first snapshot, so a stub can never restore over a stub. |
There was a problem hiding this comment.
These calls are not safe as nested pairs. After an outer and inner stubIndexedDb(), the inner restoreIndexedDb() removes the outer caller's stub. The test named nested also calls restore only once, so it verifies repeated setup rather than nested ownership.
|
@ital0 thanks for the review! I'll take another look, thanks for your patience 🙏 ! |
- pending-device-modal.test.tsx and use-pending-device-notification.test.tsx
register a worker-global mock.module('@/db/encryption'). Bun also rewrites the
binding needsSyncSetupWizard reads inside ./config, so resetting the config
store could not switch encryption back off. Re-provide both modules with the
store-backed implementation, the same defense db/encryption/config.test.ts uses.
- cover the helper's contract: the fake exposes no transaction, so key-storage
calls throw instead of hanging on an event it never fires.
- reset the global and the helper's snapshot in afterEach, so a failed assertion
cannot leave either dirty for whatever the shuffle runs next.
- correct the delete-vs-undefined comment: the ReferenceError comes from key
storage reading the bare global, not from the boot storage pre-flight.
Verified: seeds 3200613218 and 4217200907 fail on ca4ff05 and pass here;
sidebar-footer paired with all 25 mock-registering test files went from 5
failing runs to 0 across 100 randomized runs; three full `bun run test` clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YPSDjepyzL3q9bZgDmtPm2
ital0
left a comment
ital0
left a comment
There was a problem hiding this comment.
Thanks for the follow-up. I took another pass at the latest commit and reran the failing file pairs. The IndexedDB helper is in much better shape now: cleanup runs in afterEach, repeated stubbing restores the original global, unsupported key-storage use fails instead of hanging, and the comments now match the implementation.
The two known pending-device orders now pass. I found one more leaked encryption export, though. If use-sync-enabled-toggle.test.ts runs first, its mocked needsSyncSetupWizard survives the new sidebar guards because the spread comes from an already mocked module and only isEncryptionEnabled is replaced. In that order, the sidebar first renders the retry state, then the migration effect disables sync. The existing assertions run before that effect settles, so the tests remain green while observing a transient state.
I reproduced this against the current main integration and left the details inline. I also noted two smaller repository-convention issues in the new code.
| const realEncryption = await import('@/db/encryption') | ||
| mock.module('@/db/encryption', () => ({ | ||
| ...realEncryption, | ||
| isEncryptionEnabled: () => useConfigStore.getState().config.e2eeEnabled === true, | ||
| })) |
There was a problem hiding this comment.
I may be missing a Bun detail here, but realEncryption can already be the namespace registered by an earlier mock.module(). use-sync-enabled-toggle.test.ts replaces needsSyncSetupWizard, and this spread carries that replacement forward because this guard only overwrites isEncryptionEnabled. I reproduced that order with E2EE off: needsSyncSetupWizard() still returned true, the hook rendered with sync enabled, and the migration effect then disabled it. The sidebar assertions pass before that effect settles, so this still leaves an order-dependent false positive.
These two registrations are also worker-global and permanent. That is the same behavior called out in docs/development/testing.md under "Avoid mock.module() for Shared Modules."
| // the real store-backed implementation, the same defense `db/encryption/config.test.ts` | ||
| // uses. Without it, a shuffle that runs either of those files first sends the sync | ||
| // retry tests into key storage and fails them on the missing `indexedDB` global. | ||
| const realEncryptionConfig = await import('@/db/encryption/config') |
There was a problem hiding this comment.
Small convention note: these await import(...) calls are not tied to a documented circular dependency. AGENTS.md asks us to prefer top-level imports in that case. Here the dynamic ordering seems to exist only to support the module-mock workaround.
| Reflect.deleteProperty(globalThis, 'indexedDB') | ||
| } | ||
|
|
||
| const openStub = async () => |
There was a problem hiding this comment.
Small style note: openStub is a new test utility without JSDoc. AGENTS.md asks new utility functions to carry a short JSDoc comment.
2 participants
Note: this PR description was drafted by Claude via back-and-forth with @njbrake. The reasoning and decisions are his; the prose is Claude's.
Removes a cross-file dependency that makes
sidebar-footer.test.tsxfail depending on test order.Its "sync retry flow" block enables sync, which puts the encryption config on the render path. That reads the bare
indexedDBglobal, which happy-dom does not implement, so the tests only passed when some earlier test file had already stubbed it. With--randomize, an ordering that runs this file first fails withReferenceError: indexedDB is not defined.use-app-initialization.test.tsxwas the accidental supplier, and its restore was itself wrong: it capturedglobalThis.indexedDBat module load (undefined, since happy-dom has none) and wrote that back inafterAll, leaving the property defined-but-undefined rather than absent.Adds
stubIndexedDb/restoreIndexedDb, which delete the global when it did not exist before, and has both files opt in explicitly. The absence is load-bearing elsewhere, since the boot pipeline's storage pre-flight is supposed to report STORAGE_UNAVAILABLE without it, so this stays opt-in rather than going into the global preload.I saw this fail on a randomized run of
main. I could not pin a seed that reproduces it onmainitself, but it reproduces deterministically once any single extra test file changes the shuffle.