Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mocking go-github #113

Closed
tbruyelle opened this issue Jun 4, 2014 · 14 comments
Closed

Mocking go-github #113

tbruyelle opened this issue Jun 4, 2014 · 14 comments

Comments

@tbruyelle
Copy link
Contributor

@tbruyelle tbruyelle commented Jun 4, 2014

I would like to write tests for an app that uses a lot go-github.

For instance I would like to mock some methods in the client.*Services structs. Is this possible with the current usage of structs ? Wouldn't it be preferable that the client struct holds some interfaces rather than some structs ? With interfaces, mocking is very easy with struct embedding.

@willnorris

This comment has been minimized.

Copy link
Member

@willnorris willnorris commented Jun 4, 2014

yeah, that would have made a lot of sense... unfortunately, I wasn't aware of gomock when I started. 😦 I'm pretty sure that would be a backward-compatible change though, it would just be a decent amount of work.

@tbruyelle

This comment has been minimized.

Copy link
Contributor Author

@tbruyelle tbruyelle commented Jun 5, 2014

I agree about backward compatibility. I'll make the change in my fork and I would show you when I'm satisfied with the result.

@willnorris

This comment has been minimized.

Copy link
Member

@willnorris willnorris commented Jun 11, 2014

Not sure if you've started work on this, but I remember the other thing that bugged me about making these interfaces... the godocs for interfaces with lots of methods are pretty terrible, since they all get squished into a single pre block. That's not a great reason not to use them, but it will certainly have a non-trivial impact on the usability of the documentation.

@tbruyelle

This comment has been minimized.

Copy link
Contributor Author

@tbruyelle tbruyelle commented Jun 12, 2014

No I have'nt started yet (some main job to do :)).
I had a look to the godocs for interfaces, and yes it's ugly... I agree it would damage the actual documentation. This is a problem.

There is an old bug [1] and a CL [2], maybe godoc for interface will be improved in go 1.3. Until that, I don't know what to do.

[1] https://code.google.com/p/go/issues/detail?id=5860
[2] https://codereview.appspot.com/12723043

@willnorris

This comment has been minimized.

Copy link
Member

@willnorris willnorris commented Jun 12, 2014

So here's one approach we've used here at Google (not for this library, but same situation)... Define the interface in your own application that mirrors the concrete type, then build your application to rely on that interface. In production, you just provide the concrete type from go-github and it satisfies the interface; in test you use gomock or whatever to provide alternate implementations.

This is a little uglier with go-github because the implementations you'd want to mock are the individual Service types, not the client itself. The good news is that you only need to define interfaces for the services and methods that your application cares about.

I'm not sure if it's a good long-term solution for you (it certainly works better with simpler types), but might be worth trying on one or two methods as an experiment.

@tbruyelle

This comment has been minimized.

Copy link
Contributor Author

@tbruyelle tbruyelle commented Jun 13, 2014

Wow thanks for the information !
So if I well understood, I should create an interface for the go-github methods I use in my app, and use it as a replacement of the github.client struct. The interface implementation in production would be a struct that contains the github.client and chains the method invocations to it.

Something like that

type GoGithub interface {
     ListRelease(...)
    // other methods used in my app
}

and the implementation

type GoGithubCli struct {
    client *github.client
}

func (g GoGithubCli) ListRelease(...) {
   g.client.ListRelease(...)
}

For the tests I just have to provide an other implementation. That's fine !

@willnorris

This comment has been minimized.

Copy link
Member

@willnorris willnorris commented Jun 13, 2014

that would certainly work, and may end up being cleaner than what I was originally proposing, I'm not sure. I was thinking about defining interfaces for the individual services. So in your app...

type githubRepoService interface {
  ListReleases(...)
  // other repository methods used in your app
}

// other services used in your app

You don't need to provide a concrete implementation of these interfaces because go-github already does. You would however need to wrap them in a client that uses the interfaces...

type GitHubClient struct {
  Repositories  githubRepoService
  // optionally store and export the underlying *github.Client 
  // if you want easy access to client.Rate or other fields
}

func NewClient(httpClient *http.Client) GitHubClient {
  client := github.NewClient(httpClient)
  // optionally set client.BaseURL, client.UserAgent, etc

  return GitHubClient{
    Repositories: client.Repositories
    // any other services your app uses
  }
}

