Skip to content

Commit 0bfe709

Browse files
authored
[test] Add tests for proxy.MatchRoute and proxy.MatchGraphQL (#2229)
## Test Coverage Improvement: `proxy.MatchRoute` and `proxy.MatchGraphQL` ### Functions Analyzed - **Package**: `internal/proxy` - **Functions**: `MatchRoute`, `MatchGraphQL`, `extractOwnerRepo` - **Previous Coverage**: Multiple untested route/pattern branches (0%) - **Complexity**: High — combined 34 branches across the two functions ### Why These Functions? `MatchRoute` and `MatchGraphQL` are the security-critical routing entry points for the proxy layer. Several concrete route patterns in `MatchRoute` and two GraphQL tool-name patterns in `MatchGraphQL` had **zero test coverage**, along with multiple fallback branches in the private `extractOwnerRepo` helper (reachable only through `MatchGraphQL`). ### Tests Added (`internal/proxy/proxy_coverage_test.go`) #### `TestMatchRoute_AdditionalRoutes` Covers every route pattern absent from the existing `TestMatchRoute`: | Route | Tool | |-------|------| | `/repos/{o}/{r}/git/ref/tags/{tag}` | `get_tag` | | `/repos/{o}/{r}/git/trees/{path}` | `get_file_contents` | | `/repos/{o}/{r}/labels` | `list_labels` | | `/repos/{o}/{r}/actions/workflows` | `actions_list` (list_workflows) | | `/repos/{o}/{r}/actions/runs` | `actions_list` (list_workflow_runs) | | `/repos/{o}/{r}/pulls/{n}/comments` | `pull_request_read` (get_review_comments) | | `/search/repositories` | `search_repositories` | | `/repos/{o}/{r}` (bare root) | fallback `get_file_contents` | | `/repos/{o}/{r}/unknown-resource` | fallback `get_file_contents` | #### `TestMatchGraphQL_AdditionalPatterns` Covers the two GraphQL patterns missing from `TestMatchGraphQL`: - ✅ `projectV2` / `ProjectV2` queries → `list_projects` - ✅ Generic `repository(...)` queries (no issue/PR/search/project sub-fields) → `get_file_contents` #### `TestMatchGraphQL_ExtractOwnerRepo_EdgeCases` Exhaustively covers the fallback branches in `extractOwnerRepo`: - ✅ No owner or repo anywhere → both empty strings - ✅ Owner from variables only (repo stays empty) - ✅ Owner from query pattern, repo from `name` variable key - ✅ Owner from `owner` variable key, repo from query pattern - ✅ `repo` variable key used when `name` key is absent - ✅ `name` key takes priority over `repo` key - ✅ `varOwnerPattern` fallback (inline `"owner": "…"` in query string) - ✅ `varRepoPattern` fallback with `"repo"` key inline in query string - ✅ Both owner and repo from inline JSON fallbacks - ✅ Non-string variable values rejected by type assertion; falls back to query extraction - ✅ `nil` variables → extraction from query pattern --- *Generated by Test Coverage Improver* *Next run will target the next most complex under-tested function* > Generated by [Test Coverage Improver](https://github.com/github/gh-aw-mcpg/actions/runs/23352624377) · [◷](https://github.com/search?q=repo%3Agithub%2Fgh-aw-mcpg+%22gh-aw-workflow-id%3A+test-coverage-improver%22&type=pullrequests) > [!WARNING] > <details> > <summary>⚠️ Firewall blocked 1 domain</summary> > > The following domain was blocked by the firewall during workflow execution: > > - `proxy.golang.org` > > To allow these domains, add them to the `network.allowed` list in your workflow frontmatter: > > ```yaml > network: > allowed: > - defaults > - "proxy.golang.org" > ``` > > See [Network Configuration](https://github.github.com/gh-aw/reference/network/) for more information. > > </details> <!-- gh-aw-agentic-workflow: Test Coverage Improver, engine: copilot, id: 23352624377, workflow_id: test-coverage-improver, run: https://github.com/github/gh-aw-mcpg/actions/runs/23352624377 --> <!-- gh-aw-workflow-id: test-coverage-improver -->
2 parents 1c83b02 + d1d050d commit 0bfe709

1 file changed

Lines changed: 299 additions & 0 deletions

File tree

Lines changed: 299 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,299 @@
1+
package proxy
2+
3+
import (
4+
"testing"
5+
6+
"github.com/stretchr/testify/assert"
7+
"github.com/stretchr/testify/require"
8+
)
9+
10+
// TestMatchRoute_AdditionalRoutes covers route patterns not exercised by
11+
// the existing TestMatchRoute test: git tags/trees, labels, actions, PR
12+
// review-comments, search/repositories, and the generic repo fallback.
13+
func TestMatchRoute_AdditionalRoutes(t *testing.T) {
14+
tests := []struct {
15+
name string
16+
path string
17+
wantTool string
18+
wantArgs map[string]interface{}
19+
wantNil bool
20+
}{
21+
// Git tag
22+
{
23+
name: "get tag",
24+
path: "/repos/org/repo/git/ref/tags/v1.2.3",
25+
wantTool: "get_tag",
26+
wantArgs: map[string]interface{}{"owner": "org", "repo": "repo", "tag": "v1.2.3"},
27+
},
28+
{
29+
name: "get tag with dots in version",
30+
path: "/repos/github/copilot/git/ref/tags/v2.0.0-beta.1",
31+
wantTool: "get_tag",
32+
wantArgs: map[string]interface{}{"owner": "github", "repo": "copilot", "tag": "v2.0.0-beta.1"},
33+
},
34+
35+
// Git trees
36+
{
37+
name: "get file via git tree",
38+
path: "/repos/org/repo/git/trees/main",
39+
wantTool: "get_file_contents",
40+
wantArgs: map[string]interface{}{"owner": "org", "repo": "repo", "path": "main"},
41+
},
42+
{
43+
name: "get file via git tree with SHA",
44+
path: "/repos/org/repo/git/trees/abc123def",
45+
wantTool: "get_file_contents",
46+
wantArgs: map[string]interface{}{"owner": "org", "repo": "repo", "path": "abc123def"},
47+
},
48+
49+
// Labels collection
50+
{
51+
name: "list labels",
52+
path: "/repos/org/repo/labels",
53+
wantTool: "list_labels",
54+
wantArgs: map[string]interface{}{"owner": "org", "repo": "repo"},
55+
},
56+
57+
// Actions – workflows
58+
{
59+
name: "list workflows",
60+
path: "/repos/github/my-app/actions/workflows",
61+
wantTool: "actions_list",
62+
wantArgs: map[string]interface{}{"owner": "github", "repo": "my-app", "method": "list_workflows"},
63+
},
64+
65+
// Actions – runs
66+
{
67+
name: "list workflow runs",
68+
path: "/repos/github/my-app/actions/runs",
69+
wantTool: "actions_list",
70+
wantArgs: map[string]interface{}{"owner": "github", "repo": "my-app", "method": "list_workflow_runs"},
71+
},
72+
73+
// PR review comments (distinct from /reviews)
74+
{
75+
name: "PR review comments",
76+
path: "/repos/org/repo/pulls/7/comments",
77+
wantTool: "pull_request_read",
78+
wantArgs: map[string]interface{}{"owner": "org", "repo": "repo", "pullNumber": "7", "method": "get_review_comments"},
79+
},
80+
81+
// Search repositories
82+
{
83+
name: "search repositories",
84+
path: "/search/repositories",
85+
wantTool: "search_repositories",
86+
wantArgs: map[string]interface{}{},
87+
},
88+
89+
// Generic repo-scoped fallback — bare repo root
90+
{
91+
name: "bare repo root fallback",
92+
path: "/repos/org/repo",
93+
wantTool: "get_file_contents",
94+
wantArgs: map[string]interface{}{"owner": "org", "repo": "repo"},
95+
},
96+
// Generic repo-scoped fallback — unrecognised sub-path
97+
{
98+
name: "unrecognised sub-path fallback",
99+
path: "/repos/org/repo/unknown-resource",
100+
wantTool: "get_file_contents",
101+
wantArgs: map[string]interface{}{"owner": "org", "repo": "repo"},
102+
},
103+
// Generic repo-scoped fallback — deeply nested unrecognised path
104+
{
105+
name: "deeply nested unrecognised path",
106+
path: "/repos/org/repo/statuses/abc123",
107+
wantTool: "get_file_contents",
108+
wantArgs: map[string]interface{}{"owner": "org", "repo": "repo"},
109+
},
110+
}
111+
112+
for _, tt := range tests {
113+
t.Run(tt.name, func(t *testing.T) {
114+
match := MatchRoute(tt.path)
115+
if tt.wantNil {
116+
assert.Nil(t, match)
117+
return
118+
}
119+
require.NotNil(t, match, "expected route match for %s", tt.path)
120+
assert.Equal(t, tt.wantTool, match.ToolName)
121+
assert.Equal(t, tt.wantArgs, match.Args)
122+
})
123+
}
124+
}
125+
126+
// TestMatchGraphQL_AdditionalPatterns covers GraphQL patterns not exercised by
127+
// the existing TestMatchGraphQL test: projectV2 (list_projects) and the generic
128+
// repository pattern (get_file_contents).
129+
func TestMatchGraphQL_AdditionalPatterns(t *testing.T) {
130+
tests := []struct {
131+
name string
132+
body string
133+
wantTool string
134+
wantNil bool
135+
}{
136+
// ProjectV2 — pattern 6 in graphqlPatterns
137+
{
138+
name: "projectV2 query",
139+
body: `{"query":"query { node(id: \"PVT_abc\") { ... on ProjectV2 { title items(first: 10) { nodes { content { ... on Issue { title } } } } } } }"}`,
140+
wantTool: "list_projects",
141+
},
142+
{
143+
name: "lowercase projectv2",
144+
body: `{"query":"query { viewer { projectv2(number: 1) { title } } }"}`,
145+
wantTool: "list_projects",
146+
},
147+
148+
// Generic repository query — pattern 7 in graphqlPatterns (no issue/PR/search/projectV2)
149+
{
150+
name: "generic repository query",
151+
body: `{"query":"query { repository(owner: \"org\", name: \"repo\") { readme { text } } }"}`,
152+
wantTool: "get_file_contents",
153+
},
154+
{
155+
name: "repository defaultBranchRef query",
156+
body: `{"query":"query { repository(owner: \"octocat\", name: \"hello-world\") { defaultBranchRef { name } } }"}`,
157+
wantTool: "get_file_contents",
158+
},
159+
}
160+
161+
for _, tt := range tests {
162+
t.Run(tt.name, func(t *testing.T) {
163+
match := MatchGraphQL([]byte(tt.body))
164+
if tt.wantNil {
165+
assert.Nil(t, match)
166+
return
167+
}
168+
require.NotNil(t, match, "expected GraphQL match for %q", tt.name)
169+
assert.Equal(t, tt.wantTool, match.ToolName)
170+
})
171+
}
172+
}
173+
174+
// TestMatchGraphQL_ExtractOwnerRepo_EdgeCases covers the branches in
175+
// extractOwnerRepo that are not reached by TestMatchGraphQL_ExtractsOwnerRepo:
176+
//
177+
// - no owner/repo available anywhere
178+
// - owner from variables only (no repo in variables or query)
179+
// - repo from variables "name" key, owner from query pattern
180+
// - owner from variables, repo from query pattern
181+
// - varOwnerPattern fallback (inline JSON in query string)
182+
// - varRepoPattern fallback with "repo" key (inline JSON in query string)
183+
// - both owner and repo from inline JSON fallbacks
184+
// - non-string variable values are ignored
185+
func TestMatchGraphQL_ExtractOwnerRepo_EdgeCases(t *testing.T) {
186+
tests := []struct {
187+
name string
188+
body string
189+
wantTool string
190+
wantNil bool
191+
wantOwner string
192+
wantRepo string
193+
}{
194+
{
195+
// Neither variables nor query text contains owner/repo.
196+
name: "no owner or repo anywhere",
197+
body: `{"query":"query { search(query: \"is:issue\", type: ISSUE, first: 10) { nodes { title } } }"}`,
198+
wantTool: "search_issues",
199+
wantOwner: "",
200+
wantRepo: "",
201+
},
202+
{
203+
// Variables supply owner; neither variables nor queryRepoPattern provide repo.
204+
name: "owner from variables only",
205+
body: `{"query":"query { search(query: \"is:issue\", type: ISSUE, first: 10) { nodes { title } } }","variables":{"owner":"myorg"}}`,
206+
wantTool: "search_issues",
207+
wantOwner: "myorg",
208+
wantRepo: "",
209+
},
210+
{
211+
// Variables supply repo via "name" key; owner comes from the inline query pattern.
212+
name: "owner from query, repo from variables name key",
213+
body: `{"query":"query { repository(owner: \"github\", name: $name) { issues { nodes { title } } } }","variables":{"name":"copilot"}}`,
214+
wantTool: "list_issues",
215+
wantOwner: "github",
216+
wantRepo: "copilot",
217+
},
218+
{
219+
// Variables supply owner; repo comes from the inline query pattern.
220+
name: "owner from variables, repo from query pattern",
221+
body: `{"query":"query { repository(owner: $owner, name: \"my-repo\") { issues { nodes { title } } } }","variables":{"owner":"my-org"}}`,
222+
wantTool: "list_issues",
223+
wantOwner: "my-org",
224+
wantRepo: "my-repo",
225+
},
226+
{
227+
// Variables supply repo via "repo" key (not "name").
228+
name: "repo from variables repo key",
229+
body: `{"query":"query { search(query: \"is:issue\", type: ISSUE, first: 10) { nodes { title } } }","variables":{"owner":"myorg","repo":"myrepo"}}`,
230+
wantTool: "search_issues",
231+
wantOwner: "myorg",
232+
wantRepo: "myrepo",
233+
},
234+
{
235+
// "name" key takes priority over "repo" key when both are present.
236+
name: "name key takes priority over repo key",
237+
body: `{"query":"query { search(query: \"is:issue\", type: ISSUE, first: 10) { nodes { title } } }","variables":{"owner":"myorg","name":"name-repo","repo":"repo-key"}}`,
238+
wantTool: "search_issues",
239+
wantOwner: "myorg",
240+
wantRepo: "name-repo",
241+
},
242+
{
243+
// varOwnerPattern fallback: "owner": "value" literal in the query string,
244+
// no variables provided, no repository(...) pattern in the query.
245+
name: "owner from inline JSON in query string",
246+
body: `{"query":"query { search(query: \"is:issue\") { nodes { title } } } \"owner\": \"inline-org\""}`,
247+
wantTool: "search_issues",
248+
wantOwner: "inline-org",
249+
wantRepo: "",
250+
},
251+
{
252+
// varRepoPattern fallback with "repo" key literal in query.
253+
name: "repo from inline JSON repo key in query string",
254+
body: `{"query":"query { search(query: \"is:issue\") { nodes { title } } } \"repo\": \"inline-repo\""}`,
255+
wantTool: "search_issues",
256+
wantOwner: "",
257+
wantRepo: "inline-repo",
258+
},
259+
{
260+
// Both owner and repo from inline JSON in query string.
261+
name: "both owner and repo from inline JSON in query string",
262+
body: `{"query":"query { search(query: \"is:issue\") { nodes { title } } } \"owner\": \"inline-org\" \"name\": \"inline-repo\""}`,
263+
wantTool: "search_issues",
264+
wantOwner: "inline-org",
265+
wantRepo: "inline-repo",
266+
},
267+
{
268+
// Non-string variable values are skipped by type assertion; falls
269+
// back to extracting from the query string instead.
270+
name: "non-string variable values ignored",
271+
body: `{"query":"query { repository(owner: \"typed-org\", name: \"typed-repo\") { issues { nodes { title } } } }","variables":{"owner":42,"name":true}}`,
272+
wantTool: "list_issues",
273+
wantOwner: "typed-org",
274+
wantRepo: "typed-repo",
275+
},
276+
{
277+
// Nil variables (no "variables" key) — should still extract from query.
278+
name: "nil variables falls back to query extraction",
279+
body: `{"query":"query { repository(owner: \"fallback-org\", name: \"fallback-repo\") { issues { nodes { title } } } }"}`,
280+
wantTool: "list_issues",
281+
wantOwner: "fallback-org",
282+
wantRepo: "fallback-repo",
283+
},
284+
}
285+
286+
for _, tt := range tests {
287+
t.Run(tt.name, func(t *testing.T) {
288+
match := MatchGraphQL([]byte(tt.body))
289+
if tt.wantNil {
290+
assert.Nil(t, match)
291+
return
292+
}
293+
require.NotNil(t, match, "expected GraphQL match for %q", tt.name)
294+
assert.Equal(t, tt.wantTool, match.ToolName, "tool name mismatch")
295+
assert.Equal(t, tt.wantOwner, match.Owner, "owner mismatch")
296+
assert.Equal(t, tt.wantRepo, match.Repo, "repo mismatch")
297+
})
298+
}
299+
}

0 commit comments

Comments
 (0)