Add draft for mock http client for unittesting #1980

migueleliasweb · 2021-07-12T12:27:55Z

No description provided.

gmlewis · 2021-07-12T12:48:13Z

Thanks, @migueleliasweb .
Before doing a full code review, let's please get all the tests passing.

It is currently stuck on:

Run go vet ./...
# github.com/google/go-github/v37/mock
vet: mock/client.go:62:20: NopCloser not declared by package io
Error: Process completed with exit code 2.

migueleliasweb · 2021-07-12T12:59:31Z

Thanks, @migueleliasweb .
Before doing a full code review, let's please get all the tests passing.

It is currently stuck on:
Run go vet ./...
# github.com/google/go-github/v37/mock
vet: mock/client.go:62:20: NopCloser not declared by package io
Error: Process completed with exit code 2.

This is due to the differences in golang 1.16. I changed it to use the ioutil package instead.

gmlewis · 2021-07-12T13:08:02Z

Now the error is:

--- FAIL: TestMockClient (0.00s)
panic: runtime error: index out of range [0] with length 0 [recovered]
	panic: runtime error: index out of range [0] with length 0

goroutine 6 [running]:
testing.tRunner.func1.2(0x8ce040, 0xc00001e570)
	/opt/hostedtoolcache/go/1.16.5/x64/src/testing/testing.go:1143 +0x49f
testing.tRunner.func1(0xc000001800)
	/opt/hostedtoolcache/go/1.16.5/x64/src/testing/testing.go:1146 +0x695
panic(0x8ce040, 0xc00001e570)
	/opt/hostedtoolcache/go/1.16.5/x64/src/runtime/panic.go:971 +0x499
github.com/google/go-github/v37/mock.(*MockRoundTripper).RoundTrip(0xc000010290, 0xc000146600, 0x0, 0x0, 0x0)
	/home/runner/work/go-github/go-github/mock/client.go:52 +0x5c5
net/http.send(0xc000146600, 0x971340, 0xc000010290, 0x0, 0x0, 0x0, 0x0, 0x0, 0xc0000102b0, 0xc00016b508)
	/opt/hostedtoolcache/go/1.16.5/x64/src/net/http/client.go:251 +0x6db
net/http.(*Client).send(0xc00006d5f0, 0xc000146600, 0x0, 0x0, 0x0, 0xc0000102b0, 0x0, 0x1, 0x730000004a82e2)
	/opt/hostedtoolcache/go/1.16.5/x64/src/net/http/client.go:175 +0x1d6
net/http.(*Client).do(0xc00006d5f0, 0xc000146600, 0x0, 0x0, 0x0)
	/opt/hostedtoolcache/go/1.16.5/x64/src/net/http/client.go:717 +0x2cc

migueleliasweb · 2021-07-12T13:43:24Z

Now the error is:

--- FAIL: TestMockClient (0.00s)
panic: runtime error: index out of range [0] with length 0 [recovered]
	panic: runtime error: index out of range [0] with length 0

goroutine 6 [running]:
testing.tRunner.func1.2(0x8ce040, 0xc00001e570)
	/opt/hostedtoolcache/go/1.16.5/x64/src/testing/testing.go:1143 +0x49f
testing.tRunner.func1(0xc000001800)
	/opt/hostedtoolcache/go/1.16.5/x64/src/testing/testing.go:1146 +0x695
panic(0x8ce040, 0xc00001e570)
	/opt/hostedtoolcache/go/1.16.5/x64/src/runtime/panic.go:971 +0x499
github.com/google/go-github/v37/mock.(*MockRoundTripper).RoundTrip(0xc000010290, 0xc000146600, 0x0, 0x0, 0x0)
	/home/runner/work/go-github/go-github/mock/client.go:52 +0x5c5
net/http.send(0xc000146600, 0x971340, 0xc000010290, 0x0, 0x0, 0x0, 0x0, 0x0, 0xc0000102b0, 0xc00016b508)
	/opt/hostedtoolcache/go/1.16.5/x64/src/net/http/client.go:251 +0x6db
