From e85a0ee7204ca5fab7834e4fe70f20e3baaf198f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 15 Dec 2025 14:28:26 +0000 Subject: [PATCH 1/7] Initial plan From ac61a9fc8d7c9fbb0d7c2d1481457086b85c1ac6 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 15 Dec 2025 14:39:55 +0000 Subject: [PATCH 2/7] Add testify/mock-based HTTP mocking infrastructure and migrate git_test.go - Add MockHTTPClientWithHandlers helper function for HTTP-level mocking - Add path pattern matching support for GitHub API endpoints - Migrate pkg/github/git_test.go from go-github-mock to new infrastructure - Keep go-github-mock dependency for now (other files still use it) Co-authored-by: JoannaaKL <67866556+JoannaaKL@users.noreply.github.com> --- go.mod | 1 + go.sum | 2 + pkg/github/git_test.go | 64 ++++------- pkg/github/helper_test.go | 235 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 259 insertions(+), 43 deletions(-) diff --git a/go.mod b/go.mod index 661778fc3..3276d7451 100644 --- a/go.mod +++ b/go.mod @@ -23,6 +23,7 @@ require ( github.com/gorilla/mux v1.8.0 // indirect github.com/josharian/intern v1.0.0 // indirect github.com/mailru/easyjson v0.7.7 // indirect + github.com/stretchr/objx v0.5.2 // indirect github.com/yudai/golcs v0.0.0-20170316035057-ecda9a501e82 // indirect go.yaml.in/yaml/v3 v3.0.4 // indirect golang.org/x/exp v0.0.0-20240719175910-8a7402abbf56 // indirect diff --git a/go.sum b/go.sum index e422a548c..528bd3796 100644 --- a/go.sum +++ b/go.sum @@ -88,6 +88,8 @@ github.com/spf13/pflag v1.0.10/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3A github.com/spf13/viper v1.21.0 h1:x5S+0EU27Lbphp4UKm1C+1oQO+rKx36vfCoaVebLFSU= github.com/spf13/viper v1.21.0/go.mod h1:P0lhsswPGWD/1lZJ9ny3fYnVqxiegrlNrEmgLjbTCAY= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= +github.com/stretchr/objx v0.5.2 h1:xuMeJ0Sdp5ZMRXx/aWO6RZxdr3beISkG5/G/aIRr3pY= +github.com/stretchr/objx v0.5.2/go.mod h1:FRsXN1f5AsAjCGJKqEizvkpNtU+EGNCLh3NxZ/8L+MA= github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= github.com/stretchr/testify v1.6.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U= diff --git a/pkg/github/git_test.go b/pkg/github/git_test.go index 66cbccd6e..ceeb75c44 100644 --- a/pkg/github/git_test.go +++ b/pkg/github/git_test.go @@ -11,7 +11,6 @@ import ( "github.com/github/github-mcp-server/pkg/translations" "github.com/google/go-github/v79/github" "github.com/google/jsonschema-go/jsonschema" - "github.com/migueleliasweb/go-github-mock/src/mock" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -71,16 +70,10 @@ func Test_GetRepositoryTree(t *testing.T) { }{ { name: "successfully get repository tree", - mockedClient: mock.NewMockedHTTPClient( - mock.WithRequestMatchHandler( - mock.GetReposByOwnerByRepo, - mockResponse(t, http.StatusOK, mockRepo), - ), - mock.WithRequestMatchHandler( - mock.GetReposGitTreesByOwnerByRepoByTreeSha, - mockResponse(t, http.StatusOK, mockTree), - ), - ), + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + "GET /repos/{owner}/{repo}": mockResponse(t, http.StatusOK, mockRepo), + "GET /repos/{owner}/{repo}/git/trees/{tree}": mockResponse(t, http.StatusOK, mockTree), + }), requestArgs: map[string]interface{}{ "owner": "owner", "repo": "repo", @@ -88,16 +81,10 @@ func Test_GetRepositoryTree(t *testing.T) { }, { name: "successfully get repository tree with path filter", - mockedClient: mock.NewMockedHTTPClient( - mock.WithRequestMatchHandler( - mock.GetReposByOwnerByRepo, - mockResponse(t, http.StatusOK, mockRepo), - ), - mock.WithRequestMatchHandler( - mock.GetReposGitTreesByOwnerByRepoByTreeSha, - mockResponse(t, http.StatusOK, mockTree), - ), - ), + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + "GET /repos/{owner}/{repo}": mockResponse(t, http.StatusOK, mockRepo), + "GET /repos/{owner}/{repo}/git/trees/{tree}": mockResponse(t, http.StatusOK, mockTree), + }), requestArgs: map[string]interface{}{ "owner": "owner", "repo": "repo", @@ -106,15 +93,12 @@ func Test_GetRepositoryTree(t *testing.T) { }, { name: "repository not found", - mockedClient: mock.NewMockedHTTPClient( - mock.WithRequestMatchHandler( - mock.GetReposByOwnerByRepo, - http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusNotFound) - _, _ = w.Write([]byte(`{"message": "Not Found"}`)) - }), - ), - ), + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + "GET /repos/{owner}/{repo}": http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"message": "Not Found"}`)) + }), + }), requestArgs: map[string]interface{}{ "owner": "owner", "repo": "nonexistent", @@ -124,19 +108,13 @@ func Test_GetRepositoryTree(t *testing.T) { }, { name: "tree not found", - mockedClient: mock.NewMockedHTTPClient( - mock.WithRequestMatchHandler( - mock.GetReposByOwnerByRepo, - mockResponse(t, http.StatusOK, mockRepo), - ), - mock.WithRequestMatchHandler( - mock.GetReposGitTreesByOwnerByRepoByTreeSha, - http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusNotFound) - _, _ = w.Write([]byte(`{"message": "Not Found"}`)) - }), - ), - ), + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + "GET /repos/{owner}/{repo}": mockResponse(t, http.StatusOK, mockRepo), + "GET /repos/{owner}/{repo}/git/trees/{tree}": http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"message": "Not Found"}`)) + }), + }), requestArgs: map[string]interface{}{ "owner": "owner", "repo": "repo", diff --git a/pkg/github/helper_test.go b/pkg/github/helper_test.go index 9c55ba841..336d1cd0b 100644 --- a/pkg/github/helper_test.go +++ b/pkg/github/helper_test.go @@ -1,12 +1,17 @@ package github import ( + "bytes" "encoding/json" + "io" "net/http" + "net/url" + "strings" "testing" "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" ) @@ -272,3 +277,233 @@ func getResourceResult(t *testing.T, result *mcp.CallToolResult) *mcp.ResourceCo require.IsType(t, &mcp.ResourceContents{}, resource.Resource) return resource.Resource } + +// MockRoundTripper is a mock HTTP transport using testify/mock +type MockRoundTripper struct { + mock.Mock + handlers map[string]http.HandlerFunc +} + +// NewMockRoundTripper creates a new mock round tripper +func NewMockRoundTripper() *MockRoundTripper { + return &MockRoundTripper{ + handlers: make(map[string]http.HandlerFunc), + } +} + +// RoundTrip implements the http.RoundTripper interface +func (m *MockRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { + // Normalize the request path and method for matching + key := req.Method + " " + req.URL.Path + + // Check if we have a specific handler for this request + if handler, ok := m.handlers[key]; ok { + // Use httptest.ResponseRecorder to capture the handler's response + recorder := &responseRecorder{ + header: make(http.Header), + body: &bytes.Buffer{}, + } + handler(recorder, req) + + return &http.Response{ + StatusCode: recorder.statusCode, + Header: recorder.header, + Body: io.NopCloser(bytes.NewReader(recorder.body.Bytes())), + Request: req, + }, nil + } + + // Fall back to mock.Mock assertions if defined + args := m.Called(req) + if args.Get(0) == nil { + return nil, args.Error(1) + } + return args.Get(0).(*http.Response), args.Error(1) +} + +// On registers an expectation using testify/mock +func (m *MockRoundTripper) OnRequest(method, path string, handler http.HandlerFunc) *MockRoundTripper { + key := method + " " + path + m.handlers[key] = handler + return m +} + +// OnAny registers a catch-all handler +func (m *MockRoundTripper) OnAny(handler http.HandlerFunc) *MockRoundTripper { + // This is a simple implementation that doesn't use the handler map + // It's kept for compatibility but not recommended + return m +} + +// NewMockHTTPClient creates an HTTP client with a mock transport +func NewMockHTTPClient() (*http.Client, *MockRoundTripper) { + transport := NewMockRoundTripper() + client := &http.Client{Transport: transport} + return client, transport +} + +// responseRecorder is a simple response recorder for the mock transport +type responseRecorder struct { + statusCode int + header http.Header + body *bytes.Buffer +} + +func (r *responseRecorder) Header() http.Header { + return r.header +} + +func (r *responseRecorder) Write(data []byte) (int, error) { + if r.statusCode == 0 { + r.statusCode = http.StatusOK + } + return r.body.Write(data) +} + +func (r *responseRecorder) WriteHeader(statusCode int) { + r.statusCode = statusCode +} + +// matchPath checks if a request path matches a pattern (supports simple wildcards) +func matchPath(pattern, path string) bool { + // Simple exact match for now + if pattern == path { + return true + } + + // Support for path parameters like /repos/{owner}/{repo}/issues/{issue_number} + patternParts := strings.Split(strings.Trim(pattern, "/"), "/") + pathParts := strings.Split(strings.Trim(path, "/"), "/") + + if len(patternParts) != len(pathParts) { + return false + } + + for i := range patternParts { + // Check if this is a path parameter (enclosed in {}) + if strings.HasPrefix(patternParts[i], "{") && strings.HasSuffix(patternParts[i], "}") { + continue // Path parameters match anything + } + if patternParts[i] != pathParts[i] { + return false + } + } + + return true +} + +// MockHTTPClientWithHandler creates an HTTP client with a single handler function +func MockHTTPClientWithHandler(handler http.HandlerFunc) *http.Client { + transport := &mockTransport{handler: handler} + return &http.Client{Transport: transport} +} + +type mockTransport struct { + handler http.HandlerFunc +} + +func (m *mockTransport) RoundTrip(req *http.Request) (*http.Response, error) { + recorder := &responseRecorder{ + header: make(http.Header), + body: &bytes.Buffer{}, + } + m.handler(recorder, req) + + return &http.Response{ + StatusCode: recorder.statusCode, + Header: recorder.header, + Body: io.NopCloser(bytes.NewReader(recorder.body.Bytes())), + Request: req, + }, nil +} + +// MockHTTPClientWithHandlers creates an HTTP client with multiple handlers for different paths +func MockHTTPClientWithHandlers(handlers map[string]http.HandlerFunc) *http.Client { + transport := &multiHandlerTransport{handlers: handlers} + return &http.Client{Transport: transport} +} + +type multiHandlerTransport struct { + handlers map[string]http.HandlerFunc +} + +func (m *multiHandlerTransport) RoundTrip(req *http.Request) (*http.Response, error) { + // Try to find a handler for this request + key := req.Method + " " + req.URL.Path + + // First try exact match + if handler, ok := m.handlers[key]; ok { + recorder := &responseRecorder{ + header: make(http.Header), + body: &bytes.Buffer{}, + } + handler(recorder, req) + + return &http.Response{ + StatusCode: recorder.statusCode, + Header: recorder.header, + Body: io.NopCloser(bytes.NewReader(recorder.body.Bytes())), + Request: req, + }, nil + } + + // Then try pattern matching + for pattern, handler := range m.handlers { + parts := strings.SplitN(pattern, " ", 2) + if len(parts) == 2 { + method, pathPattern := parts[0], parts[1] + if req.Method == method && matchPath(pathPattern, req.URL.Path) { + recorder := &responseRecorder{ + header: make(http.Header), + body: &bytes.Buffer{}, + } + handler(recorder, req) + + return &http.Response{ + StatusCode: recorder.statusCode, + Header: recorder.header, + Body: io.NopCloser(bytes.NewReader(recorder.body.Bytes())), + Request: req, + }, nil + } + } + } + + // No handler found + return &http.Response{ + StatusCode: http.StatusNotFound, + Body: io.NopCloser(bytes.NewReader([]byte("not found"))), + Request: req, + }, nil +} + +// extractPathParams extracts path parameters from a URL path given a pattern +func extractPathParams(pattern, path string) map[string]string { + params := make(map[string]string) + patternParts := strings.Split(strings.Trim(pattern, "/"), "/") + pathParts := strings.Split(strings.Trim(path, "/"), "/") + + if len(patternParts) != len(pathParts) { + return params + } + + for i := range patternParts { + if strings.HasPrefix(patternParts[i], "{") && strings.HasSuffix(patternParts[i], "}") { + paramName := strings.Trim(patternParts[i], "{}") + params[paramName] = pathParts[i] + } + } + + return params +} + +// ParseRequestPath is a helper to extract path parameters +func ParseRequestPath(t *testing.T, req *http.Request, pattern string) url.Values { + t.Helper() + params := extractPathParams(pattern, req.URL.Path) + values := url.Values{} + for k, v := range params { + values.Set(k, v) + } + return values +} From 7ab8bcbb0c6d5915946cae3fced115aa1cf809b1 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 15 Dec 2025 14:46:03 +0000 Subject: [PATCH 3/7] Complete pilot migration and add migration documentation - Migrate pkg/github/code_scanning_test.go to new infrastructure - Add comprehensive migration documentation in docs/testing-migration.md - Fix linter warning in helper_test.go - All tests and lint checks pass - 2 of 16 test files migrated, 14 remaining for incremental migration Co-authored-by: JoannaaKL <67866556+JoannaaKL@users.noreply.github.com> --- docs/testing-migration.md | 130 +++++++++++++++++++++++++++++++ pkg/github/code_scanning_test.go | 61 ++++++--------- pkg/github/helper_test.go | 34 ++++---- 3 files changed, 171 insertions(+), 54 deletions(-) create mode 100644 docs/testing-migration.md diff --git a/docs/testing-migration.md b/docs/testing-migration.md new file mode 100644 index 000000000..2aa80beb9 --- /dev/null +++ b/docs/testing-migration.md @@ -0,0 +1,130 @@ +# Testing Infrastructure Migration + +## Overview + +This document describes the ongoing migration from `migueleliasweb/go-github-mock` to `stretchr/testify`-based HTTP mocking infrastructure. + +## Motivation + +As described in issue #1492, we are migrating from `go-github-mock` to consolidate our testing dependencies: + +1. **Dependency Consolidation**: We already use `stretchr/testify` for assertions +2. **Maintenance**: Reduce the number of external dependencies +3. **Flexibility**: Custom HTTP mocking provides more control over test scenarios +4. **Clarity**: Simpler, more readable test setup + +## New Testing Infrastructure + +The new mocking infrastructure is located in `pkg/github/helper_test.go` and provides: + +### MockHTTPClientWithHandlers + +Creates an HTTP client with multiple handlers for different API endpoints: + +```go +mockedClient := MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + "GET /repos/{owner}/{repo}": mockResponse(t, http.StatusOK, mockRepo), + "GET /repos/{owner}/{repo}/git/trees/{tree}": mockResponse(t, http.StatusOK, mockTree), +}) +``` + +### Path Pattern Matching + +Supports GitHub API path patterns with parameter placeholders: +- `{owner}`, `{repo}`, `{tree}`, etc. are treated as wildcards +- Exact paths are matched first, then patterns +- Query parameters can be validated using the existing `expectQueryParams` helper + +### Helper Functions + +- `mockResponse(t, statusCode, body)` - Creates JSON responses +- `expectQueryParams(t, params).andThen(handler)` - Validates query parameters +- `MockHTTPClientWithHandler(handler)` - Single handler for all requests + +## Migration Pattern + +### Before (using go-github-mock): + +```go +mockedClient := mock.NewMockedHTTPClient( + mock.WithRequestMatch( + mock.GetReposByOwnerByRepo, + mockRepo, + ), + mock.WithRequestMatchHandler( + mock.GetReposGitTreesByOwnerByRepoByTreeSha, + mockResponse(t, http.StatusOK, mockTree), + ), +) +``` + +### After (using new infrastructure): + +```go +mockedClient := MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + "GET /repos/{owner}/{repo}": mockResponse(t, http.StatusOK, mockRepo), + "GET /repos/{owner}/{repo}/git/trees/{tree}": mockResponse(t, http.StatusOK, mockTree), +}) +``` + +## Migration Status + +### Completed +- ✅ `pkg/github/git_test.go` - 196 lines (pilot migration) +- ✅ `pkg/github/code_scanning_test.go` - 258 lines + +### Remaining (in order of size, smallest first) +- ⏳ `pkg/github/secret_scanning_test.go` - 263 lines +- ⏳ `pkg/github/dependabot_test.go` - 269 lines +- ⏳ `pkg/github/repository_resource_test.go` - 314 lines +- ⏳ `pkg/github/context_tools_test.go` - 498 lines +- ⏳ `pkg/github/security_advisories_test.go` - 546 lines +- ⏳ `pkg/github/gists_test.go` - 621 lines +- ⏳ `pkg/github/search_test.go` - 760 lines +- ⏳ `pkg/github/notifications_test.go` - 783 lines +- ⏳ `pkg/github/actions_test.go` - 1442 lines +- ⏳ `pkg/github/projects_test.go` - 1684 lines +- ⏳ `pkg/github/pullrequests_test.go` - 3263 lines +- ⏳ `pkg/github/repositories_test.go` - 3472 lines +- ⏳ `pkg/github/issues_test.go` - 3705 lines +- ⏳ `pkg/raw/raw_mock.go` - Special case (defines endpoint patterns) + +**Total remaining: ~14,210 lines of test code across 14 files** + +## API Endpoint Patterns + +Common GitHub API endpoints and their patterns: + +| Endpoint | Pattern | +|----------|---------| +| Get Repository | `GET /repos/{owner}/{repo}` | +| Get Tree | `GET /repos/{owner}/{repo}/git/trees/{tree}` | +| Get Issue | `GET /repos/{owner}/{repo}/issues/{issue_number}` | +| List Issues | `GET /repos/{owner}/{repo}/issues` | +| Get Pull Request | `GET /repos/{owner}/{repo}/pulls/{pull_number}` | +| Code Scanning Alert | `GET /repos/{owner}/{repo}/code-scanning/alerts/{alert_number}` | +| Secret Scanning Alert | `GET /repos/{owner}/{repo}/secret-scanning/alerts/{alert_number}` | + +For a complete reference, see the [GitHub REST API documentation](https://docs.github.com/en/rest). + +## Testing Best Practices + +1. **Use pattern matching for flexibility**: `{owner}`, `{repo}`, etc. +2. **Validate query parameters when needed**: Use `expectQueryParams` +3. **Keep test setup close to test cases**: Define handlers inline +4. **Reuse mock data across test cases**: Define once at the top +5. **Test both success and failure cases**: Include error scenarios + +## Next Steps + +1. Continue migrating test files incrementally (smallest first) +2. Once all files are migrated, remove `go-github-mock` dependency +3. Update `pkg/raw/raw_mock.go` to use new patterns +4. Run `go mod tidy` to clean up dependencies +5. Update third-party licenses with `./script/licenses` + +## References + +- Issue: #1492 +- testify documentation: https://pkg.go.dev/github.com/stretchr/testify +- GitHub API: https://docs.github.com/en/rest diff --git a/pkg/github/code_scanning_test.go b/pkg/github/code_scanning_test.go index 13e89fc30..9709cfcaf 100644 --- a/pkg/github/code_scanning_test.go +++ b/pkg/github/code_scanning_test.go @@ -10,7 +10,6 @@ import ( "github.com/github/github-mcp-server/pkg/translations" "github.com/google/go-github/v79/github" "github.com/google/jsonschema-go/jsonschema" - "github.com/migueleliasweb/go-github-mock/src/mock" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -50,12 +49,9 @@ func Test_GetCodeScanningAlert(t *testing.T) { }{ { name: "successful alert fetch", - mockedClient: mock.NewMockedHTTPClient( - mock.WithRequestMatch( - mock.GetReposCodeScanningAlertsByOwnerByRepoByAlertNumber, - mockAlert, - ), - ), + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + "GET /repos/{owner}/{repo}/code-scanning/alerts/{alert_number}": mockResponse(t, http.StatusOK, mockAlert), + }), requestArgs: map[string]interface{}{ "owner": "owner", "repo": "repo", @@ -66,15 +62,12 @@ func Test_GetCodeScanningAlert(t *testing.T) { }, { name: "alert fetch fails", - mockedClient: mock.NewMockedHTTPClient( - mock.WithRequestMatchHandler( - mock.GetReposCodeScanningAlertsByOwnerByRepoByAlertNumber, - http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusNotFound) - _, _ = w.Write([]byte(`{"message": "Not Found"}`)) - }), - ), - ), + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + "GET /repos/{owner}/{repo}/code-scanning/alerts/{alert_number}": http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"message": "Not Found"}`)) + }), + }), requestArgs: map[string]interface{}{ "owner": "owner", "repo": "repo", @@ -171,19 +164,16 @@ func Test_ListCodeScanningAlerts(t *testing.T) { }{ { name: "successful alerts listing", - mockedClient: mock.NewMockedHTTPClient( - mock.WithRequestMatchHandler( - mock.GetReposCodeScanningAlertsByOwnerByRepo, - expectQueryParams(t, map[string]string{ - "ref": "main", - "state": "open", - "severity": "high", - "tool_name": "codeql", - }).andThen( - mockResponse(t, http.StatusOK, mockAlerts), - ), + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + "GET /repos/{owner}/{repo}/code-scanning/alerts": expectQueryParams(t, map[string]string{ + "ref": "main", + "state": "open", + "severity": "high", + "tool_name": "codeql", + }).andThen( + mockResponse(t, http.StatusOK, mockAlerts), ), - ), + }), requestArgs: map[string]interface{}{ "owner": "owner", "repo": "repo", @@ -197,15 +187,12 @@ func Test_ListCodeScanningAlerts(t *testing.T) { }, { name: "alerts listing fails", - mockedClient: mock.NewMockedHTTPClient( - mock.WithRequestMatchHandler( - mock.GetReposCodeScanningAlertsByOwnerByRepo, - http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusUnauthorized) - _, _ = w.Write([]byte(`{"message": "Unauthorized access"}`)) - }), - ), - ), + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + "GET /repos/{owner}/{repo}/code-scanning/alerts": http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + _, _ = w.Write([]byte(`{"message": "Unauthorized access"}`)) + }), + }), requestArgs: map[string]interface{}{ "owner": "owner", "repo": "repo", diff --git a/pkg/github/helper_test.go b/pkg/github/helper_test.go index 336d1cd0b..a2d157949 100644 --- a/pkg/github/helper_test.go +++ b/pkg/github/helper_test.go @@ -295,7 +295,7 @@ func NewMockRoundTripper() *MockRoundTripper { func (m *MockRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { // Normalize the request path and method for matching key := req.Method + " " + req.URL.Path - + // Check if we have a specific handler for this request if handler, ok := m.handlers[key]; ok { // Use httptest.ResponseRecorder to capture the handler's response @@ -304,7 +304,7 @@ func (m *MockRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) body: &bytes.Buffer{}, } handler(recorder, req) - + return &http.Response{ StatusCode: recorder.statusCode, Header: recorder.header, @@ -312,7 +312,7 @@ func (m *MockRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) Request: req, }, nil } - + // Fall back to mock.Mock assertions if defined args := m.Called(req) if args.Get(0) == nil { @@ -329,7 +329,7 @@ func (m *MockRoundTripper) OnRequest(method, path string, handler http.HandlerFu } // OnAny registers a catch-all handler -func (m *MockRoundTripper) OnAny(handler http.HandlerFunc) *MockRoundTripper { +func (m *MockRoundTripper) OnAny(_ http.HandlerFunc) *MockRoundTripper { // This is a simple implementation that doesn't use the handler map // It's kept for compatibility but not recommended return m @@ -370,15 +370,15 @@ func matchPath(pattern, path string) bool { if pattern == path { return true } - + // Support for path parameters like /repos/{owner}/{repo}/issues/{issue_number} patternParts := strings.Split(strings.Trim(pattern, "/"), "/") pathParts := strings.Split(strings.Trim(path, "/"), "/") - + if len(patternParts) != len(pathParts) { return false } - + for i := range patternParts { // Check if this is a path parameter (enclosed in {}) if strings.HasPrefix(patternParts[i], "{") && strings.HasSuffix(patternParts[i], "}") { @@ -388,7 +388,7 @@ func matchPath(pattern, path string) bool { return false } } - + return true } @@ -408,7 +408,7 @@ func (m *mockTransport) RoundTrip(req *http.Request) (*http.Response, error) { body: &bytes.Buffer{}, } m.handler(recorder, req) - + return &http.Response{ StatusCode: recorder.statusCode, Header: recorder.header, @@ -430,7 +430,7 @@ type multiHandlerTransport struct { func (m *multiHandlerTransport) RoundTrip(req *http.Request) (*http.Response, error) { // Try to find a handler for this request key := req.Method + " " + req.URL.Path - + // First try exact match if handler, ok := m.handlers[key]; ok { recorder := &responseRecorder{ @@ -438,7 +438,7 @@ func (m *multiHandlerTransport) RoundTrip(req *http.Request) (*http.Response, er body: &bytes.Buffer{}, } handler(recorder, req) - + return &http.Response{ StatusCode: recorder.statusCode, Header: recorder.header, @@ -446,7 +446,7 @@ func (m *multiHandlerTransport) RoundTrip(req *http.Request) (*http.Response, er Request: req, }, nil } - + // Then try pattern matching for pattern, handler := range m.handlers { parts := strings.SplitN(pattern, " ", 2) @@ -458,7 +458,7 @@ func (m *multiHandlerTransport) RoundTrip(req *http.Request) (*http.Response, er body: &bytes.Buffer{}, } handler(recorder, req) - + return &http.Response{ StatusCode: recorder.statusCode, Header: recorder.header, @@ -468,7 +468,7 @@ func (m *multiHandlerTransport) RoundTrip(req *http.Request) (*http.Response, er } } } - + // No handler found return &http.Response{ StatusCode: http.StatusNotFound, @@ -482,18 +482,18 @@ func extractPathParams(pattern, path string) map[string]string { params := make(map[string]string) patternParts := strings.Split(strings.Trim(pattern, "/"), "/") pathParts := strings.Split(strings.Trim(path, "/"), "/") - + if len(patternParts) != len(pathParts) { return params } - + for i := range patternParts { if strings.HasPrefix(patternParts[i], "{") && strings.HasSuffix(patternParts[i], "}") { paramName := strings.Trim(patternParts[i], "{}") params[paramName] = pathParts[i] } } - + return params } From 9d985bbbb491c0f3bc21a5f0703ed261b89098bb Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 15 Dec 2025 14:48:34 +0000 Subject: [PATCH 4/7] Address code review feedback: consolidate transport implementations - Remove unused OnAny method - Extract executeHandler helper to eliminate code duplication - Consolidate MockHTTPClientWithHandler to use MockHTTPClientWithHandlers - Simplify transport implementation (single multiHandlerTransport) - All tests and lint checks pass Co-authored-by: JoannaaKL <67866556+JoannaaKL@users.noreply.github.com> --- pkg/github/helper_test.go | 66 +++++++++++++-------------------------- 1 file changed, 22 insertions(+), 44 deletions(-) diff --git a/pkg/github/helper_test.go b/pkg/github/helper_test.go index a2d157949..e1a47d386 100644 --- a/pkg/github/helper_test.go +++ b/pkg/github/helper_test.go @@ -328,13 +328,6 @@ func (m *MockRoundTripper) OnRequest(method, path string, handler http.HandlerFu return m } -// OnAny registers a catch-all handler -func (m *MockRoundTripper) OnAny(_ http.HandlerFunc) *MockRoundTripper { - // This is a simple implementation that doesn't use the handler map - // It's kept for compatibility but not recommended - return m -} - // NewMockHTTPClient creates an HTTP client with a mock transport func NewMockHTTPClient() (*http.Client, *MockRoundTripper) { transport := NewMockRoundTripper() @@ -392,29 +385,28 @@ func matchPath(pattern, path string) bool { return true } -// MockHTTPClientWithHandler creates an HTTP client with a single handler function -func MockHTTPClientWithHandler(handler http.HandlerFunc) *http.Client { - transport := &mockTransport{handler: handler} - return &http.Client{Transport: transport} -} - -type mockTransport struct { - handler http.HandlerFunc -} - -func (m *mockTransport) RoundTrip(req *http.Request) (*http.Response, error) { +// executeHandler executes an HTTP handler and returns the response +func executeHandler(handler http.HandlerFunc, req *http.Request) *http.Response { recorder := &responseRecorder{ header: make(http.Header), body: &bytes.Buffer{}, } - m.handler(recorder, req) + handler(recorder, req) return &http.Response{ StatusCode: recorder.statusCode, Header: recorder.header, Body: io.NopCloser(bytes.NewReader(recorder.body.Bytes())), Request: req, - }, nil + } +} + +// MockHTTPClientWithHandler creates an HTTP client with a single handler function +func MockHTTPClientWithHandler(handler http.HandlerFunc) *http.Client { + handlers := map[string]http.HandlerFunc{ + "": handler, // Empty key acts as catch-all + } + return MockHTTPClientWithHandlers(handlers) } // MockHTTPClientWithHandlers creates an HTTP client with multiple handlers for different paths @@ -428,43 +420,29 @@ type multiHandlerTransport struct { } func (m *multiHandlerTransport) RoundTrip(req *http.Request) (*http.Response, error) { + // Check for catch-all handler + if handler, ok := m.handlers[""]; ok { + return executeHandler(handler, req), nil + } + // Try to find a handler for this request key := req.Method + " " + req.URL.Path // First try exact match if handler, ok := m.handlers[key]; ok { - recorder := &responseRecorder{ - header: make(http.Header), - body: &bytes.Buffer{}, - } - handler(recorder, req) - - return &http.Response{ - StatusCode: recorder.statusCode, - Header: recorder.header, - Body: io.NopCloser(bytes.NewReader(recorder.body.Bytes())), - Request: req, - }, nil + return executeHandler(handler, req), nil } // Then try pattern matching for pattern, handler := range m.handlers { + if pattern == "" { + continue // Skip catch-all + } parts := strings.SplitN(pattern, " ", 2) if len(parts) == 2 { method, pathPattern := parts[0], parts[1] if req.Method == method && matchPath(pathPattern, req.URL.Path) { - recorder := &responseRecorder{ - header: make(http.Header), - body: &bytes.Buffer{}, - } - handler(recorder, req) - - return &http.Response{ - StatusCode: recorder.statusCode, - Header: recorder.header, - Body: io.NopCloser(bytes.NewReader(recorder.body.Bytes())), - Request: req, - }, nil + return executeHandler(handler, req), nil } } } From f4f426b1bb8583ecd9dfcb3f3f1dd2dc751826cf Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 15 Dec 2025 15:15:08 +0000 Subject: [PATCH 5/7] Remove docs/testing-migration.md per review feedback Co-authored-by: JoannaaKL <67866556+JoannaaKL@users.noreply.github.com> --- docs/testing-migration.md | 130 -------------------------------------- 1 file changed, 130 deletions(-) delete mode 100644 docs/testing-migration.md diff --git a/docs/testing-migration.md b/docs/testing-migration.md deleted file mode 100644 index 2aa80beb9..000000000 --- a/docs/testing-migration.md +++ /dev/null @@ -1,130 +0,0 @@ -# Testing Infrastructure Migration - -## Overview - -This document describes the ongoing migration from `migueleliasweb/go-github-mock` to `stretchr/testify`-based HTTP mocking infrastructure. - -## Motivation - -As described in issue #1492, we are migrating from `go-github-mock` to consolidate our testing dependencies: - -1. **Dependency Consolidation**: We already use `stretchr/testify` for assertions -2. **Maintenance**: Reduce the number of external dependencies -3. **Flexibility**: Custom HTTP mocking provides more control over test scenarios -4. **Clarity**: Simpler, more readable test setup - -## New Testing Infrastructure - -The new mocking infrastructure is located in `pkg/github/helper_test.go` and provides: - -### MockHTTPClientWithHandlers - -Creates an HTTP client with multiple handlers for different API endpoints: - -```go -mockedClient := MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - "GET /repos/{owner}/{repo}": mockResponse(t, http.StatusOK, mockRepo), - "GET /repos/{owner}/{repo}/git/trees/{tree}": mockResponse(t, http.StatusOK, mockTree), -}) -``` - -### Path Pattern Matching - -Supports GitHub API path patterns with parameter placeholders: -- `{owner}`, `{repo}`, `{tree}`, etc. are treated as wildcards -- Exact paths are matched first, then patterns -- Query parameters can be validated using the existing `expectQueryParams` helper - -### Helper Functions - -- `mockResponse(t, statusCode, body)` - Creates JSON responses -- `expectQueryParams(t, params).andThen(handler)` - Validates query parameters -- `MockHTTPClientWithHandler(handler)` - Single handler for all requests - -## Migration Pattern - -### Before (using go-github-mock): - -```go -mockedClient := mock.NewMockedHTTPClient( - mock.WithRequestMatch( - mock.GetReposByOwnerByRepo, - mockRepo, - ), - mock.WithRequestMatchHandler( - mock.GetReposGitTreesByOwnerByRepoByTreeSha, - mockResponse(t, http.StatusOK, mockTree), - ), -) -``` - -### After (using new infrastructure): - -```go -mockedClient := MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - "GET /repos/{owner}/{repo}": mockResponse(t, http.StatusOK, mockRepo), - "GET /repos/{owner}/{repo}/git/trees/{tree}": mockResponse(t, http.StatusOK, mockTree), -}) -``` - -## Migration Status - -### Completed -- ✅ `pkg/github/git_test.go` - 196 lines (pilot migration) -- ✅ `pkg/github/code_scanning_test.go` - 258 lines - -### Remaining (in order of size, smallest first) -- ⏳ `pkg/github/secret_scanning_test.go` - 263 lines -- ⏳ `pkg/github/dependabot_test.go` - 269 lines -- ⏳ `pkg/github/repository_resource_test.go` - 314 lines -- ⏳ `pkg/github/context_tools_test.go` - 498 lines -- ⏳ `pkg/github/security_advisories_test.go` - 546 lines -- ⏳ `pkg/github/gists_test.go` - 621 lines -- ⏳ `pkg/github/search_test.go` - 760 lines -- ⏳ `pkg/github/notifications_test.go` - 783 lines -- ⏳ `pkg/github/actions_test.go` - 1442 lines -- ⏳ `pkg/github/projects_test.go` - 1684 lines -- ⏳ `pkg/github/pullrequests_test.go` - 3263 lines -- ⏳ `pkg/github/repositories_test.go` - 3472 lines -- ⏳ `pkg/github/issues_test.go` - 3705 lines -- ⏳ `pkg/raw/raw_mock.go` - Special case (defines endpoint patterns) - -**Total remaining: ~14,210 lines of test code across 14 files** - -## API Endpoint Patterns - -Common GitHub API endpoints and their patterns: - -| Endpoint | Pattern | -|----------|---------| -| Get Repository | `GET /repos/{owner}/{repo}` | -| Get Tree | `GET /repos/{owner}/{repo}/git/trees/{tree}` | -| Get Issue | `GET /repos/{owner}/{repo}/issues/{issue_number}` | -| List Issues | `GET /repos/{owner}/{repo}/issues` | -| Get Pull Request | `GET /repos/{owner}/{repo}/pulls/{pull_number}` | -| Code Scanning Alert | `GET /repos/{owner}/{repo}/code-scanning/alerts/{alert_number}` | -| Secret Scanning Alert | `GET /repos/{owner}/{repo}/secret-scanning/alerts/{alert_number}` | - -For a complete reference, see the [GitHub REST API documentation](https://docs.github.com/en/rest). - -## Testing Best Practices - -1. **Use pattern matching for flexibility**: `{owner}`, `{repo}`, etc. -2. **Validate query parameters when needed**: Use `expectQueryParams` -3. **Keep test setup close to test cases**: Define handlers inline -4. **Reuse mock data across test cases**: Define once at the top -5. **Test both success and failure cases**: Include error scenarios - -## Next Steps - -1. Continue migrating test files incrementally (smallest first) -2. Once all files are migrated, remove `go-github-mock` dependency -3. Update `pkg/raw/raw_mock.go` to use new patterns -4. Run `go mod tidy` to clean up dependencies -5. Update third-party licenses with `./script/licenses` - -## References - -- Issue: #1492 -- testify documentation: https://pkg.go.dev/github.com/stretchr/testify -- GitHub API: https://docs.github.com/en/rest From 9e403f5eff79a904b3f664add1bf5224522dca05 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 15 Dec 2025 16:01:22 +0000 Subject: [PATCH 6/7] Reuse mock.EndpointPattern constants instead of hardcoded paths - Add mock import to code_scanning_test.go and git_test.go - Replace hardcoded paths with mock.GetRepos*.Pattern references - Ensures consistency with existing test patterns and easier maintenance Co-authored-by: JoannaaKL <67866556+JoannaaKL@users.noreply.github.com> --- pkg/github/code_scanning_test.go | 9 +++++---- pkg/github/git_test.go | 15 ++++++++------- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/pkg/github/code_scanning_test.go b/pkg/github/code_scanning_test.go index 9709cfcaf..9b9597986 100644 --- a/pkg/github/code_scanning_test.go +++ b/pkg/github/code_scanning_test.go @@ -10,6 +10,7 @@ import ( "github.com/github/github-mcp-server/pkg/translations" "github.com/google/go-github/v79/github" "github.com/google/jsonschema-go/jsonschema" + "github.com/migueleliasweb/go-github-mock/src/mock" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -50,7 +51,7 @@ func Test_GetCodeScanningAlert(t *testing.T) { { name: "successful alert fetch", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - "GET /repos/{owner}/{repo}/code-scanning/alerts/{alert_number}": mockResponse(t, http.StatusOK, mockAlert), + mock.GetReposCodeScanningAlertsByOwnerByRepoByAlertNumber.Method + " " + mock.GetReposCodeScanningAlertsByOwnerByRepoByAlertNumber.Pattern: mockResponse(t, http.StatusOK, mockAlert), }), requestArgs: map[string]interface{}{ "owner": "owner", @@ -63,7 +64,7 @@ func Test_GetCodeScanningAlert(t *testing.T) { { name: "alert fetch fails", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - "GET /repos/{owner}/{repo}/code-scanning/alerts/{alert_number}": http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + mock.GetReposCodeScanningAlertsByOwnerByRepoByAlertNumber.Method + " " + mock.GetReposCodeScanningAlertsByOwnerByRepoByAlertNumber.Pattern: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusNotFound) _, _ = w.Write([]byte(`{"message": "Not Found"}`)) }), @@ -165,7 +166,7 @@ func Test_ListCodeScanningAlerts(t *testing.T) { { name: "successful alerts listing", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - "GET /repos/{owner}/{repo}/code-scanning/alerts": expectQueryParams(t, map[string]string{ + mock.GetReposCodeScanningAlertsByOwnerByRepo.Method + " " + mock.GetReposCodeScanningAlertsByOwnerByRepo.Pattern: expectQueryParams(t, map[string]string{ "ref": "main", "state": "open", "severity": "high", @@ -188,7 +189,7 @@ func Test_ListCodeScanningAlerts(t *testing.T) { { name: "alerts listing fails", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - "GET /repos/{owner}/{repo}/code-scanning/alerts": http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + mock.GetReposCodeScanningAlertsByOwnerByRepo.Method + " " + mock.GetReposCodeScanningAlertsByOwnerByRepo.Pattern: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusUnauthorized) _, _ = w.Write([]byte(`{"message": "Unauthorized access"}`)) }), diff --git a/pkg/github/git_test.go b/pkg/github/git_test.go index ceeb75c44..990bdb797 100644 --- a/pkg/github/git_test.go +++ b/pkg/github/git_test.go @@ -11,6 +11,7 @@ import ( "github.com/github/github-mcp-server/pkg/translations" "github.com/google/go-github/v79/github" "github.com/google/jsonschema-go/jsonschema" + "github.com/migueleliasweb/go-github-mock/src/mock" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -71,8 +72,8 @@ func Test_GetRepositoryTree(t *testing.T) { { name: "successfully get repository tree", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - "GET /repos/{owner}/{repo}": mockResponse(t, http.StatusOK, mockRepo), - "GET /repos/{owner}/{repo}/git/trees/{tree}": mockResponse(t, http.StatusOK, mockTree), + mock.GetReposByOwnerByRepo.Method + " " + mock.GetReposByOwnerByRepo.Pattern: mockResponse(t, http.StatusOK, mockRepo), + mock.GetReposGitTreesByOwnerByRepoByTreeSha.Method + " " + mock.GetReposGitTreesByOwnerByRepoByTreeSha.Pattern: mockResponse(t, http.StatusOK, mockTree), }), requestArgs: map[string]interface{}{ "owner": "owner", @@ -82,8 +83,8 @@ func Test_GetRepositoryTree(t *testing.T) { { name: "successfully get repository tree with path filter", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - "GET /repos/{owner}/{repo}": mockResponse(t, http.StatusOK, mockRepo), - "GET /repos/{owner}/{repo}/git/trees/{tree}": mockResponse(t, http.StatusOK, mockTree), + mock.GetReposByOwnerByRepo.Method + " " + mock.GetReposByOwnerByRepo.Pattern: mockResponse(t, http.StatusOK, mockRepo), + mock.GetReposGitTreesByOwnerByRepoByTreeSha.Method + " " + mock.GetReposGitTreesByOwnerByRepoByTreeSha.Pattern: mockResponse(t, http.StatusOK, mockTree), }), requestArgs: map[string]interface{}{ "owner": "owner", @@ -94,7 +95,7 @@ func Test_GetRepositoryTree(t *testing.T) { { name: "repository not found", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - "GET /repos/{owner}/{repo}": http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + mock.GetReposByOwnerByRepo.Method + " " + mock.GetReposByOwnerByRepo.Pattern: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusNotFound) _, _ = w.Write([]byte(`{"message": "Not Found"}`)) }), @@ -109,8 +110,8 @@ func Test_GetRepositoryTree(t *testing.T) { { name: "tree not found", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - "GET /repos/{owner}/{repo}": mockResponse(t, http.StatusOK, mockRepo), - "GET /repos/{owner}/{repo}/git/trees/{tree}": http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + mock.GetReposByOwnerByRepo.Method + " " + mock.GetReposByOwnerByRepo.Pattern: mockResponse(t, http.StatusOK, mockRepo), + mock.GetReposGitTreesByOwnerByRepoByTreeSha.Method + " " + mock.GetReposGitTreesByOwnerByRepoByTreeSha.Pattern: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusNotFound) _, _ = w.Write([]byte(`{"message": "Not Found"}`)) }), From b96a79187051b087a98be18267cca29ef27d9d27 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 15 Dec 2025 16:16:56 +0000 Subject: [PATCH 7/7] Define endpoint path constants in this repo instead of reusing go-github-mock - Add endpoint pattern constants to helper_test.go - Update code_scanning_test.go to use new constants - Update git_test.go to use new constants - Removes dependency on go-github-mock endpoint patterns - Prepares for eventual removal of go-github-mock dependency Co-authored-by: JoannaaKL <67866556+JoannaaKL@users.noreply.github.com> --- pkg/github/code_scanning_test.go | 9 ++++----- pkg/github/git_test.go | 15 +++++++-------- pkg/github/helper_test.go | 14 ++++++++++++++ 3 files changed, 25 insertions(+), 13 deletions(-) diff --git a/pkg/github/code_scanning_test.go b/pkg/github/code_scanning_test.go index 9b9597986..4f9a00145 100644 --- a/pkg/github/code_scanning_test.go +++ b/pkg/github/code_scanning_test.go @@ -10,7 +10,6 @@ import ( "github.com/github/github-mcp-server/pkg/translations" "github.com/google/go-github/v79/github" "github.com/google/jsonschema-go/jsonschema" - "github.com/migueleliasweb/go-github-mock/src/mock" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -51,7 +50,7 @@ func Test_GetCodeScanningAlert(t *testing.T) { { name: "successful alert fetch", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - mock.GetReposCodeScanningAlertsByOwnerByRepoByAlertNumber.Method + " " + mock.GetReposCodeScanningAlertsByOwnerByRepoByAlertNumber.Pattern: mockResponse(t, http.StatusOK, mockAlert), + GetReposCodeScanningAlertsByOwnerByRepoByAlertNumber: mockResponse(t, http.StatusOK, mockAlert), }), requestArgs: map[string]interface{}{ "owner": "owner", @@ -64,7 +63,7 @@ func Test_GetCodeScanningAlert(t *testing.T) { { name: "alert fetch fails", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - mock.GetReposCodeScanningAlertsByOwnerByRepoByAlertNumber.Method + " " + mock.GetReposCodeScanningAlertsByOwnerByRepoByAlertNumber.Pattern: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + GetReposCodeScanningAlertsByOwnerByRepoByAlertNumber: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusNotFound) _, _ = w.Write([]byte(`{"message": "Not Found"}`)) }), @@ -166,7 +165,7 @@ func Test_ListCodeScanningAlerts(t *testing.T) { { name: "successful alerts listing", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - mock.GetReposCodeScanningAlertsByOwnerByRepo.Method + " " + mock.GetReposCodeScanningAlertsByOwnerByRepo.Pattern: expectQueryParams(t, map[string]string{ + GetReposCodeScanningAlertsByOwnerByRepo: expectQueryParams(t, map[string]string{ "ref": "main", "state": "open", "severity": "high", @@ -189,7 +188,7 @@ func Test_ListCodeScanningAlerts(t *testing.T) { { name: "alerts listing fails", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - mock.GetReposCodeScanningAlertsByOwnerByRepo.Method + " " + mock.GetReposCodeScanningAlertsByOwnerByRepo.Pattern: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + GetReposCodeScanningAlertsByOwnerByRepo: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusUnauthorized) _, _ = w.Write([]byte(`{"message": "Unauthorized access"}`)) }), diff --git a/pkg/github/git_test.go b/pkg/github/git_test.go index 990bdb797..7a08326bf 100644 --- a/pkg/github/git_test.go +++ b/pkg/github/git_test.go @@ -11,7 +11,6 @@ import ( "github.com/github/github-mcp-server/pkg/translations" "github.com/google/go-github/v79/github" "github.com/google/jsonschema-go/jsonschema" - "github.com/migueleliasweb/go-github-mock/src/mock" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -72,8 +71,8 @@ func Test_GetRepositoryTree(t *testing.T) { { name: "successfully get repository tree", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - mock.GetReposByOwnerByRepo.Method + " " + mock.GetReposByOwnerByRepo.Pattern: mockResponse(t, http.StatusOK, mockRepo), - mock.GetReposGitTreesByOwnerByRepoByTreeSha.Method + " " + mock.GetReposGitTreesByOwnerByRepoByTreeSha.Pattern: mockResponse(t, http.StatusOK, mockTree), + GetReposByOwnerByRepo: mockResponse(t, http.StatusOK, mockRepo), + GetReposGitTreesByOwnerByRepoByTree: mockResponse(t, http.StatusOK, mockTree), }), requestArgs: map[string]interface{}{ "owner": "owner", @@ -83,8 +82,8 @@ func Test_GetRepositoryTree(t *testing.T) { { name: "successfully get repository tree with path filter", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - mock.GetReposByOwnerByRepo.Method + " " + mock.GetReposByOwnerByRepo.Pattern: mockResponse(t, http.StatusOK, mockRepo), - mock.GetReposGitTreesByOwnerByRepoByTreeSha.Method + " " + mock.GetReposGitTreesByOwnerByRepoByTreeSha.Pattern: mockResponse(t, http.StatusOK, mockTree), + GetReposByOwnerByRepo: mockResponse(t, http.StatusOK, mockRepo), + GetReposGitTreesByOwnerByRepoByTree: mockResponse(t, http.StatusOK, mockTree), }), requestArgs: map[string]interface{}{ "owner": "owner", @@ -95,7 +94,7 @@ func Test_GetRepositoryTree(t *testing.T) { { name: "repository not found", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - mock.GetReposByOwnerByRepo.Method + " " + mock.GetReposByOwnerByRepo.Pattern: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + GetReposByOwnerByRepo: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusNotFound) _, _ = w.Write([]byte(`{"message": "Not Found"}`)) }), @@ -110,8 +109,8 @@ func Test_GetRepositoryTree(t *testing.T) { { name: "tree not found", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - mock.GetReposByOwnerByRepo.Method + " " + mock.GetReposByOwnerByRepo.Pattern: mockResponse(t, http.StatusOK, mockRepo), - mock.GetReposGitTreesByOwnerByRepoByTreeSha.Method + " " + mock.GetReposGitTreesByOwnerByRepoByTreeSha.Pattern: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + GetReposByOwnerByRepo: mockResponse(t, http.StatusOK, mockRepo), + GetReposGitTreesByOwnerByRepoByTree: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusNotFound) _, _ = w.Write([]byte(`{"message": "Not Found"}`)) }), diff --git a/pkg/github/helper_test.go b/pkg/github/helper_test.go index e1a47d386..871a13fe2 100644 --- a/pkg/github/helper_test.go +++ b/pkg/github/helper_test.go @@ -15,6 +15,20 @@ import ( "github.com/stretchr/testify/require" ) +// GitHub API endpoint patterns for testing +// These constants define the URL patterns used in HTTP mocking for tests +const ( + // Repository endpoints + GetReposByOwnerByRepo = "GET /repos/{owner}/{repo}" + + // Git endpoints + GetReposGitTreesByOwnerByRepoByTree = "GET /repos/{owner}/{repo}/git/trees/{tree}" + + // Code scanning endpoints + GetReposCodeScanningAlertsByOwnerByRepo = "GET /repos/{owner}/{repo}/code-scanning/alerts" + GetReposCodeScanningAlertsByOwnerByRepoByAlertNumber = "GET /repos/{owner}/{repo}/code-scanning/alerts/{alert_number}" +) + type expectations struct { path string queryParams map[string]string