refactor: mock GQL client instead of getFileSHA function to follow conventions

Author: LuluBeatson

pkg/github/repositories.go

@@ -19,10 +19,6 @@ import (
 	"github.com/shurcooL/githubv4"
 )
 
-// getFileSHAFunc is a package-level variable that holds the getFileSHA function
-// This allows tests to mock the behavior
-var getFileSHAFunc = getFileSHA
-
 func GetCommit(getClient GetClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) {
 	return mcp.NewTool("get_commit",
 			mcp.WithDescription(t("TOOL_GET_COMMITS_DESCRIPTION", "Get details for a commit from a GitHub repository")),
@@ -516,7 +512,7 @@ func GetFileContents(getClient GetClientFn, getRawClient raw.GetRawClientFn, get
 				if err != nil {
 					return mcp.NewToolResultError("failed to get GitHub GraphQL client"), nil
 				}
-				fileSHA, err := getFileSHAFunc(ctx, gqlClient, owner, repo, path, rawOpts)
+				fileSHA, err := getFileSHA(ctx, gqlClient, owner, repo, path, rawOpts)
 				if err != nil {
 					return mcp.NewToolResultError(fmt.Sprintf("failed to get file SHA: %s", err)), nil
 				}

pkg/github/repositories_test.go

@@ -9,6 +9,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/github/github-mcp-server/internal/githubv4mock"
 	"github.com/github/github-mcp-server/internal/toolsnaps"
 	"github.com/github/github-mcp-server/pkg/raw"
 	"github.com/github/github-mcp-server/pkg/translations"
@@ -20,11 +21,39 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-// mockGetFileSHA is a test helper that mocks the getFileSHA function so that we don't need to mock the GraphQL client in Test_GetFileContents.
-func mockGetFileSHA(expectedSHA string) func(context.Context, *githubv4.Client, string, string, string, *raw.ContentOpts) (string, error) {
-	return func(_ context.Context, _ *githubv4.Client, _, _, _ string, _ *raw.ContentOpts) (string, error) {
-		return expectedSHA, nil
+func mockGQLClientFileSHA(t *testing.T, owner, repo, expression, expectedSHA string) *githubv4.Client {
+	// Create the query structure that matches getFileSHA function
+	query := struct {
+		Repository struct {
+			Object struct {
+				Blob struct {
+					OID string
+				} `graphql:"... on Blob"`
+			} `graphql:"object(expression: $expression)"`
+		} `graphql:"repository(owner: $owner, name: $repo)"`
+	}{}
+
+	// Match any variables, we don't care about the exact values in this test
+	variables := map[string]interface{}{
+		"owner":      githubv4.String(owner),
+		"repo":       githubv4.String(repo),
+		"expression": githubv4.String(expression),
 	}
+
+	// Create the mock response with the expected SHA
+	mockResponse := githubv4mock.DataResponse(map[string]any{
+		"repository": map[string]any{
+			"object": map[string]any{
+				"oid": expectedSHA,
+			},
+		},
+	})
+
+	// Create the matcher and mock client
+	matcher := githubv4mock.NewQueryMatcher(query, variables, mockResponse)
+	httpClient := githubv4mock.NewMockedHTTPClient(matcher)
+
+	return githubv4.NewClient(httpClient)
 }
 
 func Test_GetFileContents(t *testing.T) {
@@ -68,7 +97,7 @@ func Test_GetFileContents(t *testing.T) {
 	tests := []struct {
 		name           string
 		mockedClient   *http.Client
-		mockFileSHA    string
+		mockGQLClient  *githubv4.Client
 		requestArgs    map[string]interface{}
 		expectError    bool
 		expectedResult interface{}
@@ -107,7 +136,7 @@ func Test_GetFileContents(t *testing.T) {
 					}),
 				),
 			),
-			mockFileSHA: "abc123",
+			mockGQLClient: mockGQLClientFileSHA(t, "owner", "repo", "refs/heads/main:README.md", "abc123"),
 			requestArgs: map[string]interface{}{
 				"owner": "owner",
 				"repo":  "repo",
@@ -153,7 +182,7 @@ func Test_GetFileContents(t *testing.T) {
 					}),
 				),
 			),
-			mockFileSHA: "def456",
+			mockGQLClient: mockGQLClientFileSHA(t, "owner", "repo", "refs/heads/main:test.png", "def456"),
 			requestArgs: map[string]interface{}{
 				"owner": "owner",
 				"repo":  "repo",
@@ -199,7 +228,7 @@ func Test_GetFileContents(t *testing.T) {
 					),
 				),
 			),
-			mockFileSHA: "", // Directory content doesn't need SHA
+			mockGQLClient: nil,
 			requestArgs: map[string]interface{}{
 				"owner": "owner",
 				"repo":  "repo",
@@ -233,7 +262,7 @@ func Test_GetFileContents(t *testing.T) {
 					}),
 				),
 			),
-			mockFileSHA: "", // Error case doesn't need SHA
+			mockGQLClient: mockGQLClientFileSHA(t, "owner", "repo", "refs/heads/main:nonexistent.md", ""),
 			requestArgs: map[string]interface{}{
 				"owner": "owner",
 				"repo":  "repo",
@@ -247,20 +276,10 @@ func Test_GetFileContents(t *testing.T) {
 
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
-			// Mock the getFileSHA function if mockFileSHA is provided
-			if tc.mockFileSHA != "" {
-				originalGetFileSHA := getFileSHAFunc
-				getFileSHAFunc = mockGetFileSHA(tc.mockFileSHA)
-				defer func() {
-					getFileSHAFunc = originalGetFileSHA
-				}()
-			}
-
 			// Setup client with mock
 			client := github.NewClient(tc.mockedClient)
 			mockRawClient := raw.NewClient(client, &url.URL{Scheme: "https", Host: "raw.example.com", Path: "/"})
-			mockGQLClient := githubv4.NewClient(tc.mockedClient)
-			_, handler := GetFileContents(stubGetClientFn(client), stubGetRawClientFn(mockRawClient), stubGetGQLClientFn(mockGQLClient), translations.NullTranslationHelper)
+			_, handler := GetFileContents(stubGetClientFn(client), stubGetRawClientFn(mockRawClient), stubGetGQLClientFn(tc.mockGQLClient), translations.NullTranslationHelper)
 
 			// Create call request
 			request := createMCPRequest(tc.requestArgs)