migueleliasweb / go-github-mock

A library to aid unittesting code that uses Golang's Github SDK
MIT License
94 stars 22 forks source link

can not correctly mock GetReposContentsByOwnerByRepoByPath when path has "/" #26

Closed LeoQuote closed 2 years ago

LeoQuote commented 2 years ago
        mock.WithRequestMatch(
            mock.GetReposContentsByOwnerByRepoByPath,
            github.RepositoryContent{
                Encoding: github.String("base64"),
                Path:     github.String("path/test-file.txt"),
                Content:  github.String("fake-content"),
            },
        ),

if I call it with

client.Repositories.GetContents(
            *r.context, r.org, r.repo, github.String("path/test-file.txt"), &github.RepositoryContentGetOptions{})

it will raise exception

%!s(PANIC=Error method: runtime error: invalid memory address or nil pointer dereference)\nresponse: {{\"Response\":null,\"message\":\"mock response not found for /repos/owner/repo-name/contents/path/test-file.txt\",\"errors\":null}}"

seems to be the path could not match pattern

migueleliasweb commented 2 years ago

Oh boi, this seems to be an interesting edge case.

The {path} can actually contain forward slashes (I was unaware of that, tbh) and that seems to break the URL parsing for this endpoint. Without / in the path, the matching works.

LeoQuote commented 2 years ago

hmm, I should urlencode this {path} or this is regarded as bug ?

migueleliasweb commented 2 years ago

I think I have a plan but that might require some extra smarts during the generation of the endpoints pattern in order to account for edge cases ( I'm sure there will be more eventually )

I can probably get this working soon-ish.

I will mark this as a bug.

migueleliasweb commented 2 years ago

Hi @LeoQuote, can you test the branch from https://github.com/migueleliasweb/go-github-mock/pull/27.

Let me know if it fixes your problem.

LeoQuote commented 2 years ago

Thanks for your quick response, will try it next week!

LeoQuote commented 2 years ago

hmmm maybe releavant , I tried to mock PatchReposGitRefsByOwnerByRepoByRef also failed .

    mockedHTTPClient := mock.NewMockedHTTPClient(
        mock.WithRequestMatch(
            mock.GetReposByOwnerByRepo,
            github.Repository{
                DefaultBranch: github.String("base"),
                Name:          github.String("repo-name"),
            }),
        mock.WithRequestMatch(
            mock.PatchReposGitRefsByOwnerByRepoByRef,
            github.Reference{
                Ref: github.String("refs/heads/new-branch"),
            }),
    )
    c := github.NewClient(mockedHTTPClient)

    ctx := context.Background()
    _, _, err := c.Git.UpdateRef(ctx, "owner", "repo-name", &github.Reference{
        Ref:    github.String("refs/heads/new-branch"),
        Object: &github.GitObject{SHA: github.String("fake-sha")},
    }, false)

    assert.Nil(t, err)
     github_test.go:101: 
            Error Trace:    github_test.go:101
            Error:          Expected nil, but got: &github.ErrorResponse{Response:(*http.Response)(nil), Message:"mock response not found for /repos/owner/repo-name/git/refs/heads/new-branch", Errors:[]github.Error(nil), Block:(*github.ErrorBlock)(nil), DocumentationURL:""}
            Test:           TestPush
migueleliasweb commented 2 years ago

@LeoQuote yeah, it's similar problem to the GetContents call. I will also fix that and create a new release.

migueleliasweb commented 2 years ago

@LeoQuote Can you test again your cases with https://github.com/migueleliasweb/go-github-mock/pull/30? It should cater for both cases.

migueleliasweb commented 2 years ago

Merged https://github.com/migueleliasweb/go-github-mock/pull/30