Skip to content

Commit 19da6f2

Browse files
committed
match tool outputs whole instead of probing fragments
Substring assertions on llmxml renders let regressions in any unprobed attribute or line pass unnoticed; the rendered text is the product contract a model reads, so it is now pinned byte for byte. Multi-line renders compare against golden files (regenerate with go test ./internal/tools/ -update and review the diff); one-line acks compare inline. Error-path assertions stay substring-based -- error wording is not a contract. Wire-path assertions tightened to exact equality alongside, which immediately surfaced a detail Contains had hidden: the files listing request ends with a trailing slash.
1 parent b15a4ba commit 19da6f2

15 files changed

Lines changed: 114 additions & 80 deletions

internal/gerritclient/review_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ func Test_SetReview_Gating(t *testing.T) {
7272
require.NoError(t, err)
7373

7474
require.Len(t, *posts, 1)
75-
assert.Contains(t, (*posts)[0], "/revisions/current/review")
75+
assert.Equal(t, "/a/changes/123/revisions/current/review", (*posts)[0])
7676
})
7777

7878
t.Run("foreign change refused by default", func(t *testing.T) {

internal/gerritclient/scoping_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,7 @@ func Test_ProjectScoping(t *testing.T) {
146146

147147
assert.NotNil(t, files)
148148
require.Len(t, *requests, 1)
149-
assert.Contains(t, (*requests)[0], "/files")
149+
assert.Equal(t, "/a/changes/core~123/revisions/current/files/", (*requests)[0])
150150
})
151151

152152
t.Run("unscoped client passes everything through", func(t *testing.T) {

internal/tools/get-change_test.go

Lines changed: 1 addition & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -137,24 +137,7 @@ func Test_GetChange(t *testing.T) {
137137
text, ok := res.Content[0].(*mcp.TextContent)
138138
require.True(t, ok)
139139

140-
for _, part := range []string{
141-
`<change number="123"`,
142-
`project="core"`,
143-
`status="NEW"`,
144-
`owner="Alice (alice)"`,
145-
`submittable="true"`,
146-
`current_revision="abc123def"`,
147-
"<subject>Fix nil deref in scanner</subject>",
148-
`<label name="Code-Review">`,
149-
`<vote value="2" by="Bob (bob)"/>`,
150-
`<reviewer state="REVIEWER">Bob (bob)</reviewer>`,
151-
`<message author="Bob (bob)"`,
152-
"Looks good to me",
153-
} {
154-
assert.Contains(t, text.Text, part)
155-
}
156-
157-
assert.NotContains(t, text.Text, "Carol", "zero votes must not render")
140+
golden(t, "get-change", text.Text)
158141

159142
wantOpts := []string{"DETAILED_LABELS", "DETAILED_ACCOUNTS", "CURRENT_REVISION", "MESSAGES", "SUBMITTABLE"}
160143
for _, opt := range wantOpts {

internal/tools/golden_test.go

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
package tools_test
2+
3+
import (
4+
"flag"
5+
"os"
6+
"path/filepath"
7+
"testing"
8+
9+
"github.com/stretchr/testify/assert"
10+
"github.com/stretchr/testify/require"
11+
)
12+
13+
//nolint:gochecknoglobals // test binary flag, must register at package init
14+
var update = flag.Bool("update", false, "rewrite golden files with actual tool output")
15+
16+
// golden asserts that got matches testdata/<name>.golden byte for byte. Tool
17+
// output is the product contract — the rendered llmxml is exactly what a
18+
// model reads — so renders are pinned whole, not probed for fragments.
19+
// Running the package tests with -update rewrites the files from actual
20+
// output; review the diff before committing.
21+
func golden(t *testing.T, name, got string) {
22+
t.Helper()
23+
24+
path := filepath.Join("testdata", name+".golden")
25+
26+
if *update {
27+
require.NoError(t, os.MkdirAll(filepath.Dir(path), 0o750))
28+
require.NoError(t, os.WriteFile(path, []byte(got), 0o600))
29+
}
30+
31+
want, err := os.ReadFile(path) //nolint:gosec // path is test-owned: testdata/<name>.golden
32+
require.NoError(t, err, "missing golden file; regenerate with: go test ./internal/tools/ -update")
33+
assert.Equal(t, string(want), got)
34+
}

internal/tools/post-comments_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ func Test_PostComments(t *testing.T) {
7474
},
7575
})
7676

77-
assert.Contains(t, out, `<review_posted change="123" message="true" comments="3" notify="OWNER"/>`)
77+
assert.Equal(t, `<review_posted change="123" message="true" comments="3" notify="OWNER"/>`, out)
7878

7979
assert.Equal(t, "Overall: looks solid", (*body)["message"])
8080
assert.Equal(t, "OWNER", (*body)["notify"])

internal/tools/read-tools_test.go

Lines changed: 6 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ package tools_test
22

33
import (
44
"net/http"
5-
"strings"
65
"testing"
76

87
"github.com/modelcontextprotocol/go-sdk/mcp"
@@ -64,17 +63,7 @@ func Test_SearchChanges(t *testing.T) {
6463

6564
out := callTool(t, cs, "search_changes", map[string]any{"query": "status:open owner:self"})
6665

67-
for _, part := range []string{
68-
`<changes query="status:open owner:self"`,
69-
`count="2"`,
70-
`more="true"`,
71-
`<change number="1"`,
72-
`owner="Alice (alice)"`,
73-
`>First</change>`,
74-
`status="MERGED"`,
75-
} {
76-
assert.Contains(t, out, part)
77-
}
66+
golden(t, "search-changes", out)
7867

7968
assert.Contains(t, *lastURL, "q=status")
8069
assert.Contains(t, *lastURL, "n=25", "default limit applies")
@@ -95,21 +84,9 @@ func Test_ListChangeFiles(t *testing.T) {
9584

9685
out := callTool(t, cs, "list_change_files", map[string]any{"change": "123"})
9786

98-
for _, part := range []string{
99-
`<files change="123" revision="current" count="4">`,
100-
`<file path="/COMMIT_MSG" status="A"`,
101-
`<file path="core/scanner.go" status="M" insertions="5" deletions="2"/>`,
102-
`binary="true"`,
103-
`old_path="core/old.go"`,
104-
} {
105-
assert.Contains(t, out, part)
106-
}
87+
golden(t, "list-change-files", out)
10788

10889
assert.Contains(t, *lastURL, "/a/changes/123/revisions/current/files")
109-
110-
logoIdx := strings.Index(out, "assets/logo.png")
111-
scannerIdx := strings.Index(out, "core/scanner.go")
112-
assert.Less(t, logoIdx, scannerIdx, "files sorted by path")
11390
}
11491

11592
func Test_GetFileDiff(t *testing.T) {
@@ -135,16 +112,7 @@ func Test_GetFileDiff(t *testing.T) {
135112
"change": "123", "file": "core/scanner.go", "revision": "2",
136113
})
137114

138-
for _, part := range []string{
139-
`<diff change="123" revision="2" file="core/scanner.go" change_type="MODIFIED">`,
140-
"... 40 common lines skipped ...",
141-
" func scan() {",
142-
"-\told := 1",
143-
"+\tnew := 2",
144-
"+\textra := 3",
145-
} {
146-
assert.Contains(t, out, part)
147-
}
115+
golden(t, "file-diff-text", out)
148116

149117
assert.Contains(t, *lastURL, "/a/changes/123/revisions/2/files/core%2Fscanner.go/diff")
150118
})
@@ -158,8 +126,7 @@ func Test_GetFileDiff(t *testing.T) {
158126

159127
out := callTool(t, cs, "get_file_diff", map[string]any{"change": "123", "file": "logo.png"})
160128

161-
assert.Contains(t, out, `binary="true"`)
162-
assert.Contains(t, out, "/>")
129+
golden(t, "file-diff-binary", out)
163130
})
164131
}
165132

@@ -194,24 +161,9 @@ func Test_GetChangeComments(t *testing.T) {
194161

195162
out := callTool(t, cs, "get_change_comments", map[string]any{"change": "123"})
196163

197-
for _, part := range []string{
198-
`<comments change="123" filter="all" threads="3">`,
199-
`<thread resolved="true">`,
200-
`<thread resolved="false">`,
201-
`<comment id="c1"`,
202-
`in_reply_to="c1"`,
203-
`lines="20-25"`,
204-
"Is this nil-safe?",
205-
"Orphan reply",
206-
} {
207-
assert.Contains(t, out, part)
208-
}
164+
golden(t, "change-comments-all", out)
209165

210166
assert.Contains(t, *lastURL, "/a/changes/123/comments")
211-
212-
c1 := strings.Index(out, `<comment id="c1"`)
213-
c2 := strings.Index(out, `<comment id="c2"`)
214-
assert.Less(t, c1, c2, "replies follow their root chronologically")
215167
})
216168