Adding more methods for your application only requires adding them to the relevant service interface, no need for an implementation, since again, go-github itself is the implementation.

The calling style should be exactly the same as using go-github directly... client.Repositories.ListReleases(...). The only change would be calling this NewClient method, rather than the one in go-github.

@tbruyelle

This comment has been minimized.

Copy link
Contributor Author

@tbruyelle tbruyelle commented Jun 17, 2014

Thanks for the clarification, I see. Your method is less verbose, I don't have to code a production implementation of the interface.

@willnorris

This comment has been minimized.

Copy link
Member

@willnorris willnorris commented Apr 28, 2015

I'm going to go ahead and close this out, as there's no work to be done. The above mocking strategy described in my last comment (where the application itself defines the interfaces which go-github implements) is used heavily inside Google, both for go-github and lots of other libraries, and it's worked really well for us.

@willnorris willnorris closed this Apr 28, 2015
@noonien

This comment has been minimized.

Copy link

@noonien noonien commented Sep 23, 2015

Could you please provide an example of a projects or code that mocks go-github, or a similar library, in the manner that you suggest?

I've grepped trough all the projects that import go-github and not a single one has managed to mock this library. The best effort I've seen just used a httptest and checked that the appropriate paths were called.

@willnorris

This comment has been minimized.

Copy link
Member

@willnorris willnorris commented Sep 23, 2015

Unfortunately, all of the examples I have are internal Google projects which aren't open source. But they use exactly the method described above together with gomock.

@Skarlso

This comment has been minimized.

Copy link

@Skarlso Skarlso commented Sep 13, 2018

For anyone potentially looking for an example of how to use the above scheme in a project. Here is an example of this:

In code: https://github.com/gaia-pipeline/gaia/blob/master/pipeline/git.go#L168-L194
In test: https://github.com/gaia-pipeline/gaia/blob/master/pipeline/git_test.go#L252

@artem-sidorenko

This comment has been minimized.

@Skarlso

This comment has been minimized.

Copy link

@Skarlso Skarlso commented Jan 15, 2019

@artem-sidorenko Oh excellent. Thanks for fixing that. :)

artem-sidorenko added a commit to artem-sidorenko/chagen that referenced this issue Jan 15, 2019
inspired by google/go-github#113

We have now better testing setup: we mock the github API client in our tests, so:
- better test coverage
- less layers (no API wrapper anymore)
- test setup is more go-style

Signed-off-by: Artem Sidorenko <artem@posteo.de>
mccurdyc added a commit to mccurdyc/neighbor that referenced this issue Jan 6, 2020
* define a minimal GitHub client interface.
* this will enable testing of funcitons/methods that make calls to GitHub via mocks of this interface.
* google/go-github#113
mccurdyc added a commit to mccurdyc/neighbor that referenced this issue Jan 10, 2020
* sdk/search - WIP adding backend stub

Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>

* builtin/project/github - GitHub project backend

* first stub of GitHub as a project backend

* sdk/project - WIP; define project backend

* stubbed out a project backend

* sdk/search - update search backend

* define generic SearchMethods
* simplify search Backend interface
  * removed NextPage()
  * removed ProcessResults()
* updated BackendConfig
  * removed NumberOfDesiredResults as this will be part of the Search method call
  * added AuthMethod and SearchMethod

* builtin/search/github - WIP; GitHub search backend

* stub out a search backend for GitHub
* partially-implemented Factory function
* stubbed functions for searching via the supported methods on GitHub

* builtin/search/github - handle auth in Factory fn

* similar to builtin/retrieval/git Factory for handling auth

* cmd/neighbor - use GitHub search Factory

* pkg/github/search: migrating code to GitHub search

* builtin/project/github: remove Retrieval fn as req

* don't make retrieval function required on project creation

* builtin/search/github: first rough pass

* builtin/search/github: calculate page size helpers

* builtin/search/github: WIP rough first pass

* cmd/neighbor: stub for GitHub search factory

* pkg/github: move repository search

* moved to builtin/search/github

* sdk/search: minor cleanup

