Repository navigation
Tighten test fidelity in connection, proxy, and session storage tests - #2389
Merged
Merged
Conversation
Each simulated tab gets its own Jotai store, and createSessionScopedAtom reads the breadcrumb only at creation with no cross-tab listener, so with this harness an open tab never observes another tab's write and the tests pass trivially. The reload tests cover the isolation that can regress.
Restores the already-open tab tests removed earlier. They could not fail because the open tab was never mounted, so a cross-tab sync wired through onMount would not reach it. Subscribing the tab first makes each test fail under such a sync, which was confirmed by injecting one.
kmcginnes
added a commit
that referenced
this pull request
Oct 9, 2026
## Summary
A review recommended deleting cross-tab isolation tests as "can't fail"
after reasoning about the current code, when the tests only needed to
mount the open tab's atom. These two rules make that verdict something a
reviewer proves rather than argues.
```diff
REVIEW.md › Tests that can't fail
- Ask whether the test would fail if the change were reverted.
+ Name a plausible regression of the behavior the test's name promises,
+ and prove the verdict with a mutation. A test that stays green is the
+ finding; tighten it until the mutation turns it red. Delete only when
+ no regression of that behavior could reach the test.
docs/agents/testing.md › Special cases
+ Jotai mount effects: store.get doesn't mount an atom, so onMount never
+ runs. Mount it with store.sub(atom, () => {}) when the behavior depends on it.
```
The Jotai note is the gotcha behind the near-miss: [the test fidelity PR
(#2389)](#2389) showed that an
`onMount` cross-tab sync left the old tests green and turned the
subscribed ones red.
## Merge Danger
**Door:** two-way
**Impact:** none
Review and testing guidance only.
## Related
- Related to #2389
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
1 participant
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.
Summary
A few tests either couldn't fail or checked less than their names claimed, so this tightens them.
defaultConnection.jsonkeeps the full path of itsGRAPH_EXP_PUBLIC_OR_PROXY_ENDPOINT. The Docker image unification PR (#1773) deleted this check without a replacement.sessionScopedStorage.test.tsandactiveConnectionStorage.test.ts) never mounted the open tab's atom. A cross-tab sync wired throughonMount, the usual Jotai pattern, never reached it, so they passed either way. The harnesses now exposesubscribe(), and each test subscribes the open tab first.mapWithConcurrency.Evidence
Before: with an
onMountBroadcastChannel sync injected intocreateWriteThroughAtom, the already-open tab test still passed.After: with the same injection, the subscribed test fails on every run, and it passes against the real code.
Merge Danger
Door: two-way
This only touches tests and one doc comment. No production code changes.
Impact: none