[side-quest/integration-test-utilities-and-cleanup] Integration test utilities and copy-pasta cleanup#618
Open
couimet wants to merge 12 commits into
Open
[side-quest/integration-test-utilities-and-cleanup] Integration test utilities and copy-pasta cleanup#618couimet wants to merge 12 commits into
couimet wants to merge 12 commits into
Conversation
…utilities and copy-pasta cleanup Adds reusable assertion helpers (`withClipboardSentinel`, `withClipboardChanged`, `withClipboardRestored`, `assertClipboardEqualsGeneratedLink`, `assertTerminalBufferEqualsGeneratedLink`, `assertTerminalBufferContainsGeneratedLink`, `assertClipboardPreservedAndTerminalLink`, `assertLogContains`) and converts all call sites across test suite files to use them. ## Changes - New assertion helpers in `clipboardHelpers.ts`, `capturingPtyHelpers.ts`, `logBasedUiAssertions.ts`, and `logHelpers.ts` eliminate repeated clipboard/terminal/log assertion boilerplate - Converted integration test suites to use the new helpers, reducing copy-pasta and making test intent more readable - Fixed broken tests and smart padding issues uncovered during the conversion campaign - Updated CLAUDE.md with shell-quoting gotcha and `:with-extensions --grep` documentation - Removed `expectContextKeys` calls from test suites — those belong to the ContextKeyService extraction (PR #3) and would fail without that infrastructure ## Test Plan - [ ] All 1982 unit tests pass - [ ] Integration tests pass: `pnpm test:release:automated` - [ ] Integration tests pass with extensions: `pnpm test:release:with-extensions`
|
New error codes related to clipboard operations were introduced, and existing test cases were modified to reflect changes in clipboard behavior. Additional test cases may be needed to cover new error handling. Suggested test cases:
Generated by QA Gap Check (GPT-4o-mini via GitHub Models) |
This comment has been minimized.
This comment has been minimized.
… a raw value (it was padded)
✅ CI / Integration Tests (with overrides) — run summaryDuration: 1m 6s QA TC IDs: 2 exercised across 1 features Report: View run & artifacts Reproduce locally: Feature breakdown
|
✅ CI / Integration Tests (automated) — run summaryDuration: 11m 1s QA TC IDs: 157 exercised across 0 features Report: View run & artifacts Reproduce locally: Feature breakdown
|
✅ CI / Integration Tests (with extensions) — run summaryDuration: 12m 20s Unit tests: Ran in separate Test & Validate job QA TC IDs: 189 exercised across 25 features Report: View run & artifacts Reproduce locally: Feature breakdown
|
Owner
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
WalkthroughThis PR replaces clipboard-preserver plumbing with a new ChangesClipboard and terminal routing refactor
Sequence Diagram(s)sequenceDiagram
participant SendRouter
participant ClipboardService
participant TerminalPasteService
participant VscodeAdapter
SendRouter->>ClipboardService: route clipboard work
TerminalPasteService->>ClipboardService: stage clipboard text
TerminalPasteService->>VscodeAdapter: showTerminal and executeCommand
ClipboardService->>VscodeAdapter: read/write/restore clipboard
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![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__/helpers/createMockTerminalPasteService.ts (1)
5-8: ⚡ Quick winAvoid
unknowndouble-cast in test helper mocks.
as unknown as ...suppresses interface drift checks. Prefer a typed mock object (orsatisfies) so added service methods fail at compile time.Suggested patch
export const createMockTerminalPasteService = (): jest.Mocked<TerminalPasteService> => - ({ + ({ pasteIntoTerminal: jest.fn().mockResolvedValue(Result.ok(undefined)), - }) as unknown as jest.Mocked<TerminalPasteService>; + }) satisfies jest.Mocked<TerminalPasteService>;🤖 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 `@packages/rangelink-vscode-extension/src/__tests__/helpers/createMockTerminalPasteService.ts` around lines 5 - 8, The helper createMockTerminalPasteService uses an unsafe "as unknown as jest.Mocked<TerminalPasteService>" double-cast which hides interface drift; replace it with a properly typed mock object so missing/extra methods fail at compile time—e.g., construct a const mock: jest.Mocked<TerminalPasteService> (or use the TypeScript "satisfies" operator) and assign pasteIntoTerminal: jest.fn().mockResolvedValue(Result.ok(undefined)), then return that mock from createMockTerminalPasteService; reference TerminalPasteService, createMockTerminalPasteService, and pasteIntoTerminal to locate the change.packages/rangelink-vscode-extension/src/__integration-tests__/suite/builtInAiAssistants.test.ts (1)
1010-1019: ⚡ Quick winApply assertLogContains consistently across validation tests.
The
claude-code-007test (lines 901-905) was updated to use the newassertLogContainshelper, but this nearly identical validation test for Gemini still uses manual log scanning. Apply the same refactor here for consistency.♻️ Refactor to match claude-code-007 pattern
- const lines = logCapture.getLinesSince('before-gc-006'); - const warningLog = lines.find( - (line) => - line.includes('coldStartDelayMs must be greater than coldRefocusIntervalMs') && - line.includes('using defaults'), - ); - assert.ok( - warningLog, - 'Expected validation warning log with "using defaults" when coldStartDelayMs <= coldRefocusIntervalMs', - ); + assertLogContains( + 'before-gc-006', + 'coldStartDelayMs must be greater than coldRefocusIntervalMs', + 'Expected validation warning log when coldStartDelayMs <= coldRefocusIntervalMs', + );🤖 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 `@packages/rangelink-vscode-extension/src/__integration-tests__/suite/builtInAiAssistants.test.ts` around lines 1010 - 1019, Replace the manual log scanning in the Gemini validation test with the shared helper: find the test block that currently calls logCapture.getLinesSince('before-gc-006') and assert.ok(...) and use the assertLogContains helper (same usage as in the claude-code-007 test) to assert the presence of the warning "coldStartDelayMs must be greater than coldRefocusIntervalMs" together with "using defaults"; update the test to call assertLogContains(logCapture, 'before-gc-006', 'coldStartDelayMs must be greater than coldRefocusIntervalMs', 'using defaults') (or the project's existing helper signature) so the assertion is consistent and more readable.packages/rangelink-vscode-extension/src/__integration-tests__/suite/smartPadding.test.ts (1)
373-412: Clarify smart-padding-009 pasteLink setup vs default
smart-padding-009doesn’t explicitly setrangelink.smartPadding.pasteLink, butpackages/rangelink-vscode-extension/package.jsonsets its default to"both", so the test’spasteLink=bothtitle andgetGeneratedLink(..., { smartPad: 'both' })assertion are consistent.- Add an explicit
smartPadding.pasteLink = "both"(or rename the test to “default”) to make the intent resilient to future default changes.🤖 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 `@packages/rangelink-vscode-extension/src/__integration-tests__/suite/smartPadding.test.ts` around lines 373 - 412, The test smart-padding-009 relies on the default smartPadding.pasteLink value being "both" but doesn't set it explicitly; update the test (inside the smart-padding-009 block, before calling CMD_COPY_LINK_RELATIVE) to explicitly set the extension setting rangelink.smartPadding.pasteLink to "both" (e.g., via the test harness or VS Code workspace configuration API) so the assertion using getGeneratedLink('before-pad-009', { smartPad: 'both' }) and the test title remain correct even if defaults change.
🤖 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.
Inline comments:
In `@packages/rangelink-core-ts/src/errors/RangeLinkErrorCodes.ts`:
- Around line 20-28: Within the Clipboard errors block of
RangeLinkErrorCodes.ts, reorder the enum members so they are alphabetically
sorted: move CLIPBOARD_FN_EXECUTION_FAILED to come immediately after
CLIPBOARD_CAPTURE_EXECUTION_FAILED and before CLIPBOARD_READ_FAILED; ensure the
Clipboard section (CLIPBOARD_CAPTURE_EXECUTION_FAILED,
CLIPBOARD_FN_EXECUTION_FAILED, CLIPBOARD_READ_FAILED,
CLIPBOARD_RESTORE_WRITE_FAILED, CLIPBOARD_STAGE_WRITE_FAILED) follows the file's
"alphabetical order within each category" rule.
In
`@packages/rangelink-vscode-extension/src/__integration-tests__/suite/clipboardPreservation.test.ts`:
- Around line 370-374: Replace the substring-only assertions that look for
"Focus failed, cannot paste link" with structured-context checks: capture the
same logged line(s), call parseLogContext(...) on the captured log entry and
assert the resulting ctx.reason === 'COMMAND_FOCUS_FAILED' and ctx.fn (or
equivalent function identifier emitted by
ComposablePasteDestination.performPaste) matches the paste operation; do this
for the assertions currently around the focus-failure check (the block
exercising ComposablePasteDestination.performPaste) and the second occurrence so
the test verifies the structured context instead of only the message substring.
In
`@packages/rangelink-vscode-extension/src/__tests__/destinations/capabilities/insertFactories/terminalInsertFactory.test.ts`:
- Around line 58-83: The test for TerminalInsertFactory.forTarget currently
verifies pasteIntoTerminal calls but omits logger assertions; update the test to
assert that mockLogger.info was called for each insert with the appropriate
messages/arguments (one for 'content for terminal 1' targeting terminal1 and one
for 'content for terminal 2' targeting terminal2), and keep the existing
mockLogger.error assertion; locate the test using TerminalInsertFactory,
forTarget, insertFn1/insertFn2 and add expectations on mockLogger.info to match
the per-terminal success log contract.
In
`@packages/rangelink-vscode-extension/src/__tests__/helpers/createMockClipboardService.ts`:
- Around line 7-16: The mock ClipboardService functions stage, route, and
capture currently allow rejected callback promises to bubble as thrown
rejections; update each to catch errors and return a Result.err(...) instead of
throwing so the mock matches the real service contract — specifically modify the
stage mock (jest.fn((_text, fn) => ...)), the route mock (jest.fn((fn,
_shouldRestore) => ...)) and the capture mock (jest.fn((producer) => ...)) to
wrap their fn()/producer() calls with .then(...).catch(err => Result.err(err))
and ensure the resolved success path still returns Result.ok(...).
---
Nitpick comments:
In
`@packages/rangelink-vscode-extension/src/__integration-tests__/suite/builtInAiAssistants.test.ts`:
- Around line 1010-1019: Replace the manual log scanning in the Gemini
validation test with the shared helper: find the test block that currently calls
logCapture.getLinesSince('before-gc-006') and assert.ok(...) and use the
assertLogContains helper (same usage as in the claude-code-007 test) to assert
the presence of the warning "coldStartDelayMs must be greater than
coldRefocusIntervalMs" together with "using defaults"; update the test to call
assertLogContains(logCapture, 'before-gc-006', 'coldStartDelayMs must be greater
than coldRefocusIntervalMs', 'using defaults') (or the project's existing helper
signature) so the assertion is consistent and more readable.
In
`@packages/rangelink-vscode-extension/src/__integration-tests__/suite/smartPadding.test.ts`:
- Around line 373-412: The test smart-padding-009 relies on the default
smartPadding.pasteLink value being "both" but doesn't set it explicitly; update
the test (inside the smart-padding-009 block, before calling
CMD_COPY_LINK_RELATIVE) to explicitly set the extension setting
rangelink.smartPadding.pasteLink to "both" (e.g., via the test harness or VS
Code workspace configuration API) so the assertion using
getGeneratedLink('before-pad-009', { smartPad: 'both' }) and the test title
remain correct even if defaults change.
In
`@packages/rangelink-vscode-extension/src/__tests__/helpers/createMockTerminalPasteService.ts`:
- Around line 5-8: The helper createMockTerminalPasteService uses an unsafe "as
unknown as jest.Mocked<TerminalPasteService>" double-cast which hides interface
drift; replace it with a properly typed mock object so missing/extra methods
fail at compile time—e.g., construct a const mock:
jest.Mocked<TerminalPasteService> (or use the TypeScript "satisfies" operator)
and assign pasteIntoTerminal: jest.fn().mockResolvedValue(Result.ok(undefined)),
then return that mock from createMockTerminalPasteService; reference
TerminalPasteService, createMockTerminalPasteService, and pasteIntoTerminal to
locate the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 31cf6a5f-cba8-441e-888e-c337c1d30f71
📒 Files selected for processing (72)
CLAUDE.mdpackages/rangelink-core-ts/src/errors/RangeLinkErrorCodes.tspackages/rangelink-vscode-extension/qa/qa-test-cases-unreleased.yamlpackages/rangelink-vscode-extension/src/__integration-tests__/helpers/capturingPtyHelpers.tspackages/rangelink-vscode-extension/src/__integration-tests__/helpers/clipboardHelpers.tspackages/rangelink-vscode-extension/src/__integration-tests__/helpers/logBasedUiAssertions.tspackages/rangelink-vscode-extension/src/__integration-tests__/helpers/logHelpers.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/bindToDestination.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/builtInAiAssistants.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/clipboardPreservation.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/contextMenuEditorContent.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/contextMenuEditorTab.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/contextMenuExplorer.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/contextMenuTerminal.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/coreSendCommands.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/customAiAssistants.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/dirtyBufferWarning.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/linkGeneration.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/sendFilePath.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/smartPadding.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/textEditorDestination.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/unbind.test.tspackages/rangelink-vscode-extension/src/__tests__/clipboard/ClipboardService.test.tspackages/rangelink-vscode-extension/src/__tests__/clipboard/DefaultClipboardPreserver.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/ComposablePasteDestination.integration.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/capabilities/insertFactories/terminalInsertFactory.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/destinationBuilders.test.tspackages/rangelink-vscode-extension/src/__tests__/feedback/OperationFeedbackProvider.test.tspackages/rangelink-vscode-extension/src/__tests__/helpers/createMockClipboardPreserver.tspackages/rangelink-vscode-extension/src/__tests__/helpers/createMockClipboardService.tspackages/rangelink-vscode-extension/src/__tests__/helpers/createMockClipboardWriter.tspackages/rangelink-vscode-extension/src/__tests__/helpers/createMockOperationFeedbackProvider.tspackages/rangelink-vscode-extension/src/__tests__/helpers/createMockPasteDestinationForSendRouter.tspackages/rangelink-vscode-extension/src/__tests__/helpers/createMockTerminalPasteService.tspackages/rangelink-vscode-extension/src/__tests__/helpers/index.tspackages/rangelink-vscode-extension/src/__tests__/ide/vscode/VscodeAdapter.test.tspackages/rangelink-vscode-extension/src/__tests__/services/FilePathPaster.test.tspackages/rangelink-vscode-extension/src/__tests__/services/LinkGenerator.test.tspackages/rangelink-vscode-extension/src/__tests__/services/SendRouter.test.tspackages/rangelink-vscode-extension/src/__tests__/services/TerminalPasteService.test.tspackages/rangelink-vscode-extension/src/__tests__/services/TerminalSelectionService.test.tspackages/rangelink-vscode-extension/src/__tests__/utils/clipboardPreservation.test.tspackages/rangelink-vscode-extension/src/__tests__/utils/validateTerminalDefined.test.tspackages/rangelink-vscode-extension/src/clipboard/ClipboardPreserver.tspackages/rangelink-vscode-extension/src/clipboard/ClipboardService.tspackages/rangelink-vscode-extension/src/clipboard/DefaultClipboardPreserver.tspackages/rangelink-vscode-extension/src/clipboard/index.tspackages/rangelink-vscode-extension/src/constants/aiAssistantPasteConstants.tspackages/rangelink-vscode-extension/src/createWiringServices.tspackages/rangelink-vscode-extension/src/destinations/capabilities/FocusCapabilityFactory.tspackages/rangelink-vscode-extension/src/destinations/capabilities/TerminalFocusCapability.tspackages/rangelink-vscode-extension/src/destinations/capabilities/insertFactories/terminalInsertFactory.tspackages/rangelink-vscode-extension/src/destinations/destinationBuilders.tspackages/rangelink-vscode-extension/src/errors/RangeLinkExtensionErrorCodes.tspackages/rangelink-vscode-extension/src/feedback/OperationFeedbackProvider.tspackages/rangelink-vscode-extension/src/feedback/types/PasteSendOutcome.tspackages/rangelink-vscode-extension/src/i18n/messages.en.tspackages/rangelink-vscode-extension/src/ide/TerminalPasteAdapter.tspackages/rangelink-vscode-extension/src/ide/index.tspackages/rangelink-vscode-extension/src/ide/vscode/VscodeAdapter.tspackages/rangelink-vscode-extension/src/services/FilePathPaster.tspackages/rangelink-vscode-extension/src/services/LinkGenerator.tspackages/rangelink-vscode-extension/src/services/SendRouter.tspackages/rangelink-vscode-extension/src/services/TerminalPasteService.tspackages/rangelink-vscode-extension/src/services/TerminalSelectionService.tspackages/rangelink-vscode-extension/src/services/index.tspackages/rangelink-vscode-extension/src/types/MessageCode.tspackages/rangelink-vscode-extension/src/utils/clipboardPreservation.tspackages/rangelink-vscode-extension/src/utils/index.tspackages/rangelink-vscode-extension/src/utils/validateTerminalDefined.ts
💤 Files with no reviewable changes (8)
- packages/rangelink-vscode-extension/src/tests/clipboard/DefaultClipboardPreserver.test.ts
- packages/rangelink-vscode-extension/src/clipboard/ClipboardPreserver.ts
- packages/rangelink-vscode-extension/src/tests/utils/clipboardPreservation.test.ts
- packages/rangelink-vscode-extension/src/clipboard/DefaultClipboardPreserver.ts
- packages/rangelink-vscode-extension/src/tests/helpers/createMockClipboardPreserver.ts
- packages/rangelink-vscode-extension/src/integration-tests/suite/contextMenuExplorer.test.ts
- packages/rangelink-vscode-extension/src/utils/clipboardPreservation.ts
- packages/rangelink-vscode-extension/src/integration-tests/suite/contextMenuEditorTab.test.ts
Comment on lines
+20
to
+28
| // | ||
| // Clipboard errors | ||
| // | ||
| CLIPBOARD_CAPTURE_EXECUTION_FAILED = 'CLIPBOARD_CAPTURE_EXECUTION_FAILED', | ||
| CLIPBOARD_READ_FAILED = 'CLIPBOARD_READ_FAILED', | ||
| CLIPBOARD_RESTORE_WRITE_FAILED = 'CLIPBOARD_RESTORE_WRITE_FAILED', | ||
| CLIPBOARD_STAGE_WRITE_FAILED = 'CLIPBOARD_STAGE_WRITE_FAILED', | ||
| CLIPBOARD_FN_EXECUTION_FAILED = 'CLIPBOARD_FN_EXECUTION_FAILED', | ||
|
|
Contributor
There was a problem hiding this comment.
Clipboard error codes are not alphabetically sorted.
The file's header documentation (line 17) states "Please keep alphabetical order within each category for ease of maintenance." Within the Clipboard errors category, the correct alphabetical order should be:
CLIPBOARD_CAPTURE_EXECUTION_FAILEDCLIPBOARD_FN_EXECUTION_FAILED← should be here (not at end)CLIPBOARD_READ_FAILEDCLIPBOARD_RESTORE_WRITE_FAILEDCLIPBOARD_STAGE_WRITE_FAILED
📋 Proposed fix to restore alphabetical order
//
// Clipboard errors
//
CLIPBOARD_CAPTURE_EXECUTION_FAILED = 'CLIPBOARD_CAPTURE_EXECUTION_FAILED',
+ CLIPBOARD_FN_EXECUTION_FAILED = 'CLIPBOARD_FN_EXECUTION_FAILED',
CLIPBOARD_READ_FAILED = 'CLIPBOARD_READ_FAILED',
CLIPBOARD_RESTORE_WRITE_FAILED = 'CLIPBOARD_RESTORE_WRITE_FAILED',
CLIPBOARD_STAGE_WRITE_FAILED = 'CLIPBOARD_STAGE_WRITE_FAILED',
- CLIPBOARD_FN_EXECUTION_FAILED = 'CLIPBOARD_FN_EXECUTION_FAILED',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // | |
| // Clipboard errors | |
| // | |
| CLIPBOARD_CAPTURE_EXECUTION_FAILED = 'CLIPBOARD_CAPTURE_EXECUTION_FAILED', | |
| CLIPBOARD_READ_FAILED = 'CLIPBOARD_READ_FAILED', | |
| CLIPBOARD_RESTORE_WRITE_FAILED = 'CLIPBOARD_RESTORE_WRITE_FAILED', | |
| CLIPBOARD_STAGE_WRITE_FAILED = 'CLIPBOARD_STAGE_WRITE_FAILED', | |
| CLIPBOARD_FN_EXECUTION_FAILED = 'CLIPBOARD_FN_EXECUTION_FAILED', | |
| // | |
| // Clipboard errors | |
| // | |
| CLIPBOARD_CAPTURE_EXECUTION_FAILED = 'CLIPBOARD_CAPTURE_EXECUTION_FAILED', | |
| CLIPBOARD_FN_EXECUTION_FAILED = 'CLIPBOARD_FN_EXECUTION_FAILED', | |
| CLIPBOARD_READ_FAILED = 'CLIPBOARD_READ_FAILED', | |
| CLIPBOARD_RESTORE_WRITE_FAILED = 'CLIPBOARD_RESTORE_WRITE_FAILED', | |
| CLIPBOARD_STAGE_WRITE_FAILED = 'CLIPBOARD_STAGE_WRITE_FAILED', |
🤖 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 `@packages/rangelink-core-ts/src/errors/RangeLinkErrorCodes.ts` around lines 20
- 28, Within the Clipboard errors block of RangeLinkErrorCodes.ts, reorder the
enum members so they are alphabetically sorted: move
CLIPBOARD_FN_EXECUTION_FAILED to come immediately after
CLIPBOARD_CAPTURE_EXECUTION_FAILED and before CLIPBOARD_READ_FAILED; ensure the
Clipboard section (CLIPBOARD_CAPTURE_EXECUTION_FAILED,
CLIPBOARD_FN_EXECUTION_FAILED, CLIPBOARD_READ_FAILED,
CLIPBOARD_RESTORE_WRITE_FAILED, CLIPBOARD_STAGE_WRITE_FAILED) follows the file's
"alphabetical order within each category" rule.
Comment on lines
+370
to
374
| assertLogContains( | ||
| 'before-010', | ||
| 'Focus failed, cannot paste link', | ||
| 'Expected "Focus failed, cannot paste link" log — the focus command should have thrown', | ||
| ); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if focus failure logs have structured JSON fields that should be asserted
# Search for where "Focus failed" message is logged
rg -nP --type=ts -C3 'Focus failed' packages/rangelink-vscode-extension/src/
# Look for the log call that produces this message
ast-grep --pattern $'logger.$_($$$ "Focus failed" $$$)'Repository: couimet/rangeLink
Length of output: 5099
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the logger.warn call emitting "Focus failed, cannot paste ..."
sed -n '320,380p' packages/rangelink-vscode-extension/src/destinations/ComposablePasteDestination.ts
# Show the relevant test assertions
sed -n '340,430p' packages/rangelink-vscode-extension/src/__integration-tests__/suite/clipboardPreservation.test.ts
# Find how assertLogContains / parseLogContext are implemented/used
rg -n "assertLogContains|parseLogContext" packages/rangelink-vscode-extension/src/__integration-tests__/suite/clipboardPreservation.test.ts packages/rangelink-vscode-extension/src/__integration-tests__/ -SRepository: couimet/rangeLink
Length of output: 9215
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate any existing usage of parseLogContext in this integration-test suite
rg -n "parseLogContext" packages/rangelink-vscode-extension/src/__integration-tests__/suite -S
# Locate patterns in other tests asserting the same reason (COMMAND_FOCUS_FAILED) with structured fields
rg -n "COMMAND_FOCUS_FAILED|reason: 'COMMAND_FOCUS_FAILED'|reason: focusResult\\.error\\.reason" packages/rangelink-vscode-extension/src -SRepository: couimet/rangeLink
Length of output: 43
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect how assertLogContains works
sed -n '1,220p' packages/rangelink-vscode-extension/src/__integration-tests__/helpers/logHelpers.ts
# Inspect how parseLogContext works and what structured fields it extracts
sed -n '1,260p' packages/rangelink-vscode-extension/src/__integration-tests__/helpers/logBasedUiAssertions.ts
# Also inspect any structured assertions that already exist (search within the helpers file)
rg -n "assert.*Log|toHave.*Called|parseLogContext\\(|reason|COMMAND_FOCUS_FAILED|Focus failed" packages/rangelink-vscode-extension/src/__integration-tests__/helpers/logBasedUiAssertions.tsRepository: couimet/rangeLink
Length of output: 12480
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect how logs are captured (to understand line format for parseLogContext)
sed -n '1,200p' packages/rangelink-vscode-extension/src/__integration-tests__/helpers/getLogCapture.ts
# Inspect rest of logBasedUiAssertions for any helper used for substring-vs-structured checks
sed -n '260,520p' packages/rangelink-vscode-extension/src/__integration-tests__/helpers/logBasedUiAssertions.ts
# Inspect clipboardPreservation.test.ts for existing parseLogContext usage patterns
sed -n '1,240p' packages/rangelink-vscode-extension/src/__integration-tests__/suite/clipboardPreservation.test.ts
# Inspect how other tests assert similar focus-failure logs (COMMAND_FOCUS_FAILED)
rg -n "COMMAND_FOCUS_FAILED|Focus failed, cannot paste link" packages/rangelink-vscode-extension/src/__tests__/destinations packages/rangelink-vscode-extension/src/__integration-tests__/suite -S
# Inspect ComposablePasteDestination around performPaste for the exact context fields passed
sed -n '300,380p' packages/rangelink-vscode-extension/src/destinations/ComposablePasteDestination.ts
# Find parseLogContext usage in clipboardPreservation.test.ts (if any)
rg -n "parseLogContext" packages/rangelink-vscode-extension/src/__integration-tests__/suite/clipboardPreservation.test.ts -SRepository: couimet/rangeLink
Length of output: 18780
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find how other integration tests use parseLogContext + getLogCapture for structured assertions
rg -n "getLogCapture\\(\\)\\.getLinesSince\\(|parseLogContext\\(" packages/rangelink-vscode-extension/src/__integration-tests__/suite -S
# Find structured assertions on reason / COMMAND_FOCUS_FAILED in integration tests (not unit tests)
rg -n "reason.*COMMAND_FOCUS_FAILED|COMMAND_FOCUS_FAILED" packages/rangelink-vscode-extension/src/__integration-tests__ -SRepository: couimet/rangeLink
Length of output: 383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the LogCapture implementation and how it records logger output
fd -t f "LogCapture" packages/rangelink-vscode-extension/src | head
fd -t f "logCapture" packages/rangelink-vscode-extension/src | head
rg -n "class LogCapture|type LogCapture|export.*logCapture|getLinesSince\\(|getLinesSince\\(" packages/rangelink-vscode-extension/src -S
# Find where barebone-logger is wired up / formatted for captured lines
rg -n "barebone-logger|createLogger\\(|logger\\.warn\\(|logger\\." packages/rangelink-vscode-extension/src -S | head -n 80Repository: couimet/rangeLink
Length of output: 31127
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' packages/rangelink-vscode-extension/src/LogCapture.ts
sed -n '1,220p' packages/rangelink-vscode-extension/src/__tests__/LogCapture.test.tsRepository: couimet/rangeLink
Length of output: 9354
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '240,330p' packages/rangelink-vscode-extension/src/__tests__/destinations/ComposablePasteDestination.integration.test.ts
sed -n '360,430p' packages/rangelink-vscode-extension/src/__tests__/destinations/ComposablePasteDestination.integration.test.tsRepository: couimet/rangeLink
Length of output: 5665
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect structured log assertions for "Focus failed, cannot paste link"
sed -n '240,330p' packages/rangelink-vscode-extension/src/__tests__/destinations/ComposablePasteDestination.integration.test.ts
sed -n '360,430p' packages/rangelink-vscode-extension/src/__tests__/destinations/ComposablePasteDestination.integration.test.tsRepository: couimet/rangeLink
Length of output: 5665
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "COMMAND_FOCUS_FAILED" packages/rangelink-vscode-extension/src/__integration-tests__ -S
rg -n "parseLogContext\\(.*\\)\\s*\\?\\.?\\.reason|parseLogContext\\(.*\\)\\?.reason|\\.reason\\s*===\\s*'COMMAND_FOCUS_FAILED'" packages/rangelink-vscode-extension/src/__integration-tests__ -S
rg -n "assert.*reason|reason:\" packages/rangelink-vscode-extension/src/__integration-tests__ -SRepository: couimet/rangeLink
Length of output: 43
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the focus failure error reason code wiring
rg -n "COMMAND_FOCUS_FAILED" packages/rangelink-vscode-extension/src -S
rg -n "focusResult\\.error\\.reason|error\\.reason" packages/rangelink-vscode-extension/src/destinations packages/rangelink-vscode-extension/src/utils packages/rangelink-vscode-extension/src/navigation packages/rangelink-vscode-extension/src -SRepository: couimet/rangeLink
Length of output: 2537
Use structured log context assertions for focus-failure logs (avoid substring-only checks).
ComposablePasteDestination.performPaste logs the focus-failure warning with structured context ({ ...context, reason: focusResult.error.reason }) plus the human message Focus failed, cannot paste ${contentLabel}. assertLogContains(..., "Focus failed, cannot paste link", ...) only matches the message substring and doesn’t assert the semantic field (reason). Update the assertions to use parseLogContext(...) on captured lines and check ctx.reason === 'COMMAND_FOCUS_FAILED' (and ctx.fn for the paste operation) instead of relying on the substring.
Applies to: 370-374, 409-413
🤖 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
`@packages/rangelink-vscode-extension/src/__integration-tests__/suite/clipboardPreservation.test.ts`
around lines 370 - 374, Replace the substring-only assertions that look for
"Focus failed, cannot paste link" with structured-context checks: capture the
same logged line(s), call parseLogContext(...) on the captured log entry and
assert the resulting ctx.reason === 'COMMAND_FOCUS_FAILED' and ctx.fn (or
equivalent function identifier emitted by
ComposablePasteDestination.performPaste) matches the paste operation; do this
for the assertions currently around the focus-failure check (the block
exercising ComposablePasteDestination.performPaste) and the second occurrence so
the test verifies the structured context instead of only the message substring.
Source: Coding guidelines
Comment on lines
+58
to
83
| it('captures per-terminal name in closure', async () => { | ||
| const mockPasteService = createMockTerminalPasteService(); | ||
| const terminal1 = createMockTerminal({ name: 'Terminal 1' }); | ||
| const terminal2 = createMockTerminal({ name: 'Terminal 2' }); | ||
| const pasteIntoTerminalSpy = jest | ||
| .spyOn(mockAdapter, 'pasteIntoTerminal') | ||
| .mockResolvedValue(undefined); | ||
|
|
||
| const factory = new TerminalInsertFactory(mockAdapter, mockLogger); | ||
| const factory = new TerminalInsertFactory(mockPasteService, mockLogger); | ||
| const insertFn1 = factory.forTarget(terminal1); | ||
| const insertFn2 = factory.forTarget(terminal2); | ||
|
|
||
| await insertFn1('content for terminal 1'); | ||
| await insertFn2('content for terminal 2'); | ||
|
|
||
| expect(pasteIntoTerminalSpy).toHaveBeenNthCalledWith(1, terminal1); | ||
| expect(pasteIntoTerminalSpy).toHaveBeenNthCalledWith(2, terminal2); | ||
| expect(mockPasteService.pasteIntoTerminal).toHaveBeenCalledTimes(2); | ||
| expect(mockPasteService.pasteIntoTerminal).toHaveBeenNthCalledWith( | ||
| 1, | ||
| 'content for terminal 1', | ||
| terminal1, | ||
| ); | ||
| expect(mockPasteService.pasteIntoTerminal).toHaveBeenNthCalledWith( | ||
| 2, | ||
| 'content for terminal 2', | ||
| terminal2, | ||
| ); | ||
|
|
||
| expect(mockLogger.error).not.toHaveBeenCalled(); | ||
| expect(mockAdapter.writeTextToClipboard).not.toHaveBeenCalled(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Assert success logs in the per-terminal closure test.
This test verifies insert behavior but does not assert the corresponding info logs for each terminal. Add explicit logger assertions so logging remains part of the behavior contract.
As per coding guidelines, “Always include logger assertions in tests that verify method behavior - consolidate logging assertions with behavior tests, not separate tests.”
Suggested patch
it('captures per-terminal name in closure', async () => {
@@
expect(mockPasteService.pasteIntoTerminal).toHaveBeenNthCalledWith(
2,
'content for terminal 2',
terminal2,
);
+ expect(mockLogger.info).toHaveBeenNthCalledWith(
+ 1,
+ { fn: 'TerminalInsertFactory.insert', terminalName: 'Terminal 1' },
+ 'Terminal paste succeeded',
+ );
+ expect(mockLogger.info).toHaveBeenNthCalledWith(
+ 2,
+ { fn: 'TerminalInsertFactory.insert', terminalName: 'Terminal 2' },
+ 'Terminal paste succeeded',
+ );
expect(mockLogger.error).not.toHaveBeenCalled();
});🤖 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
`@packages/rangelink-vscode-extension/src/__tests__/destinations/capabilities/insertFactories/terminalInsertFactory.test.ts`
around lines 58 - 83, The test for TerminalInsertFactory.forTarget currently
verifies pasteIntoTerminal calls but omits logger assertions; update the test to
assert that mockLogger.info was called for each insert with the appropriate
messages/arguments (one for 'content for terminal 1' targeting terminal1 and one
for 'content for terminal 2' targeting terminal2), and keep the existing
mockLogger.error assertion; locate the test using TerminalInsertFactory,
forTarget, insertFn1/insertFn2 and add expectations on mockLogger.info to match
the per-terminal success log contract.
Source: Coding guidelines
Comment on lines
+7
to
+16
| stage: jest.fn((_text: string, fn: () => Promise<unknown>) => fn().then((v) => Result.ok(v))), | ||
| route: jest.fn((fn: () => Promise<unknown>, _shouldRestore?: () => boolean) => | ||
| fn().then((v) => Result.ok(v)), | ||
| ), | ||
| read: jest.fn().mockResolvedValue(Result.ok('')), | ||
| write: jest.fn().mockResolvedValue(Result.ok(undefined)), | ||
| restoreClipboard: jest.fn().mockResolvedValue(undefined), | ||
| capture: jest.fn((producer: () => Promise<unknown>) => | ||
| producer().then((produced) => Result.ok({ clipboard: '', produced })), | ||
| ), |
Contributor
There was a problem hiding this comment.
Mock ClipboardService should preserve Result contract on callback failures.
Line 7, Line 9, and Line 15 currently let rejected callbacks bubble as thrown rejections instead of returning Result.err(...). This diverges from the real service contract and can make tests validate the wrong behavior.
Proposed fix
export const createMockClipboardService = (): jest.Mocked<ClipboardService> =>
({
- stage: jest.fn((_text: string, fn: () => Promise<unknown>) => fn().then((v) => Result.ok(v))),
- route: jest.fn((fn: () => Promise<unknown>, _shouldRestore?: () => boolean) =>
- fn().then((v) => Result.ok(v)),
- ),
+ stage: jest.fn(async (_text: string, fn: () => Promise<unknown>) => {
+ try {
+ return Result.ok(await fn());
+ } catch (error) {
+ return Result.err(error as Error);
+ }
+ }),
+ route: jest.fn(async (fn: () => Promise<unknown>, _shouldRestore?: () => boolean) => {
+ try {
+ return Result.ok(await fn());
+ } catch (error) {
+ return Result.err(error as Error);
+ }
+ }),
read: jest.fn().mockResolvedValue(Result.ok('')),
write: jest.fn().mockResolvedValue(Result.ok(undefined)),
restoreClipboard: jest.fn().mockResolvedValue(undefined),
- capture: jest.fn((producer: () => Promise<unknown>) =>
- producer().then((produced) => Result.ok({ clipboard: '', produced })),
- ),
+ capture: jest.fn(async (producer: () => Promise<unknown>) => {
+ try {
+ const produced = await producer();
+ return Result.ok({ clipboard: '', produced });
+ } catch (error) {
+ return Result.err(error as Error);
+ }
+ }),
}) as unknown as jest.Mocked<ClipboardService>;🤖 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
`@packages/rangelink-vscode-extension/src/__tests__/helpers/createMockClipboardService.ts`
around lines 7 - 16, The mock ClipboardService functions stage, route, and
capture currently allow rejected callback promises to bubble as thrown
rejections; update each to catch errors and return a Result.err(...) instead of
throwing so the mock matches the real service contract — specifically modify the
stage mock (jest.fn((_text, fn) => ...)), the route mock (jest.fn((fn,
_shouldRestore) => ...)) and the capture mock (jest.fn((producer) => ...)) to
wrap their fn()/producer() calls with .then(...).catch(err => Result.err(err))
and ensure the resolved success path still returns Result.ok(...).
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
Adds reusable assertion helpers (
withClipboardSentinel,withClipboardChanged,withClipboardRestored,assertClipboardEqualsGeneratedLink,assertTerminalBufferEqualsGeneratedLink,assertTerminalBufferContainsGeneratedLink,assertClipboardPreservedAndTerminalLink,assertLogContains) and converts all call sites across test suite files to use them.Changes
clipboardHelpers.ts,capturingPtyHelpers.ts,logBasedUiAssertions.ts, andlogHelpers.tseliminate repeated clipboard/terminal/log assertion boilerplate:with-extensions --grepdocumentationexpectContextKeyscalls from test suites — those belong to the ContextKeyService extraction (PR Unifyjest.config.jsfiles to explicitly call-out some configs #3) and would fail without that infrastructureTest Plan
pnpm test:release:automatedpnpm test:release:with-extensionsSummary by CodeRabbit
Release Notes
Bug Fixes
Tests
Chores