repository resource tests #69

mntlty · 2025-04-02T05:23:01Z

Refactors repository_resource to make it easier to test, then adds some tests mostly copied from

github-mcp-server/pkg/github/repositories_test.go

Line 18 in d8b0056

func Test_GetFileContents(t *testing.T) {

which seems quite similar.

Copilot

Pull Request Overview

This pull request refactors the repository resource code to simplify testing and adds corresponding tests. The changes include updating the server to register resource templates by splitting the single function into multiple functions, adding new tests for repository content handling, and updating the resource functions in the repository resource file.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
pkg/github/server.go	Updated resource template registration to use individual functions
pkg/github/repository_resource_test.go	Added tests for file and directory content fetching
pkg/github/repository_resource.go	Refactored resource functions to return a single tuple (template and handler) for various repository content types

Comments suppressed due to low confidence (1)

pkg/github/repository_resource.go:25

The comment for getRepositoryBranchContent (and similarly for the other functions) still refers to 'getRepositoryContent'. Please update the comment to accurately reflect the functionality, for example, 'getRepositoryBranchContent defines the resource template and handler for branch-specific repository content.'

// getRepositoryContent defines the resource template and handler for the Repository Content API.

Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more

mntlty · 2025-04-02T05:24:32Z

pkg/github/repository_resource.go

@@ -14,109 +14,137 @@ import (
 )

 // getRepositoryContent defines the resource template and handler for the Repository Content API.
-func getRepositoryContent(client *github.Client, t translations.TranslationHelperFunc) (mainTemplate mcp.ResourceTemplate, reftemplate mcp.ResourceTemplate, shaTemplate mcp.ResourceTemplate, tagTemplate mcp.ResourceTemplate, prTemplate mcp.ResourceTemplate, handler server.ResourceTemplateHandlerFunc) {


this format of returning multiple values made it quite hard to evaluate and test, instead I broke them up into individual functions

pkg/github/repository_resource.go

mntlty · 2025-04-02T05:29:05Z

pkg/github/repository_resource.go

-			if ok {
-				opts.Ref = "refs/heads/" + branch[0]
-			}
+		sha, ok := request.Params.Arguments["sha"].([]string)


left this as it was previously, where one function was shared by all ResourceTemplates. I think it would be better to continue the refactor by creating individual functions that only parse arguments that are expected, for example the sha argument would only be parsed when getting a commit

Yeah, I just wanted to quickly scale out what I had written to multiple endpoints - but in truth, the shared logic could be shared, with separate opts generation and that is likely a better direction.

tl;dr I think you're right. I just spent enough time trying to decide how to wrap it as a resource, and make it work, I didn't spend much time on how it should be written.

mntlty · 2025-04-02T05:30:13Z

pkg/github/repository_resource.go

+			return nil, err
+		}
+
+		if directoryContent != nil {


not sure if directoryContent and fileContent are mutually exclusive, that is the previous behavior which I haven't changed (I only removed the unnecessary else clause)

We are only returning the names of things with directoryContent, and not the actual file data. When we are returning the file data, we actually serialise the payload.

That's the best I could come up with, the alternative would be to not have a way to inspect directory content, and just return the fact it's a directory.

It's not a perfect fit, but no way are we returning all the blobs in a folder 😅

mntlty · 2025-04-02T05:31:00Z

pkg/github/repository_resource_test.go

+	"github.com/stretchr/testify/require"
+)
+
+func Test_RepoContentsResourceHandler(t *testing.T) {


largely copied from

github-mcp-server/pkg/github/repositories_test.go

Line 18 in d8b0056

func Test_GetFileContents(t *testing.T) {

mntlty · 2025-04-02T05:31:25Z

pkg/github/repository_resource_test.go

+	expectedDirContent := []mcp.TextResourceContents{
+		{
+			URI:      "https://github.com/owner/repo/blob/main/README.md",
+			MIMEType: "",


feels like this shouldn't be empty, but that's what the current code returns 🤷

Hmmm, at very least I suppose we could special case markdown. I used the stdlib mime type lib, but it's perhaps not for for purpose.

mntlty · 2025-04-02T06:14:05Z

pkg/github/repository_resource.go

 		}
+
+		return nil, nil


I added a test for this behavior, but it seems wrong - I feel like we should be returning an error here

You are correct, and we shouldn't actually be able to hit it in practice, because the real server would falter here:

https://github.com/github/github-mcp-server/pull/69/files#diff-8c2319afb75694e93b084f504d1c1fc8de262f4301c1888c05ab067d5bf0a21bR91

So this should be one of those if you are here, there be dragons kind of something went wrong errors.

mntlty · 2025-04-02T06:14:44Z

pkg/github/repository_resource_test.go

+}
+
+func Test_getRepositoryResourceContent(t *testing.T) {
+	tmpl, _ := getRepositoryResourceContent(nil, translations.NullTranslationHelper)


I could not find a sensible assertion for the handler func, it's execution is tested above

mntlty · 2025-04-02T06:18:56Z

pkg/github/repository_resource_test.go

+	expectedDirContent := []mcp.TextResourceContents{
+		{
+			URI:      "https://github.com/owner/repo/blob/main/README.md",
+			MIMEType: "text/markdown; charset=utf-8",


frustratingly, this value exists on the ubuntu runner https://github.com/github/github-mcp-server/actions/runs/14212790977/job/39822938856?pr=69 and does not exist on the mac runner https://github.com/github/github-mcp-server/actions/runs/14212837651/job/39823067424?pr=69 , so one of the other fails. not sure how best to handle this

maybe another file type would be simpler

or maybe not, looks like "text/directory" is also an issue between the two runner types

mntlty · 2025-04-02T06:41:39Z

pkg/github/server.go

-	s.AddResourceTemplate(tagTemplate, handler)
-	s.AddResourceTemplate(shaTemplate, handler)
-	s.AddResourceTemplate(prTemplate, handler)
+	s.AddResourceTemplate(getRepositoryResourceContent(client, t))


an alternative approach here would be to return a list and apply each one, that way changes to the resource type don't need to be remembered to be applied here. on the flip side, we would also lose some control and flexibility 🤔

Definitely something to consider

SamMorrowDrums

This is really awesome, the mime type thing is something I will have a look into. There are other options, and I'd like to see if we can get it from GitHub too, rather than have to make assertions.

All the calls you made in this PR I support though, they all improve the codebase anyway! Never mind the actual testing ❤️

SamMorrowDrums · 2025-04-02T11:59:47Z

pkg/github/repository_resource.go

+			return nil, err
+		}
+
+		if directoryContent != nil {


We are only returning the names of things with directoryContent, and not the actual file data. When we are returning the file data, we actually serialise the payload.

That's the best I could come up with, the alternative would be to not have a way to inspect directory content, and just return the fact it's a directory.

It's not a perfect fit, but no way are we returning all the blobs in a folder 😅

SamMorrowDrums · 2025-04-02T12:01:06Z

pkg/github/repository_resource.go

-			if ok {
-				opts.Ref = "refs/heads/" + branch[0]
-			}
+		sha, ok := request.Params.Arguments["sha"].([]string)


Yeah, I just wanted to quickly scale out what I had written to multiple endpoints - but in truth, the shared logic could be shared, with separate opts generation and that is likely a better direction.

tl;dr I think you're right. I just spent enough time trying to decide how to wrap it as a resource, and make it work, I didn't spend much time on how it should be written.

SamMorrowDrums · 2025-04-02T12:03:41Z

pkg/github/repository_resource.go

 		}
+
+		return nil, nil


You are correct, and we shouldn't actually be able to hit it in practice, because the real server would falter here:

https://github.com/github/github-mcp-server/pull/69/files#diff-8c2319afb75694e93b084f504d1c1fc8de262f4301c1888c05ab067d5bf0a21bR91

So this should be one of those if you are here, there be dragons kind of something went wrong errors.

SamMorrowDrums · 2025-04-02T12:04:57Z

pkg/github/repository_resource_test.go

+	expectedDirContent := []mcp.TextResourceContents{
+		{
+			URI:      "https://github.com/owner/repo/blob/main/README.md",
+			MIMEType: "",


Hmmm, at very least I suppose we could special case markdown. I used the stdlib mime type lib, but it's perhaps not for for purpose.

SamMorrowDrums · 2025-04-02T12:18:46Z

pkg/github/server.go

-	s.AddResourceTemplate(tagTemplate, handler)
-	s.AddResourceTemplate(shaTemplate, handler)
-	s.AddResourceTemplate(prTemplate, handler)
+	s.AddResourceTemplate(getRepositoryResourceContent(client, t))


Definitely something to consider

* use raw repo URIs for resources * fetch repository content from raw urls * ensure no error in test write * Update pkg/github/repository_resource.go Co-authored-by: Copilot <[email protected]> * use appropriate file name for text file test --------- Co-authored-by: Copilot <[email protected]>

* refactor to make testing easier * not needed in handler func * small cleanup * create repository_resource_test * remove chatty comments * comment cleanup, function rename and some more tests * fix test for ubuntu runner * remove it for now * make required args explicit instead of panic * more tests and cleanup * chore: use raw repo resources (github#70) * use raw repo URIs for resources * fetch repository content from raw urls * ensure no error in test write * Update pkg/github/repository_resource.go Co-authored-by: Copilot <[email protected]> * use appropriate file name for text file test --------- Co-authored-by: Copilot <[email protected]> --------- Co-authored-by: Sam Morrow <[email protected]> Co-authored-by: Copilot <[email protected]>

mntlty added 4 commits April 1, 2025 19:18

refactor to make testing easier

383c2d2

not needed in handler func

1cb52f9

small cleanup

2aa3002

create repository_resource_test

d8b0056

Copilot AI review requested due to automatic review settings April 2, 2025 05:23

Copilot AI reviewed Apr 2, 2025

View reviewed changes

mntlty commented Apr 2, 2025

View reviewed changes

pkg/github/repository_resource.go Outdated Show resolved Hide resolved

mntlty commented Apr 2, 2025

View reviewed changes

mntlty added 2 commits April 1, 2025 22:33

remove chatty comments

2fdda7c

comment cleanup, function rename and some more tests

9680b24

mntlty commented Apr 2, 2025

View reviewed changes

fix test for ubuntu runner

b4e6772

mntlty commented Apr 2, 2025

View reviewed changes

remove it for now

db7a180

mntlty commented Apr 2, 2025

View reviewed changes

SamMorrowDrums reviewed Apr 2, 2025

View reviewed changes

mntlty and others added 3 commits April 2, 2025 07:20

make required args explicit instead of panic

02ebdc7

more tests and cleanup

ad58220

SamMorrowDrums merged commit b2503f3 into main Apr 2, 2025
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

repository resource tests #69

repository resource tests #69

mntlty commented Apr 2, 2025

Copilot AI left a comment

mntlty Apr 2, 2025

mntlty Apr 2, 2025

SamMorrowDrums Apr 2, 2025

mntlty Apr 2, 2025

SamMorrowDrums Apr 2, 2025

mntlty Apr 2, 2025

mntlty Apr 2, 2025

SamMorrowDrums Apr 2, 2025

mntlty Apr 2, 2025

SamMorrowDrums Apr 2, 2025

mntlty Apr 2, 2025

mntlty Apr 2, 2025

mntlty Apr 2, 2025

mntlty Apr 2, 2025

mntlty Apr 2, 2025

SamMorrowDrums Apr 2, 2025

SamMorrowDrums left a comment

SamMorrowDrums Apr 2, 2025

SamMorrowDrums Apr 2, 2025

SamMorrowDrums Apr 2, 2025

SamMorrowDrums Apr 2, 2025

SamMorrowDrums Apr 2, 2025

repository resource tests #69

repository resource tests #69

Conversation

mntlty commented Apr 2, 2025

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SamMorrowDrums left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment