[test-improver] Improve tests for proxy package#2230
Merged
Conversation
…elpers
Add test cases to proxy_test.go that cover routes and functions defined
in router.go, graphql.go, and handler.go but not previously tested:
Routes added to TestMatchRoute:
- PR review comments (/pulls/{n}/comments → get_review_comments)
- Tag lookup via git ref (/git/ref/tags/{tag} → get_tag)
- Git tree contents (/git/trees/{path} → get_file_contents)
- Label listing (/labels → list_labels)
- GitHub Actions workflows and runs (actions_list)
- Repository search (/search/repositories)
- Generic repo-scoped fallback route
GraphQL patterns added to TestMatchGraphQL:
- projectV2 query → list_projects
- Generic repository info query → get_file_contents
New test functions:
- TestWriteEmptyResponse: covers all four branches of the empty
response sentinel logic (array, GraphQL data object, plain object,
nil/unknown) and verifies the upstream status code is preserved
- TestCopyResponseHeaders: verifies allowed headers (rate-limit,
pagination, request ID) are copied and unrelated headers are not
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Expands test coverage for the internal/proxy package to better validate REST/GraphQL routing and response helper behavior, reducing the risk of enforcement bypass due to incorrect route matching or response handling.
Changes:
- Adds missing
MatchRoutetest cases for previously untested REST route patterns (including actions, labels list, tags via git ref, PR review comments, repo search, and generic repo fallback). - Adds missing
MatchGraphQLtest cases forprojectV2and genericrepository(...)patterns. - Introduces new unit tests for
writeEmptyResponseandcopyResponseHeadersusinghttptest.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+462
to
+493
| "X-Ratelimit-Limit": []string{"60"}, | ||
| "X-Ratelimit-Remaining": []string{"58"}, | ||
| "X-Ratelimit-Reset": []string{"1609459200"}, | ||
| "X-Ratelimit-Resource": []string{"core"}, | ||
| "X-Ratelimit-Used": []string{"2"}, | ||
| }} | ||
| copyResponseHeaders(w, resp) | ||
| assert.Equal(t, "60", w.Header().Get("X-Ratelimit-Limit")) | ||
| assert.Equal(t, "58", w.Header().Get("X-Ratelimit-Remaining")) | ||
| assert.Equal(t, "1609459200", w.Header().Get("X-Ratelimit-Reset")) | ||
| assert.Equal(t, "core", w.Header().Get("X-Ratelimit-Resource")) | ||
| assert.Equal(t, "2", w.Header().Get("X-Ratelimit-Used")) | ||
| }) | ||
|
|
||
| t.Run("copies pagination and request ID headers", func(t *testing.T) { | ||
| w := httptest.NewRecorder() | ||
| resp := &http.Response{Header: http.Header{ | ||
| "Link": []string{`<https://api.github.com/repos/o/r/issues?page=2>; rel="next"`}, | ||
| "X-Github-Request-Id": []string{"abc-123"}, | ||
| }} | ||
| copyResponseHeaders(w, resp) | ||
| assert.Equal(t, `<https://api.github.com/repos/o/r/issues?page=2>; rel="next"`, w.Header().Get("Link")) | ||
| assert.Equal(t, "abc-123", w.Header().Get("X-Github-Request-Id")) | ||
| }) | ||
|
|
||
| t.Run("absent headers are not written", func(t *testing.T) { | ||
| w := httptest.NewRecorder() | ||
| resp := &http.Response{Header: make(http.Header)} | ||
| copyResponseHeaders(w, resp) | ||
| assert.Empty(t, w.Header().Get("X-Ratelimit-Limit")) | ||
| assert.Empty(t, w.Header().Get("Link")) | ||
| assert.Empty(t, w.Header().Get("X-Github-Request-Id")) |
There was a problem hiding this comment.
In this test the upstream headers are set/asserted using canonicalized keys like X-Ratelimit-*, while copyResponseHeaders uses X-RateLimit-*. Go’s http.Header canonicalization makes this pass, but it’s easy to misread and can become brittle if the implementation ever switches from Header.Get to direct map access. Consider using the same header names as in copyResponseHeaders in both the input resp.Header and assertions for clarity.
Suggested change
| "X-Ratelimit-Limit": []string{"60"}, | |
| "X-Ratelimit-Remaining": []string{"58"}, | |
| "X-Ratelimit-Reset": []string{"1609459200"}, | |
| "X-Ratelimit-Resource": []string{"core"}, | |
| "X-Ratelimit-Used": []string{"2"}, | |
| }} | |
| copyResponseHeaders(w, resp) | |
| assert.Equal(t, "60", w.Header().Get("X-Ratelimit-Limit")) | |
| assert.Equal(t, "58", w.Header().Get("X-Ratelimit-Remaining")) | |
| assert.Equal(t, "1609459200", w.Header().Get("X-Ratelimit-Reset")) | |
| assert.Equal(t, "core", w.Header().Get("X-Ratelimit-Resource")) | |
| assert.Equal(t, "2", w.Header().Get("X-Ratelimit-Used")) | |
| }) | |
| t.Run("copies pagination and request ID headers", func(t *testing.T) { | |
| w := httptest.NewRecorder() | |
| resp := &http.Response{Header: http.Header{ | |
| "Link": []string{`<https://api.github.com/repos/o/r/issues?page=2>; rel="next"`}, | |
| "X-Github-Request-Id": []string{"abc-123"}, | |
| }} | |
| copyResponseHeaders(w, resp) | |
| assert.Equal(t, `<https://api.github.com/repos/o/r/issues?page=2>; rel="next"`, w.Header().Get("Link")) | |
| assert.Equal(t, "abc-123", w.Header().Get("X-Github-Request-Id")) | |
| }) | |
| t.Run("absent headers are not written", func(t *testing.T) { | |
| w := httptest.NewRecorder() | |
| resp := &http.Response{Header: make(http.Header)} | |
| copyResponseHeaders(w, resp) | |
| assert.Empty(t, w.Header().Get("X-Ratelimit-Limit")) | |
| assert.Empty(t, w.Header().Get("Link")) | |
| assert.Empty(t, w.Header().Get("X-Github-Request-Id")) | |
| "X-RateLimit-Limit": []string{"60"}, | |
| "X-RateLimit-Remaining": []string{"58"}, | |
| "X-RateLimit-Reset": []string{"1609459200"}, | |
| "X-RateLimit-Resource": []string{"core"}, | |
| "X-RateLimit-Used": []string{"2"}, | |
| }} | |
| copyResponseHeaders(w, resp) | |
| assert.Equal(t, "60", w.Header().Get("X-RateLimit-Limit")) | |
| assert.Equal(t, "58", w.Header().Get("X-RateLimit-Remaining")) | |
| assert.Equal(t, "1609459200", w.Header().Get("X-RateLimit-Reset")) | |
| assert.Equal(t, "core", w.Header().Get("X-RateLimit-Resource")) | |
| assert.Equal(t, "2", w.Header().Get("X-RateLimit-Used")) | |
| }) | |
| t.Run("copies pagination and request ID headers", func(t *testing.T) { | |
| w := httptest.NewRecorder() | |
| resp := &http.Response{Header: http.Header{ | |
| "Link": []string{`<https://api.github.com/repos/o/r/issues?page=2>; rel="next"`}, | |
| "X-GitHub-Request-Id": []string{"abc-123"}, | |
| }} | |
| copyResponseHeaders(w, resp) | |
| assert.Equal(t, `<https://api.github.com/repos/o/r/issues?page=2>; rel="next"`, w.Header().Get("Link")) | |
| assert.Equal(t, "abc-123", w.Header().Get("X-GitHub-Request-Id")) | |
| }) | |
| t.Run("absent headers are not written", func(t *testing.T) { | |
| w := httptest.NewRecorder() | |
| resp := &http.Response{Header: make(http.Header)} | |
| copyResponseHeaders(w, resp) | |
| assert.Empty(t, w.Header().Get("X-RateLimit-Limit")) | |
| assert.Empty(t, w.Header().Get("Link")) | |
| assert.Empty(t, w.Header().Get("X-GitHub-Request-Id")) |
Comment on lines
+478
to
+485
| resp := &http.Response{Header: http.Header{ | ||
| "Link": []string{`<https://api.github.com/repos/o/r/issues?page=2>; rel="next"`}, | ||
| "X-Github-Request-Id": []string{"abc-123"}, | ||
| }} | ||
| copyResponseHeaders(w, resp) | ||
| assert.Equal(t, `<https://api.github.com/repos/o/r/issues?page=2>; rel="next"`, w.Header().Get("Link")) | ||
| assert.Equal(t, "abc-123", w.Header().Get("X-Github-Request-Id")) | ||
| }) |
There was a problem hiding this comment.
Similar to the rate-limit headers, this subtest uses X-Github-Request-Id while the production copier uses X-GitHub-Request-Id. It works due to header key canonicalization, but aligning the test’s header names with the production list would make intent clearer and reduce brittleness if the implementation changes.
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
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.
File Analyzed
internal/proxy/proxy_test.gointernal/proxyImprovements Made
1. Increased Coverage — Missing Routes in
TestMatchRouteEight routes defined in
router.gohad no test cases:/pulls/{n}/commentspull_request_read(get_review_comments)/git/ref/tags/{tag}get_tag/git/trees/{path}get_file_contents/labels(list)list_labels/actions/workflowsactions_list(list_workflows)/actions/runsactions_list(list_workflow_runs)/search/repositoriessearch_repositories/repos/{o}/{r}/unknownget_file_contents(generic fallback)2. Increased Coverage — Missing GraphQL Patterns in
TestMatchGraphQLTwo GraphQL patterns in
graphql.gohad no test cases:projectV2query →list_projectsrepository(...)info query (no issues/PRs) →get_file_contents3. New Test Functions for Untested Helper Functions
writeEmptyResponseandcopyResponseHeadersinhandler.gohad zero test coverage:✅
TestWriteEmptyResponse: covers all four branches of the empty-response sentinel logic[]interface{}→"[]"map[string]interface{}with"data"key (GraphQL) →{"data":null}map[string]interface{}without"data"key (REST) →"{}"nil/unknown →"[]"(safe default)✅
TestCopyResponseHeaders: four subtests coveringLink(pagination) andX-GitHub-Request-Idare copiedContent-Type,Authorization, custom) are never copied4. Better Imports
Added
"io","net/http", and"net/http/httptest"to support the new HTTP-level tests, enabling direct unit testing of response-writing helpers without a live server.Why These Changes?
proxy_test.goonly tested the four pure routing functions (MatchRoute,StripGHHostPrefix,MatchGraphQL,IsGraphQLPath) but left 8 of 27 route entries, 2 of 7 GraphQL patterns, and both response-writing helpers completely untested. The proxy enforces DIFC security policies by routing requests to the correct guard tool — an incorrect route match silently bypasses enforcement, making routing correctness especially important to verify.Generated by Test Improver Workflow
Focuses on better patterns, increased coverage, and more stable tests