* use iota instead of string for search method
* reduce duplication search.SearchMethod to search.Method

* sdk/search: add http.Client to search backend

* add an http.Client to be used to connect to search services if necessary.

* builtin/search/github: use http.Client

* use the http.Client from the BackendConfig

* builtin/search/github: return *github.Response

* search helper functions now return a *github.Response in order to do centralized pagination

* builtin/search/github: fewer results sentinel err

* explicitly respond with an error when fewer results than desired were obtained

* cmd/neighbor: use search.Method

* translate search_type cli flag to search.Method

* builtin/search/github: tests for pageSize helper

* pkg/github: move/delete old GitHub search

* sdk/search: fix AuthMethod comment typo

* fix typo in AuthMethod's comment where AuthRequired should have been AuthMethod

* builtin/search/github: pagination and opts

* move github.SearchOptions up a level to control pagination during search
* stubbed pagination code (untested)

* builtin/search/github: GitHub SearchOptions param

* add *github.SearchOptions as a function param to allow control of pagination at a higher level

* builtin/search/github: add tests for Factory func

* builtin/searchgithub: update error messages

* update error messages for Version and Meta search to use correct naming i.e., Version and Meta instead of VersionSearch and MetaSearch, respectively.

* builtin/search/github: unsupported search method

* handle an unsupported search method by returning an error instead of proceeding.

* builtin/search/github: define client interface

* define a minimal GitHub client interface.
* this will enable testing of funcitons/methods that make calls to GitHub via mocks of this interface.
* google/go-github#113

* builtin/search/github: make testable

* make Search method testable by
  * making `maxPageSize` a struct field rather than a global
  * defining and using the minimal GitHub client

* builtin/search/github: use Client

* use the internal Client definition instead of `*github.Client` directly

* builting/search/github: Factory tests

* builtin/search/github: mockClient and methods

* builtin/search/github: fix repo owner name

* fix repo owner name in commit search
* don't use stringified version of a *github.User, instead use the username

* builtin/search/github: WIP tests for Search

* pkg/github: delete clone

* builtin/search/github: handle nil repo response

* handle the case which could cause a panic if the repository response were nil

* builtin/search/github: WIP - passing Search test

* builtin/search/github: tests for Search

* a first draft of working tests for Search
* these should probably be revisted and cleaned up

* builtin/project/github: GoDoc function comments

* builtin/search/github: GoDoc comments

* add GoDoc comments to the GitHub search backend

* builtin/project/github: fixed SourceLocation() bug

* SourceLocation() recursively called iteself infinitely many times (this was introduced by editor autocomplete).
* add minimal tests for the project package
* add tests that would have caught this behavior

* builtin/search/github: idiomatic naming

* `gitHubClient` -> `c`

* cmd/neighbor: minor tweaks

* builtin/search/github: Code search

* implement code search for obtaining repositories

* builtin/search/github: Code search

* implement code search for obtaining repositories
* add tests for search method `search.Code`

* builtin/search/github: update Client interface

* add `Code` method to `SearchService` interface for searching code on GitHub

* cmd/neighbor: update const neighbor directory

* also, make sure that the .gitignore includes the path

* docs: update README with code search example

* update example usage section
  * add code search example
* update summary paragraph
* update motivation for used neighbor

* sdk/project: update project.Backend interface

* make it more functional i.e., don't modify in place, instead return a new instance

* builtin/search/github: code search

* add and use function for obtaining cloneURL
  * defaults to `repo.GetCloneURL()`, but builds it as a fallback. This is because code search doesn't populate the `CloneURL` field for repositories
* deduplicate code search repository results
  * code search will often return numerous instances of text match in a single repository
  this means that if we naively iterate over code search results, we will include
  numerous instances of the same repository.

* builtin/project/github: use pointers

* builtin/search/github: URLRetriever interface

* tests for `getCloneURL`

* builtin/search/github: test for deduplication

* test the internal `contains` function for deduplication

* builtin/search/github: tests search deduplication

This change adds a test case and makes the necessary changes to the helper function
to test the behavior when duplicate results occur.

* builtin/search/github: tests search deduplication

This change adds a test case and makes the necessary changes to the helper function
to test the behavior when duplicate results occur.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.