- Notifications
You must be signed in to change notification settings - Fork897
repository resource tests#69
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
| @@ -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) { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this format of returning multiple values made it quite hard to evaluate and test, instead I broke them up into individual functions
Uh oh!
There was an error while loading.Please reload this page.
| if ok { | ||
| opts.Ref = "refs/heads/" + branch[0] | ||
| } | ||
| sha, ok := request.Params.Arguments["sha"].([]string) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 thesha argument would only be parsed when getting a commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 separateopts 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.
| return nil, err | ||
| } | ||
| if directoryContent != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
not sure if directoryContent and fileContent are mutually exclusive, that is the previous behavior which I haven't changed (I only removed the unnecessaryelse clause)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We are only returning the names of things withdirectoryContent, 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 😅
| "github.com/stretchr/testify/require" | ||
| ) | ||
| func Test_RepoContentsResourceHandler(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
largely copied from
| funcTest_GetFileContents(t*testing.T) { |
| expectedDirContent := []mcp.TextResourceContents{ | ||
| { | ||
| URI: "https://github.com/owner/repo/blob/main/README.md", | ||
| MIMEType: "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
feels like this shouldn't be empty, but that's what the current code returns 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
pkg/github/repository_resource.go Outdated
| } | ||
| return nil, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I added a test for this behavior, but it seems wrong - I feel like we should be returning an error here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You are correct, and we shouldn't actually be able to hit it in practice, because the real server would falter here:
So this should be one of thoseif you are here, there be dragons kind ofsomething went wrong errors.
| } | ||
| func Test_getRepositoryResourceContent(t *testing.T) { | ||
| tmpl, _ := getRepositoryResourceContent(nil, translations.NullTranslationHelper) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I could not find a sensible assertion for the handler func, it's execution is tested above
| expectedDirContent := []mcp.TextResourceContents{ | ||
| { | ||
| URI: "https://github.com/owner/repo/blob/main/README.md", | ||
| MIMEType: "text/markdown; charset=utf-8", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
frustratingly, this value exists on the ubuntu runnerhttps://github.com/github/github-mcp-server/actions/runs/14212790977/job/39822938856?pr=69 and does not exist on the mac runnerhttps://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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
maybe another file type would be simpler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
or maybe not, looks like "text/directory" is also an issue between the two runner types
| s.AddResourceTemplate(tagTemplate, handler) | ||
| s.AddResourceTemplate(shaTemplate, handler) | ||
| s.AddResourceTemplate(prTemplate, handler) | ||
| s.AddResourceTemplate(getRepositoryResourceContent(client, t)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Definitely something to consider
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 ❤️
| return nil, err | ||
| } | ||
| if directoryContent != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We are only returning the names of things withdirectoryContent, 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 😅
| if ok { | ||
| opts.Ref = "refs/heads/" + branch[0] | ||
| } | ||
| sha, ok := request.Params.Arguments["sha"].([]string) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 separateopts 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.
pkg/github/repository_resource.go Outdated
| } | ||
| return nil, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You are correct, and we shouldn't actually be able to hit it in practice, because the real server would falter here:
So this should be one of thoseif you are here, there be dragons kind ofsomething went wrong errors.
| expectedDirContent := []mcp.TextResourceContents{ | ||
| { | ||
| URI: "https://github.com/owner/repo/blob/main/README.md", | ||
| MIMEType: "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
| s.AddResourceTemplate(tagTemplate, handler) | ||
| s.AddResourceTemplate(shaTemplate, handler) | ||
| s.AddResourceTemplate(prTemplate, handler) | ||
| s.AddResourceTemplate(getRepositoryResourceContent(client, t)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.goCo-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>* use appropriate file name for text file test---------Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
b2503f3 intomainUh oh!
There was an error while loading.Please reload this page.
* 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.goCo-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>* use appropriate file name for text file test---------Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>---------Co-authored-by: Sam Morrow <info@sam-morrow.com>Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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 ind8b0056