test: patch seekSequence matching and Command.hints#435
test: patch seekSequence matching and Command.hints#435anandgupta42 wants to merge 1 commit intomainfrom
Conversation
…ction Cover two previously untested code paths: (1) Patch.deriveNewContentsFromChunks with seekSequence's 4-pass matching (exact, rstrip, trim, Unicode-normalized) and EOF anchoring/change_context seeking; (2) Command.hints() argument placeholder extraction that drives TUI argument prompts for slash commands. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> https://claude.ai/code/session_01Nvdry9ChLovXGjHNS8kR7R
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review.
Tip: disable this comment in your organization's Code Review settings.
📝 WalkthroughWalkthroughTwo new test files were added to the opencode package. The first tests the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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.
Actionable comments posted: 1
🤖 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/opencode/test/patch/seek-sequence.test.ts`:
- Around line 1-32: The test currently manually creates and removes a temp
directory using beforeEach/afterEach and a tempDir variable; replace that
pattern by using the tmpdir fixture with await using for automatic cleanup:
remove the beforeEach/afterEach blocks and tempDir variable, and instead declare
a scoped resource via "await using tmp = await tmpdir()" inside the
describe/test scope (or at the top of each test) so tests use tmp.path (or
similar) for file paths; update any references to tempDir to use the tmp fixture
and ensure the tests in this file follow the same await-using tmpdir() pattern
as required by the repo conventions.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9f93c373-b170-4dce-9e8d-1f951a60bdba
📒 Files selected for processing (2)
packages/opencode/test/command/hints.test.tspackages/opencode/test/patch/seek-sequence.test.ts
| import { describe, test, expect, beforeEach, afterEach } from "bun:test" | ||
| import { Patch } from "../../src/patch" | ||
| import * as fs from "fs/promises" | ||
| import * as path from "path" | ||
| import { tmpdir } from "os" | ||
|
|
||
| /** | ||
| * Tests for Patch.deriveNewContentsFromChunks — the core function that applies | ||
| * update chunks to file content using seekSequence's multi-pass matching. | ||
| * | ||
| * seekSequence tries 4 comparison strategies in order: | ||
| * 1. Exact match | ||
| * 2. Trailing whitespace trimmed (rstrip) | ||
| * 3. Both-end whitespace trimmed (trim) | ||
| * 4. Unicode-normalized + trimmed | ||
| * | ||
| * These tests verify that real-world patch application succeeds even when the | ||
| * LLM-generated patch text has minor whitespace or Unicode differences from | ||
| * the actual file content — a common source of "Failed to find expected lines" | ||
| * errors for users. | ||
| */ | ||
|
|
||
| describe("Patch.deriveNewContentsFromChunks — seekSequence matching", () => { | ||
| let tempDir: string | ||
|
|
||
| beforeEach(async () => { | ||
| tempDir = await fs.mkdtemp(path.join(tmpdir(), "seek-test-")) | ||
| }) | ||
|
|
||
| afterEach(async () => { | ||
| await fs.rm(tempDir, { recursive: true, force: true }) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use fixture tmpdir() with await using instead of manual mkdtemp/rm.
The current before/after lifecycle bypasses the required test fixture pattern and manual cleanup can drift from repo conventions.
🔧 Suggested refactor pattern
import { describe, test, expect, beforeEach, afterEach } from "bun:test"
import { Patch } from "../../src/patch"
import * as fs from "fs/promises"
import * as path from "path"
-import { tmpdir } from "os"
+import { tmpdir } from "../fixture/fixture"
describe("Patch.deriveNewContentsFromChunks — seekSequence matching", () => {
- let tempDir: string
-
- beforeEach(async () => {
- tempDir = await fs.mkdtemp(path.join(tmpdir(), "seek-test-"))
- })
-
- afterEach(async () => {
- await fs.rm(tempDir, { recursive: true, force: true })
- })
-
- test("exact match: replaces old_lines with new_lines", () => {
- const filePath = path.join(tempDir, "exact.txt")
+ test("exact match: replaces old_lines with new_lines", async () => {
+ await using tmp = await tmpdir()
+ const filePath = path.join(tmp.path, "exact.txt")
const content = "line1\nline2\nline3\n"
- require("fs").writeFileSync(filePath, content)
+ await fs.writeFile(filePath, content)Apply the same await using tmp = await tmpdir() pattern to the other tests in this file.
As per coding guidelines, "Use the tmpdir function from fixture/fixture.ts to create temporary directories for tests with automatic cleanup in test files" and "Always use await using syntax with tmpdir() for automatic cleanup when the variable goes out of scope".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/test/patch/seek-sequence.test.ts` around lines 1 - 32, The
test currently manually creates and removes a temp directory using
beforeEach/afterEach and a tempDir variable; replace that pattern by using the
tmpdir fixture with await using for automatic cleanup: remove the
beforeEach/afterEach blocks and tempDir variable, and instead declare a scoped
resource via "await using tmp = await tmpdir()" inside the describe/test scope
(or at the top of each test) so tests use tmp.path (or similar) for file paths;
update any references to tempDir to use the tmp fixture and ensure the tests in
this file follow the same await-using tmpdir() pattern as required by the repo
conventions.
|
Superseded by #439 which consolidates all 12 test PRs into one, deduplicates overlapping tests, and fixes bugs found during review. |
2 participants
What does this PR do?
Adds 26 new tests across 2 previously untested code paths, discovered via automated test-discovery.
1.
Patch.deriveNewContentsFromChunks+seekSequence—src/patch/index.ts(14 new tests)This is the core function that applies LLM-generated patch chunks to files.
seekSequenceuses a 4-pass matching strategy (exact → trailing-whitespace-trimmed → both-end-trimmed → Unicode-normalized) to find old lines in the file before replacing them. Zero direct tests existed forderiveNewContentsFromChunks,seekSequence,normalizeUnicode, orstripHeredoc. New coverage includes:\u201C/\u201Dcurly quotes, patch uses ASCII"— still matches\u2014, patch uses-is_end_of_fileEOF anchoring: verifies the last occurrence is targeted, not the firstchange_contextseeking: disambiguates duplicate lines by seeking to a context line firstold_linesappends contentcat <<'EOF'and<<HEREDOCwrappers are stripped before parsingWhy it matters: When an LLM generates a patch with minor whitespace or Unicode differences from the actual file, users see "Failed to find expected lines" errors. These tests encode the exact matching behavior that prevents those failures.
2.
Command.hints()—src/command/index.ts(12 new tests)This pure function extracts argument placeholders (
$1,$2,$ARGUMENTS) from command templates. These hints drive the TUI's argument prompt display when users invoke slash commands like/review,/init, or custom commands. Zero tests existed. New coverage includes:$ARGUMENTSextraction (always appended after numbered)$10) with lexicographic sort behavior documented$0, non-matching$FOO/$BAR, case sensitivityWhy it matters: If
hints()breaks, the TUI shows wrong or missing argument suggestions for slash commands, confusing users about what arguments a command expects.Type of change
Issue for this PR
N/A — proactive test coverage via automated test-discovery
How did you verify your code works?
Checklist
https://claude.ai/code/session_01Nvdry9ChLovXGjHNS8kR7R
Summary by CodeRabbit