ci: make language E2E snapshots resilient to line drift - #185
Conversation
📝 WalkthroughWalkthroughLanguage E2E tests now return structured search results, validate snapshot files, compare normalized identities, and report missing or unexpected results. Snapshot handling supports explicit files, updates, missing-file creation, read errors, and serialized Ollama execution. ChangesLanguage snapshot validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant langSearch
participant formatLangSearchResults
participant compareLangSnapshotT
participant SnapshotFile
langSearch->>formatLangSearchResults: structured search results
formatLangSearchResults->>compareLangSnapshotT: formatted results
compareLangSnapshotT->>SnapshotFile: read explicit snapshot
SnapshotFile-->>compareLangSnapshotT: snapshot content
compareLangSnapshotT-->>langSearch: comparison result or test error
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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
🧹 Nitpick comments (3)
e2e_lang_test.go (2)
33-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse one constant for the snapshot directory.
Line 35 and line 38 declare the same path independently. If one changes,
compareLangSnapshotTreads a path that no longer exists. The test then silently falls into the creation branch instead of comparing. Declare the constant first and pass it tocupaloy.SnapshotSubdirectory.♻️ Proposed refactor
+const langSnapshotDirectory = "testdata/snapshots" + var snapshotter = cupaloy.New( cupaloy.EnvVariableName("UPDATE_SNAPSHOTS"), - cupaloy.SnapshotSubdirectory("testdata/snapshots"), + cupaloy.SnapshotSubdirectory(langSnapshotDirectory), ) - -const langSnapshotDirectory = "testdata/snapshots"🤖 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 `@e2e_lang_test.go` around lines 33 - 38, Update the snapshot directory declarations so langSnapshotDirectory is declared first and reused as the argument to cupaloy.SnapshotSubdirectory in the snapshotter initialization. Remove the duplicated string literal while preserving compareLangSnapshotT’s existing path behavior.
149-151: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant loop variable copy.
Go 1.22 gives each iteration its own
q. The project targets Go 1.25.copyloopvarin golangci-lint reports line 150.♻️ Proposed refactor
for _, q := range queries { - q := q t.Run(strings.ReplaceAll(q, " ", "_"), func(t *testing.T) {As per coding guidelines: "Use Go 1.25 or newer." and "Run golangci-lint with zero issues."
🤖 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 `@e2e_lang_test.go` around lines 149 - 151, Remove the redundant q := q assignment inside the queries loop before t.Run; Go 1.25 provides per-iteration loop variable scope, so keep the existing subtest naming and callback behavior unchanged while satisfying copyloopvar.Source: Coding guidelines
e2e_lang_snapshot_test.go (1)
112-154: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConfirm the mixed use of
declaredCountand the deduplicated identity set.
countMatchescompareslen(actual)againstdeclaredCount, which counts snapshot lines.requiredOverlapuseslen(expected.identities), which is the deduplicated set. If a snapshot contains duplicate identities, the overlap threshold drops below 50% of the declared results. The test at line 266 shows this is intended. Add a short comment that records the intent, so a later reader does not treat it as a bug.🤖 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 `@e2e_lang_snapshot_test.go` around lines 112 - 154, Add a concise comment in compareLangSnapshot near countMatches and requiredOverlap documenting that declaredCount intentionally counts snapshot lines while expected.identities is deduplicated, and that this mixed comparison is intentional for duplicate identities. Do not change the existing matching logic.
🤖 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 `@e2e_lang_snapshot_test.go`:
- Around line 287-310: Update TestParseCommittedLangSnapshots to count entries
matching the TestLang_ prefix and successfully parse them, then fail the test
when the count is zero. Preserve the existing directory filtering, file-reading,
and parse error handling.
---
Nitpick comments:
In `@e2e_lang_snapshot_test.go`:
- Around line 112-154: Add a concise comment in compareLangSnapshot near
countMatches and requiredOverlap documenting that declaredCount intentionally
counts snapshot lines while expected.identities is deduplicated, and that this
mixed comparison is intentional for duplicate identities. Do not change the
existing matching logic.
In `@e2e_lang_test.go`:
- Around line 33-38: Update the snapshot directory declarations so
langSnapshotDirectory is declared first and reused as the argument to
cupaloy.SnapshotSubdirectory in the snapshotter initialization. Remove the
duplicated string literal while preserving compareLangSnapshotT’s existing path
behavior.
- Around line 149-151: Remove the redundant q := q assignment inside the queries
loop before t.Run; Go 1.25 provides per-iteration loop variable scope, so keep
the existing subtest naming and callback behavior unchanged while satisfying
copyloopvar.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: beb4448a-6f77-41fa-a4f1-5bdec15b330c
📒 Files selected for processing (4)
e2e_lang_snapshot_test.goe2e_lang_test.goe2e_test.goe2e_types_test.go
💤 Files with no reviewable changes (1)
- e2e_test.go
| func TestParseCommittedLangSnapshots(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| snapshotDirectory := filepath.Join("testdata", "snapshots") | ||
| entries, err := os.ReadDir(snapshotDirectory) | ||
| if err != nil { | ||
| t.Fatalf("failed to read snapshot directory: %v", err) | ||
| } | ||
|
|
||
| for _, entry := range entries { | ||
| if entry.IsDir() || !strings.HasPrefix(entry.Name(), "TestLang_") { | ||
| continue | ||
| } | ||
| t.Run(entry.Name(), func(t *testing.T) { | ||
| snapshot, err := os.ReadFile(filepath.Join(snapshotDirectory, entry.Name())) | ||
| if err != nil { | ||
| t.Fatalf("failed to read snapshot: %v", err) | ||
| } | ||
| if _, err := parseLangSnapshot(string(snapshot)); err != nil { | ||
| t.Fatalf("parseLangSnapshot() error = %v", err) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fail when no committed snapshot matches the prefix.
TestParseCommittedLangSnapshots passes when the directory contains no TestLang_ entry. A rename of the language tests, or a deleted snapshot directory content, then removes this coverage without a signal. Count the parsed snapshots and fail on zero.
💚 Proposed fix
+ parsed := 0
for _, entry := range entries {
if entry.IsDir() || !strings.HasPrefix(entry.Name(), "TestLang_") {
continue
}
+ parsed++
t.Run(entry.Name(), func(t *testing.T) {
snapshot, err := os.ReadFile(filepath.Join(snapshotDirectory, entry.Name()))
if err != nil {
t.Fatalf("failed to read snapshot: %v", err)
}
if _, err := parseLangSnapshot(string(snapshot)); err != nil {
t.Fatalf("parseLangSnapshot() error = %v", err)
}
})
}
+ if parsed == 0 {
+ t.Fatalf("no committed language snapshots found in %s", snapshotDirectory)
+ }
}📝 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.
| func TestParseCommittedLangSnapshots(t *testing.T) { | |
| t.Parallel() | |
| snapshotDirectory := filepath.Join("testdata", "snapshots") | |
| entries, err := os.ReadDir(snapshotDirectory) | |
| if err != nil { | |
| t.Fatalf("failed to read snapshot directory: %v", err) | |
| } | |
| for _, entry := range entries { | |
| if entry.IsDir() || !strings.HasPrefix(entry.Name(), "TestLang_") { | |
| continue | |
| } | |
| t.Run(entry.Name(), func(t *testing.T) { | |
| snapshot, err := os.ReadFile(filepath.Join(snapshotDirectory, entry.Name())) | |
| if err != nil { | |
| t.Fatalf("failed to read snapshot: %v", err) | |
| } | |
| if _, err := parseLangSnapshot(string(snapshot)); err != nil { | |
| t.Fatalf("parseLangSnapshot() error = %v", err) | |
| } | |
| }) | |
| } | |
| } | |
| func TestParseCommittedLangSnapshots(t *testing.T) { | |
| t.Parallel() | |
| snapshotDirectory := filepath.Join("testdata", "snapshots") | |
| entries, err := os.ReadDir(snapshotDirectory) | |
| if err != nil { | |
| t.Fatalf("failed to read snapshot directory: %v", err) | |
| } | |
| parsed := 0 | |
| for _, entry := range entries { | |
| if entry.IsDir() || !strings.HasPrefix(entry.Name(), "TestLang_") { | |
| continue | |
| } | |
| parsed++ | |
| t.Run(entry.Name(), func(t *testing.T) { | |
| snapshot, err := os.ReadFile(filepath.Join(snapshotDirectory, entry.Name())) | |
| if err != nil { | |
| t.Fatalf("failed to read snapshot: %v", err) | |
| } | |
| if _, err := parseLangSnapshot(string(snapshot)); err != nil { | |
| t.Fatalf("parseLangSnapshot() error = %v", err) | |
| } | |
| }) | |
| } | |
| if parsed == 0 { | |
| t.Fatalf("no committed language snapshots found in %s", snapshotDirectory) | |
| } | |
| } |
🤖 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 `@e2e_lang_snapshot_test.go` around lines 287 - 310, Update
TestParseCommittedLangSnapshots to count entries matching the TestLang_ prefix
and successfully parse them, then fail the test when the count is zero. Preserve
the existing directory filtering, file-reading, and parse error handling.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e_lang_snapshot_test.go (1)
116-161: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe declared result count is reported but never enforced.
The PR summary states that tests require the expected result count and at least 50% identity overlap.
compareLangSnapshotonly checks the overlap threshold. It printsexpected result countandactual result countin the failure message, but it never compareslen(actual)againstexpected.declaredCount. The test case at Lines 290-292 confirms this:baseline[:2]passes against a 4-row snapshot.A snapshot with
results: 0also passes for any actual slice, becauserequiredMatchesbecomes 0.Confirm which behavior you want. If the count must match, add an explicit check. If the relaxed behavior is intended, update the PR description so the contract is unambiguous.
🧩 Optional count check
expectedMatches, actualMatches := matchLangSnapshotResults(expected.results, actualResults) matchedCount := 0 for _, matched := range expectedMatches { if matched { matchedCount++ } } requiredMatches := (len(expected.results) + 1) / 2 - if matchedCount >= requiredMatches { + if len(actual) == expected.declaredCount && matchedCount >= requiredMatches { return nil }🤖 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 `@e2e_lang_snapshot_test.go` around lines 116 - 161, Enforce the declared result count in compareLangSnapshot by requiring len(actual) to equal expected.declaredCount before accepting the snapshot comparison. Preserve the existing 50% identity-overlap check for matching counts, and ensure zero-result snapshots do not automatically pass when actual results are present.
🤖 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.
Outside diff comments:
In `@e2e_lang_snapshot_test.go`:
- Around line 116-161: Enforce the declared result count in compareLangSnapshot
by requiring len(actual) to equal expected.declaredCount before accepting the
snapshot comparison. Preserve the existing 50% identity-overlap check for
matching counts, and ensure zero-result snapshots do not automatically pass when
actual results are present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a24ea35a-c147-43d8-974f-f79b33a12b07
📒 Files selected for processing (4)
.github/workflows/ci.ymle2e_lang_snapshot_test.goe2e_lang_test.goe2e_types_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e_types_test.go
1 participant
Summary
Makes language E2E snapshots compare stable file, symbol, and kind identities while tolerating line-number drift, requiring the expected result count and at least 50% identity overlap. Adds validation and unit coverage for exact, drifted, partial, duplicate, malformed, and invalid-result cases while preserving Cupaloy for snapshot updates and creation. Moves the shared search result test type into a common test file.
Testing
make testSummary by CodeRabbit