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

Mock transport with pc frames #2008

Closed

Conversation

migueleliasweb
Copy link
Contributor

@migueleliasweb migueleliasweb commented Jul 16, 2021

An attempt to address #1800

  • Uses runtime.Frames to get a the caller method in the SDK and use it to mock a response
  • Uses code generation to automagically generate all possible matches for all exported methods currently in the SDK

@google-cla google-cla bot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label Jul 16, 2021
@migueleliasweb
Copy link
Contributor Author

I've added the code generation to all methods now. Let me know your thoughts @gmlewis 👍 .

@codecov
Copy link

codecov bot commented Jul 16, 2021

Codecov Report

Merging #2008 (257d492) into master (f47f8fe) will decrease coverage by 0.39%.
The diff coverage is 56.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2008      +/-   ##
==========================================
- Coverage   97.84%   97.44%   -0.40%     
==========================================
  Files         105      108       +3     
  Lines        6809     6968     +159     
==========================================
+ Hits         6662     6790     +128     
- Misses         80      110      +30     
- Partials       67       68       +1     
Impacted Files Coverage Δ
mock/client.go 56.33% <56.33%> (ø)
github/checks.go 100.00% <0.00%> (ø)
github/repos_commits.go 100.00% <0.00%> (ø)
github/actions_runners.go 100.00% <0.00%> (ø)
github/actions_runner_groups.go 100.00% <0.00%> (ø)
github/repos_hooks_deliveries.go 93.10% <0.00%> (ø)
github/orgs_hooks_deliveries.go 100.00% <0.00%> (ø)
github/github.go 97.07% <0.00%> (+0.04%) ⬆️
github/messages.go 100.00% <0.00%> (+2.94%) ⬆️

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...257d492. Read the comment docs.

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @migueleliasweb . I'm liking this approach.
You will also want to add a new generator to this PR here:
https://github.com/google/go-github/blob/master/github/github.go#L6-L7


func MustMarshal(v interface{}) []byte {
b, err := json.Marshal(v)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this blank line.

}
}

func NewMockHttpClient(options ...MockHttpClientOption) *http.Client {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add godoc comment.

Suggested change
func NewMockHttpClient(options ...MockHttpClientOption) *http.Client {
func NewMockHTTPClient(options ...MockHTTPClientOption) *http.Client {


type MockHttpClientOption func(*MockRoundTripper)

func WithRequestMatch(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add godoc comment.


var _ http.RoundTripper = &MockRoundTripper{}

type MockHttpClientOption func(*MockRoundTripper)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add godoc comment.

Suggested change
type MockHttpClientOption func(*MockRoundTripper)
type MockHTTPClientOption func(*MockRoundTripper)

RequestMocks map[RequestMatch][][]byte
}

// RoundTrip implements http.RoundTripper interface
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// RoundTrip implements http.RoundTripper interface
// RoundTrip implements http.RoundTripper interface.


if receiverType, ok := fn.Recv.List[0].Type.(*ast.StarExpr); ok {
if receiverTypeIndent, ok := receiverType.X.(*ast.Ident); ok {
if receiverTypeIndent.Name != "" {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line can then be deleted if you take my suggestions above.

if receiverTypeIndent.Name != "" {
varName := fmt.Sprintf(
"RequestMatch%s%s",
strings.Replace(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the reduction in indentation above, this strings.Replace could all fit nicely on a single line.

return requestMatches[i].Value < requestMatches[j].Value
})

// fmt.Println("requestMatches:", requestMatches)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this debugging line.


// fmt.Println("requestMatches:", requestMatches)

buf := bytes.NewBufferString("")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
buf := bytes.NewBufferString("")
var buf bytes.Buffer

buf := bytes.NewBufferString("")

template.Must(template.New("source").Parse(source)).Execute(
buf,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
buf,
&buf,

@gmlewis
Copy link
Collaborator

gmlewis commented Jul 16, 2021

After further consideration, I think that @willnorris is correct in his comment that this would be a great utility to place in an external helper repo.

So my recommendation is that we close this PR and also close #1800 and point to a new helper repo in this repo's README.md once it has been implemented.

@gmlewis gmlewis added the DO NOT MERGE Do not merge this PR. label Jul 16, 2021
@migueleliasweb
Copy link
Contributor Author

Thanks @gmlewis for the extensive review!

I left a comment in the initial PR (#1980) so you and Will can talk more. I will wait until the decision is made to move this to a new repo or not before proceeding with the rest of the implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indication that the PR author has signed a Google Contributor License Agreement. DO NOT MERGE Do not merge this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants