Auto-generate interfaces for all services #1832
Conversation
google-cla
bot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #1832 +/- ##
==========================================
- Coverage 97.64% 97.50% -0.15%
==========================================
Files 105 106 +1
Lines 6762 6776 +14
==========================================
+ Hits 6603 6607 +4
- Misses 86 96 +10
Partials 73 73
Continue to review full report at Codecov.
|
|
@willnorris - assuming you are OK with this PR, would you prefer for me to include it into the imminent |
|
I think putting this into the main github package is a really bad idea. I'm still not wild about adding it at all, but understand the arguments being made in #1800. Would putting these into a separate test related package still serve the same purpose for clients to more easily test? |
|
OK, thank you, @willnorris - then I'll move ahead with |
That's a great idea, but I don't know the answer. Let's see what the responses are on #1800. |
In order to allow users of this lib to properly use the interfaces to improve the current testing/mocking situation we might need to also replace all types in Thoughts? |
|
Ps: @gmlewis beat me to it! Haha! Nice implementation! It's just a bit sad we can't/not supposed to use external dependencies here =(. |
|
It would also we great if we could add some testing examples in |
I'm not sure I understand this statement.
Great idea, if we choose to pursue this. I'll add a simple example that demonstrates the concept of embedding the interface. |
|
@migueleliasweb - I've gone ahead and added a simple example... but now that I've done that, I'm still not convinced that this is of any benefit whatsoever, as it is just as simple to mock only the one method that I need. What am I missing?!? What is so great about embedding an interface into a struct? |
Oh I see you example, yeah, that's not quite what I tried to do. I will create a gist with the test case I had to run. I'm stuck in a meeting rn, so this might take a lil bit. 😞 |
…face-stubbed Create getter and setter for MarketplaceService.Stubbed
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
Ah, right... @migueleliasweb - have you signed the CLAs? I believe you have, but can you please officially respond that you are OK to include your changes into this PR? |
I reckon I had to sign when I created the other PR for this repo. Do I need to sign again for this one? This is the other PR i'm talking about, it says |
It's all good from my end 👍 . |
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
@googlebot I consent. |
google-cla
bot
added
cla: yes
|
OK, I believe we now have a complete implementation with all issues resolved. What would be really nice now is if a major user of this repo (Google? Twitter? Terraform? Disney? someone else?) could actually try out these changes and see if they are OK with the approach. |
|
(Twitter is not a user of this package, so can't help there. At some point, we may move to Terraform, in which case we'd be using it indirectly, but not currently.) |
|
I started an (internal) conversation about this, and have gotten some interesting feedback so far:
That last one is particularly interesting. I don't think we tried that exact approach. I'll let you know if anything else comes along. Regarding testing it, I'm swamped this week. I'll try and poke at it time permitting. |
|
Those are some interesting points, @rspier and thank you for sharing the conversation you've had in here.
Could you expand this idea a bit more? Would this be related to embedding interfaces to other interfaces?
As Glen mentioned before, it's not that the current package implemention was untestable but I guess the interfaces would just make it a bit more nice to write tests. With the interfaces a client of this package wouldn't have to deal with httptest.
This is something we could think about doing. This is basically the way AWS Golang SDK does it as far as I can tell. They provide a "standard" implemention that is (at first glance) unaware of the interfaces that are defined in a separate package. But in the end you still need to use the interfaces everytime you need to write any sort of mocking/testing in your code. Eg: // from README.md
client := github.NewClient(nil)
orgs, _, err := client.Organizations.List(context.Background(), "willnorris", nil)
// If we don't return `client.Organizations` as an interface (OrganizationsServiceInterface), I'm not sure if we would be able to
// reimplement it as a mock/fake implementation for unittests.Ps: I'm just giving ideas to the best of my abilities. Sorry if some of this make no sense 🙃. |
Here's what was said:
This is why local interfaces for only the used surface work well.
I agree httptest feels heavy. One approach might be to write some wrappers around httptest to simplify its use in our case.
What we sometimes do is have our own local client definition: type OrganizationsService interface {
Get(ctx context.Context, org string) (*github.Organization, *github.Response, error)
}
type Client struct {
Issues IssuesService
Organizations OrganizationsService
PullRequests PullRequestsService
Repositories RepositoriesService
}
// convertClient converts a (go-)github.Client to a Client struct.
func convertClient(c *github.Client) *Client {
return &Client{
Issues: c.Issues,
Organizations: c.Organizations,
PullRequests: c.PullRequests,
Repositories: c.Repositories,
}
}Then in tests we just implement/stub/mock/fake our (local) client's needs.
I think it mostly makes sense. |
|
Adding another 2 cents. At Google we typically right code along the lines of this: type myClient struct{
orgService ghOrgInterface
}
g, err := github.NewClient(...)
c := myClient{
orgService: g.Orgnaizations,
}
type ghOrgInterface interface{
... Methods we use here
}This allows us to stub out portions of the github client. We have a githubstubs package which contain stub clients and have properties to force certain behaviors. This works well for test coverage but can easily miss subtleties in the github package and we have to do the unusual pattern of breaking up the github client into it's services to simplify testing. I'm not sure having the interfaces would be something we'd use as it's nice limiting the interface to what we use BUT if the interface is defined by a githubtest package it might be easier to use during upgrades. More recently I've been experimenting with test packages that makes it easy to use packages like httptest and so far that's worked really well and is becoming a preference where possible: // In Test
ghc := githubtest.New(
githubtest.WithOrgs(&github.Organization{}),
githubtest.WithOrgMembers(&github.Member{}),
githubtest.ForceListReposError(map[string]error{
"failing-org": error.New("Injected error"),
}),
)
c := &myClient{
github: ghc,
}
err := c.DoSomething()
if err != nil {
// Fail test...
}This is a little verbose in some respects but it abstracts the underlying test infra / magic, i.e. httptest, while letting us use the concrete types in our code. tl;dr: If you move forward with this I'd 100% move it to a separate test package to maintain a healthy split between library code vs test code. I agree with rspier that a wrapper for httptest of some kind would be really interesting and I would ❤️ to see what could be done there but I suspect it would be a larger undertaking that this current CL (and there may be situations where interfaces + stubs are more appropriate). |
|
@gauntface - I've tried to implement something like what you've described, but I'm hitting a wall. When you get a chance, please take a look at:
and see if you can give me any guidance (or examples) of how you envision this to work. Currently, I'm running up against errors like this: and can't seem to figure out how I can get the code to call the |
For my usecase this would be a bit hard to use as we inject the whole github client to another struct like so: type MySVC struct {
gh github.Client
}Far as I can tell, unless the whole client could be wrapped around an interface* this code wouldn't be able to create any kind of mocked implementation to help during the unittests as the return of * I think, currently it wouldn't be possible as inside the This might also be related to the issue Glenn faced. |
|
@gmlewis I see where you were going with that. The missing piece is the httptest usage. That way you would use the normal organization service. A basic example here (without generation): https://github.com/google/go-github/compare/master...gauntface:githubtest-example?expand=1 The most important part is: func New(t *testing.T, opts ...Option) (*github.Client, func()) {
t.Helper()
// Sort through inputs from test to dictate behavior of the
o := &options{}
for _, oo := range opts {
err := oo(o)
if err != nil {
t.Fatalf("Failed to process githubtest.New() options: %v", err)
}
}
ts := httptest.NewServer(http.HandlerFunc(fakeResponses(o)))
gh := github.NewClient(ts.Client())
tu := ts.URL
if !strings.HasSuffix(tu, "/") {
tu = tu + "/"
}
u, err := url.Parse(tu)
if err != nil {
t.Fatalf("Failed to parse test server URL: %v", err)
}
gh.BaseURL = u
return gh, ts.Close
}
func fakeResponses(opts *options) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
switch r.URL.Path {
case "/user/orgs":
b, err := json.Marshal(opts.Organizations[""])
if err != nil {
fmt.Printf("Failed to marshal response for %v\n", r.URL)
return
}
io.WriteString(w, string(b))
default:
fmt.Printf("Unhandled network request: %v\n", r.URL)
}
}
}This does as @migueleliasweb commented above and returns an actual implementation of the GitHub client, it just points to a fake server. There are plenty of things to worry about with this approach, my primary concern is whether this scales and what the right interface is for defining behaviors is. For example, in the example I skipped over handling func fakeResponses(opts *options) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
for u, d := range opts.calls {
if u != r.URL.Path {
continue
}
io.WriteString(w, d[0])
// TODO: Increment a counter or remove first item from calls queue
}
fmt.Printf("Unhandled network request: %v\n", r.URL)
}
}
type options struct {
calls map[string][]string
}
func WithOrganizations(user string, orgs []*github.Organization) Option {
return func( o *options) {
url = "/user/orgs"
if u != "" {
u := fmt.Sprintf("/user/%v/orgs")
}
o.calls[u] = append(o.calls[u], string(MustMarshal(orgs)))
}
}This may be a terrible idea. I've done this for a few projects where I've needed to fake out small portions of an API. Scaling this up to all of the GitHub project might result in headaches/hit unexpected limits. |
|
Thank you, @gauntface ! OK, it seems like the scope of this helper (and specifically its options) could truly be unbounded depending on what the downstream test wishes to accomplish. I'm having no epiphanies at present, so I'm going to let this sit for now. Please feel free to clue me in if I'm missing something fundamental that would be generally useful. (Would it simplify this implementation if the downstream test was required to provide their own pre-configured handler(s)? I'm not sure.) |
|
Nope you're spot on and I agree the arguments are the thing that concerns me most in terms of the ballooning in API. I will confess this approach assumes you want to hide as much of the complexity in the test package. What you're describing could be a simple middle ground to test if the general approach is desirable (i.e. give the githubtest package a function to call when requests occur and the caller determines the behavior). The big benefit here is that the test can force any range of behaviors and there is no limit to the public APi of githubtest. |
|
Hey @gmlewis , it's me again. Just like you said last time, I also let this idea sit for a lil bit and tried to tackle it with a fresher mind. This time, my starting point was @gauntface implementation. I quite liked the idea of leaving the tests kinda detatched in a way and just mocking/faking the server responses. Please have a look and let me know what you think. If you reckon this implementation might be promissing, I could enable the mock to some more methods. This is what the end user testing api would look like: mockedHttpClient := NewMockHttpClient(
WithRequestMatch(
RequestMatchUsersGet,
MustMarshall(github.User{
Name: github.String("foobar"),
}),
),
WithRequestMatch(
RequestMatchOrganizationsList,
MustMarshall([]github.Organization{
{
Name: github.String("foobar123thisorgwasmocked"),
},
}),
),
)
c := github.NewClient(mockedHttpClient)PR link:#1980 |
|
After a fair amount of discussion, I think it is clear that we don't want to go this route, and in fact, it may make sense to provide a separate, external repo for the purpose of mocking this client library as one possible way to go about unit testing. See #1980 (comment) for further context. Closing. |
Fixes #1800.