217169
t.Run("unresolved filter drops resolved threads", func(t *testing.T) {
@@ -221,10 +173,7 @@ func Test_GetChangeComments(t *testing.T) {
221173

222174
out := callTool(t, cs, "get_change_comments", map[string]any{"change": "123", "status": "unresolved"})
223175

224-
assert.Contains(t, out, `threads="1"`)
225-
assert.Contains(t, out, "This block races")
226-
assert.NotContains(t, out, "Is this nil-safe?")
227-
assert.NotContains(t, out, "Orphan reply")
176+
golden(t, "change-comments-unresolved", out)
228177
})
229178

230179
t.Run("invalid filter is an error", func(t *testing.T) {

internal/tools/set-vote_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ func Test_SetVote(t *testing.T) {
6666
"message": "lgtm",
6767
})
6868

69-
assert.Contains(t, out, `<vote_set change="123" label="Code-Review" value="2"/>`)
69+
assert.Equal(t, `<vote_set change="123" label="Code-Review" value="2"/>`, out)
7070

7171
assert.Equal(t, "lgtm", (*body)["message"])
7272
assert.Equal(t, map[string]any{"Code-Review": float64(2)}, (*body)["labels"])
Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
<comments change="123" filter="all" threads="3">
2+
<file path="core/scanner.go">
3+
<thread resolved="true">
4+
<comment id="c1" author="Bob (bob)" date="2026-07-01T10:00:00Z" patch_set="1" line="10">
5+
Is this nil-safe?
6+
</comment>
7+
<comment id="c2" author="Alice (alice)" date="2026-07-01T11:00:00Z" patch_set="1" line="10" in_reply_to="c1">
8+
Fixed in ps2
9+
</comment>
10+
</thread>
11+
<thread resolved="false">
12+
<comment id="c3" author="Bob (bob)" date="2026-07-02T09:00:00Z" patch_set="2" lines="20-25">
13+
This block races
14+
</comment>
15+
</thread>
16+
</file>
17+
<file path="docs/readme.md">
18+
<thread resolved="true">
19+
<comment id="c4" author="Alice (alice)" date="2026-07-01T12:00:00Z" line="1" in_reply_to="gone">
20+
Orphan reply
21+
</comment>
22+
</thread>
23+
</file>
24+
</comments>
Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
<comments change="123" filter="unresolved" threads="1">
2+
<file path="core/scanner.go">
3+
<thread resolved="false">
4+
<comment id="c3" author="Bob (bob)" date="2026-07-02T09:00:00Z" patch_set="2" lines="20-25">
5+
This block races
6+
</comment>
7+
</thread>
8+
</file>
9+
</comments>
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
<diff change="123" revision="current" file="logo.png" change_type="ADDED" binary="true"/>

0 commit comments

Comments
 (0)