net/http.(*Client).send(0xc00006d5f0, 0xc000146600, 0x0, 0x0, 0x0, 0xc0000102b0, 0x0, 0x1, 0x730000004a82e2)
	/opt/hostedtoolcache/go/1.16.5/x64/src/net/http/client.go:175 +0x1d6
net/http.(*Client).do(0xc00006d5f0, 0xc000146600, 0x0, 0x0, 0x0)
	/opt/hostedtoolcache/go/1.16.5/x64/src/net/http/client.go:717 +0x2cc

For some reason this seems to be a flakey test 🤔 It passes most times but eventually fails. I'm troubleshooting....

migueleliasweb · 2021-07-13T05:09:17Z

The tests should pass now 🤞 . Ping @gmlewis 🔔 .

codecov · 2021-07-13T11:59:18Z

Codecov Report

Merging #1980 (bb74f7b) into master (f47f8fe) will decrease coverage by 0.40%.
The diff coverage is 50.84%.

@@            Coverage Diff             @@
##           master    #1980      +/-   ##
==========================================
- Coverage   97.84%   97.43%   -0.41%     
==========================================
  Files         105      106       +1     
  Lines        6809     6868      +59     
==========================================
+ Hits         6662     6692      +30     
- Misses         80      108      +28     
- Partials       67       68       +1

