[issues/488] Raise coverage thresholds to >=95% on rangelink-vscode-extension#495
Merged
Conversation
added 14 commits
March 23, 2026 21:27
…s for logBasedUiAssertions `getLogCapture.ts` depends on `vscode.extensions` (VS Code host only) and was dragging down Jest coverage metrics with 0% across all metrics. Excluded it from `collectCoverageFrom` — targeted exclusion, not the entire `__integration-tests__/` directory. `logBasedUiAssertions.ts` is the assertion safety net for all log-based integration tests. Extended the existing test file with tests for `assertSuppressionLogged` (new function from PR #491 feedback) and `parseLogContext` edge cases (invalid JSON, missing fn, non-string fn, non-object parse results). The file is now at 100% coverage across all metrics.
The builder functions passed `compareWith`, `isAvailable`, and `getUserInstruction` callbacks into `ComposablePasteDestination` but no test ever invoked them — leaving 6 branches and 7 functions uncovered despite high line coverage. Added tests that invoke each callback through the built destination's public interface: `equals()` for compareWith, `isAvailable()` for availability delegation, and `getUserInstruction()` for both Success (returns undefined) and Failure (returns instruction string) branches. All three AI assistant builders (Cursor, Claude Code, GitHub Copilot) now have availability spy tests verifying delegation to the detection utilities. `destinationBuilders.ts` is now at 100% across all coverage metrics.
BookmarksStore had 65.78% branch coverage — all the `catch` branches for save failures and unexpected errors were untested. These branches are the safety net that prevents unhandled rejections from propagating to the extension host. Added tests for both error paths: `globalState.update` rejection (triggers the `save()` catch which wraps as `BOOKMARK_SAVE_FAILED` with operation details and cause), and non-`RangeLinkExtensionError` throws (triggers the generic catch in each CRUD method). Also covered the `error instanceof Error` ternary edge case where a non-Error value is thrown. BookmarksStore is now at 100/92/100/100 (branches at 92.1%, up from 65.78%).
…r paths The entire `executeWithUri` path (explorer context menu "Bind Here") was untested — the tab group scanning loop, multi-group picker, and single-group focus were all uncovered. The `getPlaceholder` callback in the file picker was also never invoked. Added 5 tests covering all `executeWithUri` branches (no matching groups, single group, multi-group picker selection, picker cancellation) and the `getPlaceholder` callback invocation. `BindToTextEditorCommand.ts` is now at 100/93.75/100/100 (up from 76.78/62.5/75/78.18).
`FocusCapabilityFactory` had 25% function coverage (only `createAIAssistantCapability` called), `TerminalFocusCapability` had no test for its error catch branch, and `EditorFocusCapability.resolveViewColumn` had an untested fallback when `editor.viewColumn` is undefined. Added `FocusCapabilityFactory.test.ts` (verifies all 3 factory methods return correct capability types), `TerminalFocusCapability.test.ts` (success + error catch branch), and the undefined-viewColumn fallback test in `EditorFocusCapability.test.ts`. All capabilities files now at 100% across all individual coverage metrics.
`SelectionValidator` was extracted from `RangeLinkService` in #493 but had no dedicated test file — 0% branches, 16.66% functions. Added tests covering all three public methods: `validateSelectionsAndShowError` (valid selections, no editor, empty selections), `mapSelectionsForLogging` (populated and empty arrays), and `getLineContentAtSelectionBoundaries` (in-bounds, out-of-bounds, lineAt throws, negative line numbers). `SelectionValidator.ts` is now at 100% across all coverage metrics.
…icker overflow pickers The `getPlaceholder` callbacks in secondary terminal/file pickers were never invoked during tests — they were passed to `showTerminalPicker`/`showFilePicker` but the mock implementations only called `onSelected`. Added `getPlaceholder()` invocation and literal placeholder assertions to existing overflow picker tests in both `RangeLinkStatusBar` and `DestinationPicker`. Both files now at 100% function coverage. Only the exhaustive `never` type guards remain uncovered (unreachable by design).
Preparation for extracting registration wiring into a separate file. Command objects (ShowVersionCommand, BindToDestinationCommand, JumpToDestinationCommand, GoToRangeLinkCommand, ListBookmarksCommand, ManageBookmarksCommand) were created inline within the registration block — moved them to the bootstrap section so the registration block contains only `context.subscriptions.push(...)` calls.
…scriptions.ts extension.ts was 649 lines mixing bootstrap, dependency construction, and registration wiring. This made it hard to navigate and impossible to unit test without mocking the full activation lifecycle. Moved the entire object graph (config, services, commands, providers) and all `context.subscriptions.push(...)` calls into `wireSubscriptions()`. The `ExtensionDependencies` interface has only 3 fields — `ideAdapter`, `logger`, `versionInfo` — everything else is built internally. extension.ts is now 67 lines: logger init, version info, adapter creation, locale setup, and the single `wireSubscriptions()` call. Benefits: - extension.ts is a minimal bootstrap file — reads top to bottom in seconds - wireSubscriptions.ts is independently unit-testable with just 3 mock dependencies - New commands/services are added in one place without touching bootstrap
…stration Verifies that `wireSubscriptions` registers the complete set of 50 commands and 4 link providers. Uses a real `VscodeAdapter` backed by mock VSCode — calls `wireSubscriptions` with the 3-field `ExtensionDependencies` and asserts against spy calls. Provider tests go beyond counts: they verify exact provider types via `instanceof` (FilePathTerminalProvider, RangeLinkTerminalProvider, FilePathDocumentProvider, RangeLinkDocumentProvider) and document selectors (`file` + `untitled` schemes). Command count is frozen as a literal to catch accidental deletions from the EXPECTED_COMMANDS array.
… for testable wiring `wireSubscriptions` built the entire object graph and registered closures in one function — making closures untestable (services constructed internally, registrations coupled to `context`). This kept function coverage at 85% with no path to 90%. Separated concerns into three layers following the VscodeAdapter pattern: - `SubscriptionRegistrar`: wraps `context.subscriptions.push(ideAdapter.registerCommand(...))` behind a testable interface - `createWiringServices`: factory that builds the full service object graph from 3 foundational deps - `wireSubscriptions`: pure wiring — receives a registrar and pre-built services, registers closures Tests inject a mock registrar (captures handlers for invocation) and mock services (verifiable delegation). Added 30 closure delegation tests covering all service methods, URI ternary branches, and representative commands from each pattern. Functions coverage crossed the 90% target. Coverage thresholds raised to 97/95/90/97 — all acceptance criteria met. Benefits: - Every command closure is independently testable via `registrar.getHandler(CMD_X)()` - `wireSubscriptions` is now 100% branches (pure wiring, no construction logic) - `extension.ts` stays 67 lines — unchanged from the previous extraction
…from coverage
Added the remaining 13 closure delegation tests — every command handler now verifies the correct service method is called with the correct arguments. `wireSubscriptions.ts` is at 100% across all metrics.
Excluded barrel `index.ts` re-export files from coverage collection — Istanbul counts each `export {}` as a function, inflating the denominator with phantom uncovered functions despite 100% line coverage. This removed ~85 false-negative functions from the count.
Coverage thresholds raised to 98/95/95/98 (up from 97/95/90/97).
|
New command behavior introduced for binding files to text editors, including a new placeholder for the file picker. Suggested test cases:
Generated by QA Gap Check (GPT-4o-mini via GitHub Models) |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughRefactors extension startup by extracting subscription registration and service construction into new modules ( Changes
Sequence Diagram(s)sequenceDiagram
participant Activator as activate()
participant Registrar as SubscriptionRegistrar
participant Services as createWiringServices
participant Wiring as wireSubscriptions
participant IDE as VscodeAdapter
Activator->>Registrar: createSubscriptionRegistrar(context, ideAdapter)
Activator->>Services: createWiringServices(deps, context)
Activator->>Wiring: wireSubscriptions(registrar, services)
Wiring->>Registrar: registerCommand(...), registerTerminalLinkProvider(...), registerDocumentLinkProvider(...)
Registrar->>IDE: ideAdapter.registerCommand / register*Provider (push disposables to context.subscriptions)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
packages/rangelink-vscode-extension/src/__tests__/destinations/capabilities/FocusCapabilityFactory.test.ts (1)
14-18: Consider usingbeforeEachfor test isolation.The mocks and factory instance are created at module scope, meaning all tests share the same instances. While Jest's
clearMocks/resetMocks/restoreMockssettings handle mock state, the factory itself may accumulate internal state across tests.For better isolation, consider:
describe('FocusCapabilityFactory', () => { - const mockLogger = createMockLogger(); - const mockAdapter = createMockVscodeAdapter(); - const mockClipboardPreserver = createMockClipboardPreserver(); - const factory = new FocusCapabilityFactory(mockAdapter, mockClipboardPreserver, mockLogger); + let mockLogger: ReturnType<typeof createMockLogger>; + let mockAdapter: ReturnType<typeof createMockVscodeAdapter>; + let mockClipboardPreserver: ReturnType<typeof createMockClipboardPreserver>; + let factory: FocusCapabilityFactory; + + beforeEach(() => { + mockLogger = createMockLogger(); + mockAdapter = createMockVscodeAdapter(); + mockClipboardPreserver = createMockClipboardPreserver(); + factory = new FocusCapabilityFactory(mockAdapter, mockClipboardPreserver, mockLogger); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rangelink-vscode-extension/src/__tests__/destinations/capabilities/FocusCapabilityFactory.test.ts` around lines 14 - 18, Tests currently instantiate mockLogger, mockAdapter, mockClipboardPreserver and the FocusCapabilityFactory at module scope, risking shared state across tests; change this to declare variables (e.g., mockLogger, mockAdapter, mockClipboardPreserver, factory) at top-level and initialize them inside a beforeEach block where you call createMockLogger(), createMockVscodeAdapter(), createMockClipboardPreserver() and new FocusCapabilityFactory(mockAdapter, mockClipboardPreserver, mockLogger) so each test gets fresh instances; keep any Jest mock reset config but ensure factory-specific state is isolated by re-creating these objects in beforeEach.packages/rangelink-vscode-extension/src/__tests__/destinations/destinationBuilders.test.ts (1)
312-322: Remove unnecessary type assertion by properly typing the spy helper.The
as nevercast is a workaround for the untypedjest.SpyInstancereturn inspyOnIsGitHubCopilotChatAvailable. The spy helper should declare generic type parameters matching the spied function's signature (Promise<boolean>return,[VscodeAdapter, Logger]parameters) so TypeScript can infer the correct type for.mockResolvedValue().Update the spy helper in
packages/rangelink-vscode-extension/src/__tests__/helpers/spyOnIsGitHubCopilotChatAvailable.tsto properly type its return value, then remove theas nevercast from the test.Suggested fix
-export const spyOnIsGitHubCopilotChatAvailable = (): jest.SpyInstance => +export const spyOnIsGitHubCopilotChatAvailable = (): jest.SpyInstance<Promise<boolean>, [VscodeAdapter, Logger]> =>Then in the test:
- const spy = spyOnIsGitHubCopilotChatAvailable().mockResolvedValue(true as never); + const spy = spyOnIsGitHubCopilotChatAvailable().mockResolvedValue(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rangelink-vscode-extension/src/__tests__/destinations/destinationBuilders.test.ts` around lines 312 - 322, The test uses a typed workaround `as never` because spyOnIsGitHubCopilotChatAvailable returns an untyped jest.SpyInstance; fix the spy helper in spyOnIsGitHubCopilotChatAvailable.ts to declare generic types matching the spied function signature (return Promise<boolean> and parameters [VscodeAdapter, Logger]) so its return type is Jest's SpyInstance<Promise<boolean>, [VscodeAdapter, Logger]> and then remove the `as never` cast from the test that calls spyOnIsGitHubCopilotChatAvailable in destinationBuilders.test.ts (the test that builds the GitHub Copilot Chat destination and calls destination.isAvailable()).packages/rangelink-vscode-extension/src/bookmarks/__tests__/BookmarksStore.test.ts (1)
682-725: Consider adding logger assertions for save failure tests.The
addsave failure test (Lines 676-679) correctly verifiesmockLogger.erroris called, but theupdate,remove, andrecordAccesssave failure tests omit this assertion. Per thesave()method implementation (context snippet 1), all operations log tologger.errorbefore throwing.For consistency and per coding guidelines, consider adding logger assertions:
expect(result).toBeRangeLinkExtensionErrorErr('BOOKMARK_SAVE_FAILED', { message: 'Failed to persist bookmarks during update', functionName: 'BookmarksStore.update', details: { operation: 'update' }, cause: saveError, }); + expect(mockLogger.error).toHaveBeenCalledWith( + { fn: 'BookmarksStore.update', error: saveError, syncEnabled: true }, + 'Failed to save bookmarks during update', + );As per coding guidelines: "Always test logging behavior - Include logger assertions in tests that verify method behavior."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rangelink-vscode-extension/src/bookmarks/__tests__/BookmarksStore.test.ts` around lines 682 - 725, Add assertions that mockLogger.error was called when globalState.update rejects in the BookmarksStore.update, BookmarksStore.remove, and BookmarksStore.recordAccess tests: after calling store.update(TEST_ID,...), store.remove(TEST_ID), and store.recordAccess(TEST_ID) respectively, assert mockLogger.error was invoked with the same or equivalent error/message as in the add-save-failure test; reference the mockLogger used in these tests and the save() behavior in BookmarksStore to ensure logging is asserted for each operation (update, remove, recordAccess).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/rangelink-vscode-extension/src/__tests__/helpers/spyOnIsCursorIDEDetected.ts`:
- Around line 1-4: The test helper spyOnIsCursorIDEDetected currently spies on
the namespace export but destinationBuilders imports isCursorIDEDetected as a
named binding, so the spy never intercepts calls; fix by choosing one approach:
either (A) call jest.resetModules() in the test, require the module under test
after creating the spy from spyOnIsCursorIDEDetected so destinationBuilders
picks up the spied binding, or (B) add a module-level
jest.mock('../../utils/aiAssistants/isCursorIDEDetected') at the top of the test
file so isCursorIDEDetected is mocked for imports, or (C) refactor
destinationBuilders to accept isCursorIDEDetected as an injected dependency;
reference the helper spyOnIsCursorIDEDetected, the function isCursorIDEDetected,
and destinationBuilders to locate the places to change.
In
`@packages/rangelink-vscode-extension/src/services/__tests__/SelectionValidator.test.ts`:
- Around line 80-87: Replace the partial matcher in the test by asserting the
exact object passed to mockLogger.warn instead of using expect.objectContaining;
specifically update the assertion in SelectionValidator.test.ts to call
expect(mockLogger.warn).toHaveBeenCalledWith(theExactLogObject, 'Selection
validation failed - full diagnostic context') where theExactLogObject matches
the full payload emitted by SelectionValidator.validateSelectionsAndShowError
(include fn: 'SelectionValidator.validateSelectionsAndShowError', hasEditor:
true, errorCode: 'ERROR_NO_TEXT_SELECTED' and any other fields the
implementation sends) so the test performs an exact equality check rather than a
partial match.
- Around line 51-58: Replace the partial matcher expect.objectContaining(...) in
the assertion against mockLogger.warn with an exact object equality check that
includes all expected fields from the emitted log context for
SelectionValidator.validateSelectionsAndShowError; update the test to call
expect(mockLogger.warn).toHaveBeenCalledWith(theExactLogObject, 'Selection
validation failed - full diagnostic context') where theExactLogObject contains
every property the real logger is passed (fn, hasEditor, errorCode and any other
properties the implementation supplies) so the test asserts the complete log
context instead of using expect.objectContaining; reference the
SelectionValidator.validateSelectionsAndShowError call flow and mockLogger.warn
to construct theExactLogObject.
- Around line 34-38: Replace the partial matcher with an exact object assertion:
in the test that calls mockLogger.debug, assert the full log context object
literal instead of expect.objectContaining — e.g., call
expect(mockLogger.debug).toHaveBeenCalledWith(theExactContextObject, 'Selection
validation starting'); locate the assertion around
SelectionValidator.validateSelectionsAndShowError and ensure the exact context
object includes the same keys/values the implementation passes (fn:
'SelectionValidator.validateSelectionsAndShowError', plus any other properties)
and that argument order matches the real call.
---
Nitpick comments:
In
`@packages/rangelink-vscode-extension/src/__tests__/destinations/capabilities/FocusCapabilityFactory.test.ts`:
- Around line 14-18: Tests currently instantiate mockLogger, mockAdapter,
mockClipboardPreserver and the FocusCapabilityFactory at module scope, risking
shared state across tests; change this to declare variables (e.g., mockLogger,
mockAdapter, mockClipboardPreserver, factory) at top-level and initialize them
inside a beforeEach block where you call createMockLogger(),
createMockVscodeAdapter(), createMockClipboardPreserver() and new
FocusCapabilityFactory(mockAdapter, mockClipboardPreserver, mockLogger) so each
test gets fresh instances; keep any Jest mock reset config but ensure
factory-specific state is isolated by re-creating these objects in beforeEach.
In
`@packages/rangelink-vscode-extension/src/__tests__/destinations/destinationBuilders.test.ts`:
- Around line 312-322: The test uses a typed workaround `as never` because
spyOnIsGitHubCopilotChatAvailable returns an untyped jest.SpyInstance; fix the
spy helper in spyOnIsGitHubCopilotChatAvailable.ts to declare generic types
matching the spied function signature (return Promise<boolean> and parameters
[VscodeAdapter, Logger]) so its return type is Jest's
SpyInstance<Promise<boolean>, [VscodeAdapter, Logger]> and then remove the `as
never` cast from the test that calls spyOnIsGitHubCopilotChatAvailable in
destinationBuilders.test.ts (the test that builds the GitHub Copilot Chat
destination and calls destination.isAvailable()).
In
`@packages/rangelink-vscode-extension/src/bookmarks/__tests__/BookmarksStore.test.ts`:
- Around line 682-725: Add assertions that mockLogger.error was called when
globalState.update rejects in the BookmarksStore.update, BookmarksStore.remove,
and BookmarksStore.recordAccess tests: after calling store.update(TEST_ID,...),
store.remove(TEST_ID), and store.recordAccess(TEST_ID) respectively, assert
mockLogger.error was invoked with the same or equivalent error/message as in the
add-save-failure test; reference the mockLogger used in these tests and the
save() behavior in BookmarksStore to ensure logging is asserted for each
operation (update, remove, recordAccess).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e8353d21-9c5a-45b4-a372-d78528a5184b
📒 Files selected for processing (24)
packages/rangelink-vscode-extension/jest.config.jspackages/rangelink-vscode-extension/src/SubscriptionRegistrar.tspackages/rangelink-vscode-extension/src/__tests__/commands/BindToTextEditorCommand.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/DestinationPicker.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/capabilities/EditorFocusCapability.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/capabilities/FocusCapabilityFactory.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/capabilities/TerminalFocusCapability.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/destinationBuilders.test.tspackages/rangelink-vscode-extension/src/__tests__/helpers/createMockInsertFactory.tspackages/rangelink-vscode-extension/src/__tests__/helpers/createMockPosition.tspackages/rangelink-vscode-extension/src/__tests__/helpers/createMockSubscriptionRegistrar.tspackages/rangelink-vscode-extension/src/__tests__/helpers/createMockWiringServices.tspackages/rangelink-vscode-extension/src/__tests__/helpers/index.tspackages/rangelink-vscode-extension/src/__tests__/helpers/spyOnIsClaudeCodeAvailable.tspackages/rangelink-vscode-extension/src/__tests__/helpers/spyOnIsCursorIDEDetected.tspackages/rangelink-vscode-extension/src/__tests__/helpers/spyOnIsGitHubCopilotChatAvailable.tspackages/rangelink-vscode-extension/src/__tests__/integration-helpers/logBasedUiAssertions.test.tspackages/rangelink-vscode-extension/src/__tests__/statusBar/RangeLinkStatusBar.test.tspackages/rangelink-vscode-extension/src/__tests__/wireSubscriptions.test.tspackages/rangelink-vscode-extension/src/bookmarks/__tests__/BookmarksStore.test.tspackages/rangelink-vscode-extension/src/createWiringServices.tspackages/rangelink-vscode-extension/src/extension.tspackages/rangelink-vscode-extension/src/services/__tests__/SelectionValidator.test.tspackages/rangelink-vscode-extension/src/wireSubscriptions.ts
Comment on lines
+1
to
+4
| import * as isCursorIDEDetectedModule from '../../utils/aiAssistants/isCursorIDEDetected'; | ||
|
|
||
| export const spyOnIsCursorIDEDetected = (): jest.SpyInstance => | ||
| jest.spyOn(isCursorIDEDetectedModule, 'isCursorIDEDetected'); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how isCursorIDEDetected is imported in destinationBuilders.ts and whether tests rely on this spy
rg -n "isCursorIDEDetected" --type=ts -C2Repository: couimet/rangeLink
Length of output: 15860
🏁 Script executed:
#!/bin/bash
# Check for jest.resetModules, jest.mock, or beforeEach setup in the test file
rg -A5 -B5 "jest\.resetModules|jest\.mock.*isCursorIDEDetected|beforeEach" packages/rangelink-vscode-extension/src/__tests__/destinations/destinationBuilders.test.ts | head -100Repository: couimet/rangeLink
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Read the destinationBuilders.test.ts file, especially around the spy test
cat -n packages/rangelink-vscode-extension/src/__tests__/destinations/destinationBuilders.test.ts | head -250Repository: couimet/rangeLink
Length of output: 11450
🏁 Script executed:
#!/bin/bash
# Check if the test has any skip markers or if there's a jest.resetModules anywhere in the test file
grep -n "skip\|only\|jest.resetModules" packages/rangelink-vscode-extension/src/__tests__/destinations/destinationBuilders.test.tsRepository: couimet/rangeLink
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check how utils/index.ts exports isCursorIDEDetected
cat -n packages/rangelink-vscode-extension/src/utils/index.ts | head -30Repository: couimet/rangeLink
Length of output: 1024
🏁 Script executed:
#!/bin/bash
# Check if there's a jest setup file or beforeEach in the test file that might reset modules
tail -n +200 packages/rangelink-vscode-extension/src/__tests__/destinations/destinationBuilders.test.ts | head -100Repository: couimet/rangeLink
Length of output: 4135
🏁 Script executed:
#!/bin/bash
# Check jest configuration and whether babel/ts-jest is being used
find packages/rangelink-vscode-extension -name "jest.config.*" -o -name "tsconfig.json" | xargs cat -n | head -100Repository: couimet/rangeLink
Length of output: 2152
🏁 Script executed:
#!/bin/bash
# Let's check if there are any jest.isolateModules or if modules are being reloaded in tests
# Also check if the spyOnIsCursorIDEDetected helper is imported in a way that might affect module loading order
grep -n "isolateModules\|jest.resetModules" packages/rangelink-vscode-extension -r --include="*.ts" --include="*.js"Repository: couimet/rangeLink
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check if the test file marks this test as skipped or if there are comments about it
sed -n '209,216p' packages/rangelink-vscode-extension/src/__tests__/destinations/destinationBuilders.test.tsRepository: couimet/rangeLink
Length of output: 435
🏁 Script executed:
#!/bin/bash
# Let's look at the actual implementation of buildCursorAIDestination to see how it uses isCursorIDEDetected
sed -n '150,165p' packages/rangelink-vscode-extension/src/destinations/destinationBuilders.tsRepository: couimet/rangeLink
Length of output: 743
🏁 Script executed:
#!/bin/bash
# Let's verify by checking how the module is structured - specifically, when is isCursorIDEDetected actually called?
# Look at what gets captured in the closure
cat -n packages/rangelink-vscode-extension/src/destinations/destinationBuilders.ts | head -30Repository: couimet/rangeLink
Length of output: 1473
🏁 Script executed:
#!/bin/bash
# Let's check if spyOnIsCursorIDEDetected is used anywhere else and if those tests pass
rg -n "spyOnIsCursorIDEDetected" packages/rangelink-vscode-extension --type=ts -B2 -A5Repository: couimet/rangeLink
Length of output: 3668
🏁 Script executed:
#!/bin/bash
# Check the setup file to see if it does any module reset
cat packages/rangelink-vscode-extension/src/__tests__/setup/matchers.tsRepository: couimet/rangeLink
Length of output: 2025
🏁 Script executed:
#!/bin/bash
# Let's check if there are any TODO or SKIP comments near the spy test, or if the test passes with current setup
# Also check if similar spies (spyOnIsClaudeCodeAvailable, spyOnIsGitHubCopilotChatAvailable) exist and how they're implemented
cat packages/rangelink-vscode-extension/src/__tests__/helpers/spyOnIsClaudeCodeAvailable.tsRepository: couimet/rangeLink
Length of output: 290
🏁 Script executed:
#!/bin/bash
# Let's trace through the actual module loading. Check if there's any documentation or comments about how these spies work
# Also check if the test file itself imports destinationBuilders at module level or lazily
head -30 packages/rangelink-vscode-extension/src/__tests__/destinations/destinationBuilders.test.tsRepository: couimet/rangeLink
Length of output: 887
🏁 Script executed:
#!/bin/bash
# Check if there's a jest setup that does module reloading or if we can find any evidence the test runs
# Let's look for a package.json test script to see what's actually running
cat packages/rangelink-vscode-extension/package.json | grep -A5 '"test"'Repository: couimet/rangeLink
Length of output: 595
🏁 Script executed:
#!/bin/bash
# Let me trace through one more thing - when does the test file get imported relative to the spy creation?
# Also check if there's a beforeAll that might reset modules
grep -n "beforeAll\|beforeEach\|afterEach\|afterAll" packages/rangelink-vscode-extension/src/__tests__/destinations/destinationBuilders.test.tsRepository: couimet/rangeLink
Length of output: 43
Spy cannot intercept calls from modules using named imports without module reloading.
The spyOnIsCursorIDEDetected helper uses jest.spyOn() on the namespace object, but destinationBuilders.ts imports isCursorIDEDetected via named destructuring (import { isCursorIDEDetected } from '../utils'). This captures a direct reference at module load time. The spy only affects the namespace property, not the captured binding, so calls from destinationBuilders won't be intercepted.
The test at line 209 expects spy.toHaveBeenCalledTimes(1), but this assertion will fail because the spy is never actually called. To fix this, one of the following approaches is needed:
- Use
jest.resetModules()and re-import the test subject after creating the spy - Use module-level
jest.mock('../../utils/aiAssistants/isCursorIDEDetected')at the test file top - Refactor to inject
isCursorIDEDetectedas a dependency into the destination builder context
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/rangelink-vscode-extension/src/__tests__/helpers/spyOnIsCursorIDEDetected.ts`
around lines 1 - 4, The test helper spyOnIsCursorIDEDetected currently spies on
the namespace export but destinationBuilders imports isCursorIDEDetected as a
named binding, so the spy never intercepts calls; fix by choosing one approach:
either (A) call jest.resetModules() in the test, require the module under test
after creating the spy from spyOnIsCursorIDEDetected so destinationBuilders
picks up the spied binding, or (B) add a module-level
jest.mock('../../utils/aiAssistants/isCursorIDEDetected') at the top of the test
file so isCursorIDEDetected is mocked for imports, or (C) refactor
destinationBuilders to accept isCursorIDEDetected as an injected dependency;
reference the helper spyOnIsCursorIDEDetected, the function isCursorIDEDetected,
and destinationBuilders to locate the places to change.
… logger assertions, exact matchers Addressed 4 accepted items from the CodeRabbit review: - FocusCapabilityFactory.test.ts: moved mock creation into `beforeEach` for test isolation - spyOnIsGitHubCopilotChatAvailable.ts: added generic type params (`jest.SpyInstance<Promise<boolean>, [VscodeAdapter, Logger]>`) so `mockResolvedValue` infers correctly — removed `as never` cast from destinationBuilders.test.ts - BookmarksStore.test.ts: added `mockLogger.error` assertions to update, remove, and recordAccess save failure tests (T007 compliance) - SelectionValidator.test.ts: replaced 3 `expect.objectContaining()` calls with exact object literals (T004 compliance) Ignored Feedback: - spyOnIsCursorIDEDetected interception concern: false positive — CommonJS module identity ensures the spy intercepts calls through barrel re-exports. All 3 spy tests pass and verify `toHaveBeenCalledTimes(1)`. Ref: #495 (review)
Owner
Author
|
@coderabbitai review I've been rate limited in #495 (comment) |
Contributor
|
🧠 Learnings used✅ Actions performedReview triggered.
|
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
Raises all four coverage thresholds to >=95% (up from 88-94%) by adding targeted tests for uncovered branches, extracting
extension.tsinto a testable architecture, and excluding coverage tool artifacts. Final thresholds: 98/95/95/98 (statements/branches/functions/lines).Changes
getLogCapture.ts(VS Code host dependency) and barrelindex.tsre-export files (Istanbul artifact) fromcollectCoverageFromcreateWiringServices.tsand all registration wiring intowireSubscriptions.tscontext.subscriptions.push(ideAdapter.registerCommand(...)), following the VscodeAdapter patterncreateWiringServices()builds the full service object graph from 3 foundational deps (ideAdapter,logger,versionInfo)compareWith,isAvailable,getUserInstructioncallbacks now invoked and verifiedexecuteWithUri(multi-tab-group picker) andgetPlaceholdercallback coveredTerminalFocusCapabilityerror path,FocusCapabilityFactoryfactory methods,EditorFocusCapabilityundefined viewColumn fallbackgetPlaceholdercallbacks in overflow pickers now invoked and assertedassertSuppressionLoggedandparseLogContextedge casescreateMockInsertFactorymade generic,createMockPosshorthand,spyOnIs*Availablehelpers,createMockSubscriptionRegistrar,createMockWiringServicesRelated
Summary by CodeRabbit
Chores
Tests