test: fix unit test isolation via dependency injection#96
Open
test: fix unit test isolation via dependency injection#96
Conversation
Resolves module mock leakage that was causing test failures in gates.test.ts, skills.test.ts, and other tests when run after ssh.test.ts or remote-cmd.test.ts.
Root cause: Bun's mock.module() creates global mocks that persist beyond the test file's lifecycle, even with mock.restore() in afterAll. When ssh.test.ts mocked 'fs' and 'child_process', subsequent tests that needed real filesystem operations would fail.
Solution: Replace global module mocking with dependency injection:
- src/ssh.ts: Export _setSshDepsForTest() to inject test doubles for execFileSync/statSync
- src/commands/remote.ts: Export _setRemoteCommandDepsForTest() for fs/os/fetch/Bun.write
- tests/unit/ssh.test.ts: Use DI instead of mock.module()
- tests/unit/remote-cmd.test.ts: Use DI instead of mock.module('fs')
- tests/unit/skills.test.ts: Added droid adapter test case for .factory/skills projection
Fixes:
- src/instructions.ts: Add droid .factory/skills directory mapping
- src/skills.ts: Install droid skills to .factory/skills (native discovery)
Validates with:
bun test tests/unit/ssh.test.ts tests/unit/remote-cmd.test.ts tests/unit/skills.test.ts tests/unit/gates.test.ts
All 64 tests pass (previously 9 failures)
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
📝 WalkthroughWalkthroughThis PR adds Droid as a new CLI adapter with skills installed under ChangesDroid CLI Support and Testability Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
tests/unit/ssh.test.ts (1)
133-136: 💤 Low value
mock.restore()is vestigial after the DI refactorThere are no
mock.module()calls left in this file, somock.restore()is a no-op. It's harmless but could mislead future readers into thinking there are module-level mocks in play.🧹 Proposed cleanup
afterAll(() => { _setSshDepsForTest(null) - mock.restore() })🤖 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/unit/ssh.test.ts` around lines 133 - 136, Remove the vestigial mock.restore() call in the afterAll block because there are no mock.module() usages in this test file; update the afterAll to only call _setSshDepsForTest(null) (leave _setSshDepsForTest as-is) and remove any reference to mock.restore to avoid confusion about module-level mocks (locate the afterAll in tests/unit/ssh.test.ts and edit the afterAll that currently calls mock.restore()).src/commands/remote.ts (1)
22-34: 💤 Low value
_setRemoteCommandDepsForTestlacks JSDoc, unlike its peer insrc/ssh.tsBoth
_setSshDepsForTestand_setRemoteCommandDepsForTestare exported public symbols. Adding a brief JSDoc noting that this is a test-only hook helps prevent accidental production use and satisfies the project's documentation guideline.As per coding guidelines: "Document all public APIs and functions with JSDoc comments".
📝 Proposed JSDoc
+/** + * Test-only hook: injects or resets the filesystem/network dependencies used + * by remote commands. Pass `null` to restore all implementations to their + * real defaults. + */ export function _setRemoteCommandDepsForTest(deps: {🤖 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 `@src/commands/remote.ts` around lines 22 - 34, Add a JSDoc comment above the exported function _setRemoteCommandDepsForTest stating it is a test-only dependency-injection hook for replacing filesystem/network/write implementations in tests, mirroring the style and wording used for _setSshDepsForTest in src/ssh.ts; include a brief description, list the parameter (deps) and mark the function as internal/test-only (e.g., `@internal` or `@deprecated` for production use) so it's clear this is not for production consumption.tests/unit/skills.test.ts (1)
190-204: ⚡ Quick winDroid test is missing marker assertions in
AGENTS.mdThe Droid test only checks
content.toContain('- my-skill: A test skill')but omits the<!-- flt:skills:start -->/<!-- flt:skills:end -->marker assertions that the equivalentcodextest validates (lines 174–176). Without the markers,cleanupSkillswon't be able to strip the block fromAGENTS.md, making the test blind to a regression in that path.✏️ Proposed addition
const content = readFileSync(join(workDir, 'AGENTS.md'), 'utf-8') + expect(content).toContain('<!-- flt:skills:start -->') + expect(content).toContain('<!-- flt:skills:end -->') expect(content).toContain('- my-skill: A test skill')🤖 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/unit/skills.test.ts` around lines 190 - 204, The Droid unit test for projectSkills is missing assertions that AGENTS.md contains the skill markers, so update the test (the one calling projectSkills(workDir, droidAdapter, { requested: ['my-skill'] })) to also assert that the file includes the marker comments "<!-- flt:skills:start -->" and "<!-- flt:skills:end -->" (the same markers checked in the codex test) so cleanupSkills can locate and remove the block; reference the AGENTS.md read via readFileSync and add expect(content).toContain for both markers around the existing '- my-skill: A test skill' assertion.src/skills.ts (1)
261-286: ⚡ Quick winMissing
cleanupSkillstest for the Droid adapter
cleanupSkillsexercises the samecliName !== 'claude-code' && cliName !== 'opencode'branch for Droid (removing.factory/skillsmanifest entries and stripping theAGENTS.mdmarker block), but there is no test that validates this path. The existing cleanup tests coverclaude-codeandcodexonly.Would you like me to draft the missing
cleanupSkillstest for the Droid adapter?🤖 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 `@src/skills.ts` around lines 261 - 286, Tests are missing to exercise the Droid-specific branch in cleanupSkills; add a unit test that sets adapter.cliName = 'droid' (or constructs an adapter with cliName 'droid') and verifies cleanupSkills removes .factory/skills manifest entries and strips the SKILLS_MARKER_START/END block from the instruction file (same behavior covered for claude-code/codex). Specifically, create fixtures that install skills under join('.factory','skills') (or emulate install via installAt/selected) and an instruction file containing the SKILLS_MARKER_START/END block, then call cleanupSkills and assert that manifests no longer reference the skills and that the file no longer contains the marker block; reference the cleanupSkills function, adapter.cliName, SKILLS_MARKER_START/SKILLS_MARKER_END, and installAt to locate where to hook the test.src/ssh.ts (1)
14-20: 💤 Low value
_setSshDepsForTestis missing a JSDoc commentConsistent with the project's documentation guideline and the analogous function in
src/commands/remote.ts, this exported test hook should carry a brief JSDoc explaining its test-only purpose.As per coding guidelines: "Document all public APIs and functions with JSDoc comments".
📝 Proposed JSDoc
+/** + * Test-only hook: injects or resets the child-process/filesystem dependencies + * used by SSH helpers. Pass `null` to restore all implementations to their + * real defaults. + */ export function _setSshDepsForTest(deps: {🤖 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 `@src/ssh.ts` around lines 14 - 20, Add a JSDoc comment above the exported test hook _setSshDepsForTest describing that it is a test-only API to inject dependencies (execFileSync and statSync implementations) for unit tests, noting parameters and side effects (it assigns execFileSyncImpl and statSyncImpl) and that it should not be used in production code; mirror the style and briefness of the analogous function in src/commands/remote.ts so the project documentation guideline is satisfied.
🤖 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.
Nitpick comments:
In `@src/commands/remote.ts`:
- Around line 22-34: Add a JSDoc comment above the exported function
_setRemoteCommandDepsForTest stating it is a test-only dependency-injection hook
for replacing filesystem/network/write implementations in tests, mirroring the
style and wording used for _setSshDepsForTest in src/ssh.ts; include a brief
description, list the parameter (deps) and mark the function as
internal/test-only (e.g., `@internal` or `@deprecated` for production use) so it's
clear this is not for production consumption.
In `@src/skills.ts`:
- Around line 261-286: Tests are missing to exercise the Droid-specific branch
in cleanupSkills; add a unit test that sets adapter.cliName = 'droid' (or
constructs an adapter with cliName 'droid') and verifies cleanupSkills removes
.factory/skills manifest entries and strips the SKILLS_MARKER_START/END block
from the instruction file (same behavior covered for claude-code/codex).
Specifically, create fixtures that install skills under
join('.factory','skills') (or emulate install via installAt/selected) and an
instruction file containing the SKILLS_MARKER_START/END block, then call
cleanupSkills and assert that manifests no longer reference the skills and that
the file no longer contains the marker block; reference the cleanupSkills
function, adapter.cliName, SKILLS_MARKER_START/SKILLS_MARKER_END, and installAt
to locate where to hook the test.
In `@src/ssh.ts`:
- Around line 14-20: Add a JSDoc comment above the exported test hook
_setSshDepsForTest describing that it is a test-only API to inject dependencies
(execFileSync and statSync implementations) for unit tests, noting parameters
and side effects (it assigns execFileSyncImpl and statSyncImpl) and that it
should not be used in production code; mirror the style and briefness of the
analogous function in src/commands/remote.ts so the project documentation
guideline is satisfied.
In `@tests/unit/skills.test.ts`:
- Around line 190-204: The Droid unit test for projectSkills is missing
assertions that AGENTS.md contains the skill markers, so update the test (the
one calling projectSkills(workDir, droidAdapter, { requested: ['my-skill'] }))
to also assert that the file includes the marker comments "<!-- flt:skills:start
-->" and "<!-- flt:skills:end -->" (the same markers checked in the codex test)
so cleanupSkills can locate and remove the block; reference the AGENTS.md read
via readFileSync and add expect(content).toContain for both markers around the
existing '- my-skill: A test skill' assertion.
In `@tests/unit/ssh.test.ts`:
- Around line 133-136: Remove the vestigial mock.restore() call in the afterAll
block because there are no mock.module() usages in this test file; update the
afterAll to only call _setSshDepsForTest(null) (leave _setSshDepsForTest as-is)
and remove any reference to mock.restore to avoid confusion about module-level
mocks (locate the afterAll in tests/unit/ssh.test.ts and edit the afterAll that
currently calls mock.restore()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e189c4fa-4c78-4f4b-9dbf-2afd5ab18ed6
📒 Files selected for processing (8)
DROID-OAUTH-PROXY.mdsrc/commands/remote.tssrc/instructions.tssrc/skills.tssrc/ssh.tstests/unit/remote-cmd.test.tstests/unit/skills.test.tstests/unit/ssh.test.ts
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.
Resolves module mock leakage causing test failures when run after ssh.test.ts or remote-cmd.test.ts.
Problem
Bun's
mock.module()creates global mocks that persist beyond the test file's lifecycle, even withmock.restore()inafterAll. When ssh.test.ts mocked 'fs' and 'child_process', subsequent tests needing real filesystem operations would fail (gates.test.ts, skills.test.ts, etc.).Solution
Replaced global module mocking with dependency injection pattern:
Source changes:
src/ssh.ts: Export_setSshDepsForTest()to inject test doubles for execFileSync/statSyncsrc/commands/remote.ts: Export_setRemoteCommandDepsForTest()for fs/os/fetch/Bun.writesrc/instructions.ts: Add droid.factory/skillsdirectory mappingsrc/skills.ts: Install droid skills to.factory/skills(native discovery)Test changes:
tests/unit/ssh.test.ts: Use DI instead of mock.module()tests/unit/remote-cmd.test.ts: Use DI instead of mock.module('fs')tests/unit/skills.test.ts: Added droid adapter test caseValidation
Full unit suite now shows 98/102 files passing (4 pre-existing failures unrelated to isolation issue).
Summary by CodeRabbit
New Features
Documentation