Impacted Files	Coverage Δ
mock/client.go	`50.84% <50.84%> (ø)`
github/checks.go	`100.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f47f8fe...bb74f7b. Read the comment docs.

gmlewis · 2021-07-13T12:04:46Z

OK, thanks, @migueleliasweb .

I have some big concerns about this approach... especially from a maintenance perspective.

to support the full library, each endpoint will need to be (manually most likely) added and the resulting size is going to be huge
this is driven with regexps which are challenging to always get right (and not interfere with each other)

Before doing a full code review (where I would suggest that headers be added and Marshal should have a single l, for example), I would like to hear what @gauntface and @willnorris think about this approach.

migueleliasweb · 2021-07-13T12:19:41Z

OK, thanks, @migueleliasweb .

I have some big concerns about this approach... especially from a maintenance perspective.

to support the full library, each endpoint will need to be (manually most likely) added and the resulting size is going to be huge

this is driven with regexps which are challenging to always get right (and not interfere with each other)

Before doing a full code review (where I would suggest that headers be added and Marshal should have a single l, for example), I would like to hear what @gauntface and @willnorris think about this approach.

Damn it, got Marshal wrong. I always spell it the wrong way. Second language problems, sry.

Yeah,I'm afraid you're not wrong about the regexps.

I was trying to come up with another way to match the requests but I ended up trying this one first. I thought of two other ways:

Try to use runtime.CallersFrames inside the custom RoundTripper to determine the last call made in the go-github code thus finding the caller function in the SDK. With this approach, the matcher code could be something much more clear like a string UsersService.Get instead of the somewhat ugly regexps.
Add a header to the request and match the request to that inside the RoundTripper to make it easier to distinguish between calls

Ps: fixed the typo on MustMarshal. I always feel marshal should be written with two Ls 😂

migueleliasweb · 2021-07-13T14:30:32Z

I just wrote a poc with the runtime frames. The API looks much cleaner without the regexps and I reckon it would be doable to auto generate all endpoints in this case.

Have a look: https://github.com/migueleliasweb/go-github/blob/mock-transport-pc-frames/mock/client.go

gmlewis · 2021-07-13T14:56:17Z

I just wrote a poc with the runtime frames. The API looks much cleaner without the regexps and I reckon it would be doable to auto generate all endpoints in this case.

Have a look: https://github.com/migueleliasweb/go-github/blob/mock-transport-pc-frames/mock/client.go

Ah, wow... that is quite intriguing, @migueleliasweb !

I'm guessing that the GitHub API call would be closer to the "recent" end of the call stack as opposed to the "oldest" end of the call stack... so it might make the most sense (from a performance perspective) to search for the call from the end to the beginning... actually maybe you are already doing that... if so, great.

I'm not too concerned about the performance implications of the endpoint search... I think this would be much easier to support than the other solution... especially if the critical list of methods that users would use are auto-generated.

Overall, I like this approach a lot... I think you are on to something big here!

willnorris · 2021-07-13T15:26:41Z

(just some random thoughts...)

The use of caller frames to match the intercepted request is certainly an interesting approach, and one I probably wouldn't have considered. But I think it's a bit more rigid in tying the test to the method that made the http request, rather than what the request itself is.

This is very close to being a really interesting, generic, standalone approach to intercepting and mocking outbound http requests. If that were the goal (to make this reusable beyond just go-github), then I wonder if the original URL matching approach might be more flexible. I also wonder if some of the regex logic could be made safer (or at least more maintainable) by using some of the path parsing and matching logic in something like gorillatoolkit (or similar framework)? That could allow writing paths to match using a friendlier syntax like /users/{user}

migueleliasweb · 2021-07-14T05:22:16Z

(just some random thoughts...)

The use of caller frames to match the intercepted request is certainly an interesting approach, and one I probably wouldn't have considered. But I think it's a bit more rigid in tying the test to the method that made the http request, rather than what the request itself is.

The problem as @gmlewis already pointed out, there's not (as far as I know) way to determine which call was made in the current state of the codebase. Using the regexps could end up being a bit tedious to handcraft all the required expressions and ensure they don't collide to each other. The workaround was the runtime.Frames poc I wrote.

I also wonder if some of the regex logic could be made safer (or at least more maintainable) by using some of the path parsing and matching logic in something like gorillatoolkit (or similar framework)? That could allow writing paths to match using a friendlier syntax like /users/{user}

Using something like the gorillamux would definitely make the request matching much more friendly but I thought this repo in general was trying to not bring more remote dependencies. And also, once again, maintaining all the paths, although it would be much easier with the gorilla mux or something similar, would still mean all those paths would have to be handcrafted and maintained for the future.

Thanks for the feedbackm @willnorris ! 👍

migueleliasweb · 2021-07-14T05:23:06Z

I will try to add the code generation to the runtime.Frame poc PR for some of the service methods to see how it would look.

migueleliasweb · 2021-07-16T14:26:30Z

Closing in favor of #2008

willnorris · 2021-07-16T17:55:16Z

The problem as @gmlewis already pointed out, there's not (as far as I know) way to determine which call was made in the current state of the codebase. Using the regexps could end up being a bit tedious to handcraft all the required expressions and ensure they don't collide to each other. The workaround was the runtime.Frames poc I wrote.

We're primarily talking about using this technique to test application code that is calling into go-github. Ideally, you'd be testing discrete functionality, so I'd be surprised if there were that many overlapping API calls in a given unit test. And I'm not sure that the Frames approach necessarily solves that problem anyway, but rather moves it. You could still have multiple outbound calls that are originating from the same function, so you've still got overlapping calls.

Regardless of the approach (URL matching or frame matching), I don't think this belongs in go-github. We're talking about rather generic approaches to intercepting output calls, which could make a very interesting standalone library, similar to https://github.com/nock/nock in Node. In fact, I think Nock provides a good example of doing this with URL matching and might provide a pretty well-tested API to potentially model (if you wanted to take that approach)

Even if this were not fully generic, and is instead specific to go-github, this is only intended for use by users of go-github. It's not used by the library itself. So I think this should really be its own repo, which we can optionally link to from go-github's README as one approach to testing applications that use the library. (/cc @gmlewis)

migueleliasweb · 2021-07-17T05:45:06Z

Even if this were not fully generic, and is instead specific to go-github, this is only intended for use by users of go-github. It's not used by the library itself. So I think this should really be its own repo, which we can optionally link to from go-github's README as one approach to testing applications that use the library. (/cc @gmlewis)

It is true that this code is more like a helper other than something integral to this repo so I see no big problem of moving this logic to a new repo. Something like go-github-testing or similar.

Where would this new repo reside? If inside google, I have no access, so the repo would have to be created by someone with proper access to so I could create a PR and we follow up the process in there. If not, I could create it under my user (or any of your users as well).

Also, if we're decided to move this PR to a new repo, @willnorris idea of using like gorilla to have a much more friendly URL matching would become doable. So I reckon I could close #2008 and improve this one.

gmlewis · 2021-07-17T11:31:04Z

Where would this new repo reside?

I see no reason why it couldn't be under your own user name, but we'll see what @willnorris says.

willnorris · 2021-07-21T20:43:07Z

yeah, I was imagining just under your own account, at least to start out. I don't think it will ever make sense to put it under @google, since no one from Google is actively involved with this at this point.

migueleliasweb · 2021-07-27T12:15:17Z

Hi @willnorris and @gmlewis I think I have something that is somewhat presentable. Let me know if, other than the missing CI, there's something else I'm missing.

https://github.com/migueleliasweb/go-github-mock

ps: don't know where to have this conversation, if you guys prefer to create issues in the repo itself, feel free

gmlewis · 2021-07-27T12:35:01Z

Sounds great, @migueleliasweb ! Thanks for doing that!

Feel free to create a PR to add a reference to your new repo in this repo's README.md so we can point people to it.

migueleliasweb · 2021-07-27T12:53:33Z

Will do, @gmlewis . I just wanted a couple more hours to ensure the tests are being run in the CI workflow before opening the floodgates 😂 .

Add draft for mock http client for unittesting

a05ca1c

google-cla bot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label Jul 12, 2021

migueleliasweb mentioned this pull request Jul 12, 2021

Auto-generate interfaces for all services #1832

Closed

Use ioutil for NopCloser

6ed3ab7

Fix unittests for draft mock http client

804c61e

Fix typo on mock/client.go and mock/client_test.go

bb74f7b

migueleliasweb closed this Jul 16, 2021

gmlewis mentioned this pull request Jul 16, 2021

Mock transport with pc frames #2008

Closed

Add draft for mock http client for unittesting #1980

Add draft for mock http client for unittesting #1980

Uh oh!

Conversation

migueleliasweb commented Jul 12, 2021

Uh oh!

gmlewis commented Jul 12, 2021

Uh oh!

migueleliasweb commented Jul 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmlewis commented Jul 12, 2021

Uh oh!

migueleliasweb commented Jul 12, 2021

Uh oh!

migueleliasweb commented Jul 13, 2021

Uh oh!

codecov bot commented Jul 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gmlewis commented Jul 13, 2021

Uh oh!

migueleliasweb commented Jul 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

migueleliasweb commented Jul 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmlewis commented Jul 13, 2021

Uh oh!

willnorris commented Jul 13, 2021

Uh oh!

migueleliasweb commented Jul 14, 2021

Uh oh!

migueleliasweb commented Jul 14, 2021

Uh oh!

migueleliasweb commented Jul 16, 2021

Uh oh!

willnorris commented Jul 16, 2021

Uh oh!

migueleliasweb commented Jul 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmlewis commented Jul 17, 2021

Uh oh!

willnorris commented Jul 21, 2021

Uh oh!

migueleliasweb commented Jul 27, 2021

Uh oh!

gmlewis commented Jul 27, 2021

Uh oh!

migueleliasweb commented Jul 27, 2021

Uh oh!

Uh oh!

migueleliasweb commented Jul 12, 2021 •

edited

Loading

codecov bot commented Jul 13, 2021 •

edited

Loading

migueleliasweb commented Jul 13, 2021 •

edited

Loading

migueleliasweb commented Jul 13, 2021 •

edited

Loading

migueleliasweb commented Jul 17, 2021 •

edited

Loading