fix(tests): eliminate user-scope pollution and global module mocks in tests#401
Merged
Merged
Conversation
The v0.44.0 release failed on an order-dependent test flake:
- init-project.test.ts ("vscode returns auto-installed") exercised the
real configureVscodeSettings with mocked credentials, writing the
marketplace URL into the runner's REAL ~/.config/Code/User/settings.json
(and developers' real VS Code settings on local runs).
- vscode-settings.test.ts overrode HOME per-test, but Bun caches
os.homedir() on Linux, so the override was silently ignored and the
tests read/wrote the real (now polluted) settings file.
- Bun's test-file execution order is non-deterministic across runs, so
the failure only appeared when init-project.test.ts happened to run
first - the same tree passed CI on the release PR and failed on main
13 minutes later.
Fixes (test-only, no production changes):
- init-project.test.ts: spy out configureVscodeSettings so initProject
never writes user-scope state.
- vscode-settings.test.ts: replace HOME env overrides with
spyOn(os, "homedir").mockReturnValue(tempDir) - verified the spy
reaches named imports in other modules. Keep the APPDATA env override
for the Windows branch, which reads Bun.env at call time.
- vscode-settings.test.ts: restore saved env by deleting originally
unset keys instead of Object.assign stringifying undefined.
- ARCH-005: capture both rules (mock os.homedir(), never write real
user-scope state in tests).
Signed-off-by: Rhuan Barreto <rhuan@barreto.work>
Deploying archgate-cli with
|
| Latest commit: |
75a5ccc
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a2874e3d.archgate-cli.pages.dev |
| Branch Preview URL: | https://fix-vscode-settings-test-pol.archgate-cli.pages.dev |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds policy and refactors multiple test suites to avoid writing real user-scoped state by mocking ChangesTest Isolation via os.homedir() Mocking
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage
Full HTML report available in workflow artifacts. Per-directory breakdown
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Sweep of all test files overriding HOME/APPDATA/XDG_* envs, auditing each against what the production code actually reads: - env overrides are SAFE for consumers of paths.ts resolvers (archgateHomeDir, opencodeConfigDir, opencodeDbPath, cursorUserDir, copilotSessionStateDir) and upgrade.ts/install-info.ts - all read Bun.env.HOME / USERPROFILE / XDG_* at call time. No changes needed for clean, upgrade, auth, binary-upgrade, credential-store, sentry, telemetry*, update-check, opencode-settings, plugin-install-cleanup, session-context-opencode tests. - session-context.test.ts / session-context-cursor.test.ts worked around Bun's homedir() caching by writing into the REAL ~/.claude/projects and ~/.cursor/projects with unique names. Now spy os.homedir() into a mkdtemp tempHome instead (readClaudeCode- Session/readCursorSession call homedir() at call time). - session-context-copilot.test.ts created real ~/.copilot/session-state and tracked created dirs for cleanup. Now overrides Bun.env.HOME to a mkdtemp tempHome (copilotSessionStateDir reads Bun.env at call time). - plugin-install.test.ts was the worst case: installCursorPlugin / installOpencodePlugin tests mocked fetch and Bun.spawn but NOT the filesystem prep in installEditorPluginBundle - every test run created dirs under and DELETED archgate-* agents/skills from the developer's real ~/.cursor and ~/.config/opencode. Now redirects Bun.env.HOME + XDG_CONFIG_HOME to a mkdtemp tempHome file-wide. Signed-off-by: Rhuan Barreto <rhuan@barreto.work>
…-install tests
mock.module("../../src/helpers/platform") is process-global: it replaces
the whole platform module (only resolveCommand exported, everything else
undefined) for every test file in the run and is not undone by
mock.restore(). ARCH-005 forbids mock.module on first-party modules for
exactly this reason - it is the same order-dependent flake class as the
user-scope pollution fixed in the previous commits.
Convert to the documented pattern: import * as platform + per-test
spyOn(platform, "resolveCommand") installed in beforeEach and restored
via mock.restore() in afterEach. The spy reaches the named import inside
plugin-install.ts through Bun's live bindings.
Full suite passes, confirming no other test file depended on the leaked
global mock.
Signed-off-by: Rhuan Barreto <rhuan@barreto.work>
rhuanbarreto
changed the title
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/helpers/plugin-install.test.ts (1)
117-127:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd explicit restoration of
mockResolveCommandspy.
spawnSpy.mockRestore()is called on line 119, butmockResolveCommand(created inbeforeEachon line 99) is never restored. For consistency with thespawnSpypattern and to ensure complete test isolation, explicitly restore the spy.🔧 Proposed fix
afterEach(() => { globalThis.fetch = originalFetch; spawnSpy.mockRestore(); + mockResolveCommand.mockRestore(); mock.restore(); if (savedHome === undefined) delete Bun.env.HOME;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/helpers/plugin-install.test.ts` around lines 117 - 127, The afterEach cleanup forgets to restore the mockResolveCommand spy created in beforeEach, so add an explicit restore for mockResolveCommand (e.g., call mockResolveCommand.mockRestore()) alongside the existing spawnSpy.mockRestore() in the afterEach block to ensure the spy is removed and tests remain isolated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/helpers/plugin-install.test.ts`:
- Around line 117-127: The afterEach cleanup forgets to restore the
mockResolveCommand spy created in beforeEach, so add an explicit restore for
mockResolveCommand (e.g., call mockResolveCommand.mockRestore()) alongside the
existing spawnSpy.mockRestore() in the afterEach block to ensure the spy is
removed and tests remain isolated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c470d6f9-e971-4912-819e-eb85c797d047
📒 Files selected for processing (1)
tests/helpers/plugin-install.test.ts
mock.restore() already restores all spyOn spies, but restore it explicitly for symmetry with spawnSpy and clearer intent. (CodeRabbit review feedback on PR #401.) Signed-off-by: Rhuan Barreto <rhuan@barreto.work>
…tall-cleanup
CI failed after the previous commit removed plugin-install.test.ts's
mock.module("platform"): plugin-install-cleanup.test.ts ALSO globally
mocked the platform module (and paths). On Linux file order the cleanup
file runs first, its leaked module mock is what plugin-install's new
spyOn wrapped and "restored" to, and platform.test.ts / doctor.test.ts
then saw a bare mock returning undefined instead of the real
resolveCommand. The two competing mock.modules had been masking each
other; removing only one exposed the other.
Convert the last first-party mock.modules to the ARCH-005 patterns:
- platform: per-test spyOn(platform, "resolveCommand") in beforeEach,
restored in afterEach.
- paths: no mock at all - cursorUserDir(), opencodeConfigDir(), and
internalPath() read Bun.env.HOME / XDG_CONFIG_HOME at call time, so
the existing tempDir env override (now including HOME, saved and
restored per-test) isolates the real resolvers. Drops the
cursorDirOverride indirection.
- env restore now deletes originally-unset keys instead of assigning
undefined.
Signed-off-by: Rhuan Barreto <rhuan@barreto.work>
Merged
rhuanbarreto
pushed a commit
that referenced
this pull request
Jun 10, 2026
# archgate ## [0.45.0](v0.43.0...v0.45.0) (2026-06-10) ### Features * **cursor:** automated plugin install via tarball download ([#395](#395)) ([41eb37e](41eb37e)) ### Bug Fixes * **opencode:** extract to config dir and clean stale files on install ([#400](#400)) ([51b41e7](51b41e7)) * **tests:** eliminate user-scope pollution and global module mocks in tests ([#401](#401)) ([0cff642](0cff642)) --- This PR was generated with [simple-release](https://github.com/TrigenSoftware/simple-release). <details> <summary>📄 Cheatsheet</summary> <br> You can configure the bot's behavior through a pull request comment using the `!simple-release/set-options` command. ### Command Format ````md !simple-release/set-options ```json { "bump": {}, "publish": {} } ``` ```` ### Useful Parameters #### Bump | Parameter | Type | Description | |-----------|------|-------------| | `version` | `string` | Force set specific version | | `as` | `'major' \| 'minor' \| 'patch' \| 'prerelease'` | Release type | | `prerelease` | `string` | Pre-release identifier (e.g., "alpha", "beta") | | `firstRelease` | `boolean` | Whether this is the first release | | `skip` | `boolean` | Skip version bump | | `byProject` | `Record<string, object>` | Per-project bump options for monorepos | #### Publish | Parameter | Type | Description | |-----------|------|-------------| | `skip` | `boolean` | Skip publishing | | `access` | `'public' \| 'restricted'` | Package access level | | `tag` | `string` | Tag for npm publication | ### Usage Examples #### Force specific version ````md !simple-release/set-options ```json { "bump": { "version": "2.0.0" } } ``` ```` #### Force major bump ````md !simple-release/set-options ```json { "bump": { "as": "major" } } ``` ```` #### Create alpha pre-release ````md !simple-release/set-options ```json { "bump": { "prerelease": "alpha" } } ``` ```` #### Publish with specific access and tag ````md !simple-release/set-options ```json { "bump": { "prerelease": "beta" }, "publish": { "access": "public", "tag": "beta" } } ``` ```` ### Access Restrictions The command can only be used by users with permissions: - repository owner - organization member - collaborator ### Notes - The last comment with `!simple-release/set-options` command takes priority - JSON must be valid, otherwise the command will be ignored - Parameters apply only to the current release execution - The command can be updated by editing the comment or adding a new one </details> <!-- Please do not edit this comment. simple-release-pull-request: true simple-release-branch-from: release simple-release-branch-to: main --> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
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.
Why the v0.44.0 release failed
The Release and Validate runs on the
chore(release): 0.44.0merge failed on two tests intests/helpers/vscode-settings.test.ts— the same tree had passed CI 13 minutes earlier. Root cause is an order-dependent test-pollution flake:init-project.test.tswrote the runner's REAL VS Code user settings. The"vscode returns auto-installed"test drives the realconfigureVscodeSettings→addMarketplaceToUserSettings, writing the marketplace URL into~/.config/Code/User/settings.json(and into developers' real settings on local runs).vscode-settings.test.ts'sHOMEoverride is silently ineffective on Linux. Bun cachesos.homedir(), so the tests actually read/wrote the real settings file — passing only while it didn't exist.vscode-settingsran beforeinit-project. Failing run: after — reading the polluted file (the failure diff shows exactly the strayarchgate/vscode.gitentry).No v0.44.0 tag/release was created — merging this re-triggers the Release workflow and publishes 0.44.0.
Commit 1 — fix the failing tests (test-only, no production changes)
init-project.test.ts: spy outconfigureVscodeSettings(ARCH-005 first-partyspyOnpattern) soinitProjectnever writes user-scope state.vscode-settings.test.ts: replaceHOMEenv overrides withspyOn(os, "homedir").mockReturnValue(tempDir)— verified the spy intercepts the named import insidevscode-settings.ts.APPDATAenv override stays for the Windows branch (readsBun.envat call time).restoreEnv()helper deletes originally-unset env keys instead ofObject.assignstringifyingundefined.Commit 2 — sweep all other tests for the same pattern
Audited every test overriding
HOME/USERPROFILE/APPDATA/XDG_*against what production actually reads. Env overrides are safe for allpaths.ts-based resolvers (readBun.envat call time) — no changes needed for clean/upgrade/auth/binary-upgrade/credential-store/sentry/telemetry/update-check/opencode tests. Fixed the rest:session-context.test.ts/session-context-cursor.test.ts: wrote real~/.claude/projectsand~/.cursor/projectsas a documented workaround for homedir caching → nowspyOn(os, "homedir")into mkdtemp dirs.session-context-copilot.test.ts: wrote real~/.copilot/session-state→ nowBun.env.HOMEoverride (resolver readsBun.envat call time).plugin-install.test.ts(worst case):installCursorPlugin/installOpencodePlugintests mocked fetch andBun.spawnbut not the filesystem prep — every run created dirs under and deletedarchgate-*agents/skills from the developer's real~/.cursorand~/.config/opencode. Now redirectsHOME/XDG_CONFIG_HOMEto a mkdtemp dir file-wide.Commit 3 — remove the last first-party
mock.moduleplugin-install.test.tsglobally replaced the wholeplatformmodule (process-global, not undone bymock.restore()) — the same order-dependent flake class, explicitly forbidden by ARCH-005. Converted to per-testspyOn(platform, "resolveCommand"). Full suite passes, confirming nothing depended on the leak.Validation
bun run validategreen after each commit (1281 tests, 39/39 ADR checks, knip, build check)src/helpers/plugin-install.ts:170uses.text()+JSON.parseinstead of.json()Summary by CodeRabbit
Documentation
Tests