Repository navigation
test(auto-update-checker): add unit tests for getLocalDevPath and findPluginEntry - #553
Conversation
…dPluginEntry Co-Authored-By: Giulio Vaccari <io@giuliovaccari.it>
WalkthroughA new Vitest test file is introduced that tests the checker module's functionality for local development mode detection and plugin configuration parsing. The tests utilize mocked Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 (1)
src/hooks/auto-update-checker/checker.test.ts (1)
89-134: Consider adding thefile://branch case forfindPluginEntry.
findPluginEntryalso handlesfile://...entries insrc/hooks/auto-update-checker/checker.ts:124-126; adding one test would fully cover that decision branch.➕ Optional test addition
describe("findPluginEntry", () => { @@ it("returns isPinned=false for `@latest` entry", async () => { @@ expect(result!.pinnedVersion).toBeNull(); }); + + it("returns unpinned entry for file:// plugin path", async () => { + const { findPluginEntry } = await import("./checker"); + fsMock.existsSync.mockImplementation((p: string) => p.endsWith("opencode.json")); + fsMock.readFileSync.mockReturnValue( + JSON.stringify({ + plugin: ["file:///home/user/opencode-antigravity-auth/dist/plugin.js"], + }), + ); + const result = findPluginEntry("/project"); + expect(result).not.toBeNull(); + expect(result!.isPinned).toBe(false); + expect(result!.pinnedVersion).toBeNull(); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/auto-update-checker/checker.test.ts` around lines 89 - 134, Add a test case for findPluginEntry that covers the file:// branch: mock fs.existsSync to return true for opencode.json and fs.readFileSync to return JSON with a plugin entry using a file:// URL (e.g., "file:///some/local/path" or "file://./local-plugin"); then call findPluginEntry("/project") and assert the returned entry is not null, that isPinned is false and pinnedVersion is null (or other expected behavior for file:// entries) to ensure the file:// decision branch in findPluginEntry is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/hooks/auto-update-checker/checker.test.ts`:
- Around line 89-134: Add a test case for findPluginEntry that covers the
file:// branch: mock fs.existsSync to return true for opencode.json and
fs.readFileSync to return JSON with a plugin entry using a file:// URL (e.g.,
"file:///some/local/path" or "file://./local-plugin"); then call
findPluginEntry("/project") and assert the returned entry is not null, that
isPinned is false and pinnedVersion is null (or other expected behavior for
file:// entries) to ensure the file:// decision branch in findPluginEntry is
covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2bd02c32-699e-41cf-b816-7a4759e06524
📒 Files selected for processing (1)
src/hooks/auto-update-checker/checker.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🔇 Additional comments (2)
src/hooks/auto-update-checker/checker.test.ts (2)
5-15: Solid FS mock setup for deterministic tests.Using
vi.hoistedwithvi.mock("node:fs", ...)is clean here and ensures imports of./checkerconsistently hit the mocked filesystem.
18-87: Great coverage of local-dev path parsing edge cases.These tests cover happy path + failure modes (missing config, JSONC comments/trailing commas, malformed JSON) and validate the non-throw contract well.
Greptile SummaryAdds a new test file for Confidence Score: 4/5Safe to merge — test-only change with no production code modifications and all assertions verified against the implementation. Only P2 style/coverage suggestions; no logic errors or correctness bugs were found. Score is 4 (P2s only ceiling). No files require special attention. Important Files Changed
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/hooks/auto-update-checker/checker.test.ts
Line: 89-93
Comment:
**Missing `afterEach` cleanup in second describe block**
The `findPluginEntry` describe block omits the `afterEach(() => vi.restoreAllMocks())` that is present in the first describe block. While `vi.resetAllMocks()` in `beforeEach` handles the important cleanup (resetting implementations and return values), the inconsistency may cause confusion and could matter if spy-based mocks are added here later.
```suggestion
beforeEach(() => {
vi.resetAllMocks();
fsMock.existsSync.mockReturnValue(false);
});
afterEach(() => {
vi.restoreAllMocks();
});
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/hooks/auto-update-checker/checker.test.ts
Line: 31-35
Comment:
**Dynamic `import` inside tests does not re-isolate the module**
Each `await import("./checker")` call returns the same cached module instance — Vitest does not reload the module per-test unless `vi.resetModules()` is called. The tests work correctly here because `getLocalDevPath`/`findPluginEntry` carry no module-level mutable state and only depend on the `fsMock` object that is reset in `beforeEach`. However, the repeated imports create a misleading impression that each test gets a fresh module. Consider hoisting the imports to the describe scope or calling `vi.resetModules()` in `beforeEach` if true per-test isolation is ever needed.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/hooks/auto-update-checker/checker.test.ts
Line: 31-35
Comment:
**No positive test for `isLocalDevMode` returning `true`**
The test suite for `isLocalDevMode` only verifies the `false` path. There is no test that asserts `isLocalDevMode` returns `true` when a matching `file://` entry is found in the config. The `getLocalDevPath` returning a non-null value is implicitly covered by a later test, but `isLocalDevMode` itself is never asserted to return `true`, leaving that branch untested.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "test(auto-update-checker): add unit test..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| describe("findPluginEntry", () => { | ||
| beforeEach(() => { | ||
| vi.resetAllMocks(); | ||
| fsMock.existsSync.mockReturnValue(false); | ||
| }); |
There was a problem hiding this comment.
afterEach cleanup in second describe block
The findPluginEntry describe block omits the afterEach(() => vi.restoreAllMocks()) that is present in the first describe block. While vi.resetAllMocks() in beforeEach handles the important cleanup (resetting implementations and return values), the inconsistency may cause confusion and could matter if spy-based mocks are added here later.
| describe("findPluginEntry", () => { | |
| beforeEach(() => { | |
| vi.resetAllMocks(); | |
| fsMock.existsSync.mockReturnValue(false); | |
| }); | |
| beforeEach(() => { | |
| vi.resetAllMocks(); | |
| fsMock.existsSync.mockReturnValue(false); | |
| }); | |
| afterEach(() => { | |
| vi.restoreAllMocks(); | |
| }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/hooks/auto-update-checker/checker.test.ts
Line: 89-93
Comment:
**Missing `afterEach` cleanup in second describe block**
The `findPluginEntry` describe block omits the `afterEach(() => vi.restoreAllMocks())` that is present in the first describe block. While `vi.resetAllMocks()` in `beforeEach` handles the important cleanup (resetting implementations and return values), the inconsistency may cause confusion and could matter if spy-based mocks are added here later.
```suggestion
beforeEach(() => {
vi.resetAllMocks();
fsMock.existsSync.mockReturnValue(false);
});
afterEach(() => {
vi.restoreAllMocks();
});
```
How can I resolve this? If you propose a fix, please make it concise.| }); | ||
|
|
||
| it("returns null from getLocalDevPath when no config exists", async () => { | ||
| const { getLocalDevPath } = await import("./checker"); | ||
| expect(getLocalDevPath("/some/project")).toBeNull(); |
There was a problem hiding this comment.
import inside tests does not re-isolate the module
Each await import("./checker") call returns the same cached module instance — Vitest does not reload the module per-test unless vi.resetModules() is called. The tests work correctly here because getLocalDevPath/findPluginEntry carry no module-level mutable state and only depend on the fsMock object that is reset in beforeEach. However, the repeated imports create a misleading impression that each test gets a fresh module. Consider hoisting the imports to the describe scope or calling vi.resetModules() in beforeEach if true per-test isolation is ever needed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/hooks/auto-update-checker/checker.test.ts
Line: 31-35
Comment:
**Dynamic `import` inside tests does not re-isolate the module**
Each `await import("./checker")` call returns the same cached module instance — Vitest does not reload the module per-test unless `vi.resetModules()` is called. The tests work correctly here because `getLocalDevPath`/`findPluginEntry` carry no module-level mutable state and only depend on the `fsMock` object that is reset in `beforeEach`. However, the repeated imports create a misleading impression that each test gets a fresh module. Consider hoisting the imports to the describe scope or calling `vi.resetModules()` in `beforeEach` if true per-test isolation is ever needed.
How can I resolve this? If you propose a fix, please make it concise.| }); | ||
|
|
||
| it("returns null from getLocalDevPath when no config exists", async () => { | ||
| const { getLocalDevPath } = await import("./checker"); | ||
| expect(getLocalDevPath("/some/project")).toBeNull(); |
There was a problem hiding this comment.
isLocalDevMode returning true
The test suite for isLocalDevMode only verifies the false path. There is no test that asserts isLocalDevMode returns true when a matching file:// entry is found in the config. The getLocalDevPath returning a non-null value is implicitly covered by a later test, but isLocalDevMode itself is never asserted to return true, leaving that branch untested.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/hooks/auto-update-checker/checker.test.ts
Line: 31-35
Comment:
**No positive test for `isLocalDevMode` returning `true`**
The test suite for `isLocalDevMode` only verifies the `false` path. There is no test that asserts `isLocalDevMode` returns `true` when a matching `file://` entry is found in the config. The `getLocalDevPath` returning a non-null value is implicitly covered by a later test, but `isLocalDevMode` itself is never asserted to return `true`, leaving that branch untested.
How can I resolve this? If you propose a fix, please make it concise.
1 participant
Summary
Adds unit tests for the auto-update checker in
src/hooks/auto-update-checker/checker.test.ts, covering two exported functions.isLocalDevMode/getLocalDevPath(6 tests) — verifies behaviour when no config file exists, when the config has nofile://plugin entry, when a matchingfile://path is present, when the config is JSONC with comments and trailing commas, and when the file contains malformed JSON (should returnnullwithout throwing).findPluginEntry(4 tests) — verifies it returnsnullwhen no config exists,isPinned: falsefor a bare package name,isPinned: truewith the correct version for a pinned entry like@1.5.0, andisPinned: falsefor@latest.The filesystem is fully mocked using
vi.hoisted+vi.mock("node:fs", ...)so tests run without touching the real disk. No production code was changed. All 922 tests pass.