Add httpmock from cli/cli about go-gh

[samcoe]

I like this idea and I think we should do it. I already have migrated it to go-gh so it really just needs to be moved from the internal folder to export it.

Before making it widely available there are a couple changes I would like to see to the interface while we still can:

1. Should the name change to something like TransportMock or RoundTripperMock?
2. Stubs should have names when they are registered with Register. This will allow for better error messaging directly referencing the stub name.
3. We should remove the more obscure responders that are not used in go-gh, for example FileResponse and ScopesResponder. Perhaps there are others there that we can remove.

These will require changes to the go-gh test suit so I think approaching it as making the interfaces changes first and then exporting the package in a second PR is the right way to go here.

[heaths]

1. I like TransportMock a bit better. Seems more obvious and discoverable.
2. 👍
3. While we're at it, would be nice to define a pattern for taking a string vs. object or map to marshal. I had started some changes like this for one of my recent PRs since I already had an object in test I could marshal, and construction of it is more straight forward in some cases. Certainly just a preformatted JSON response string in other cases is great. For HTTP this already exists, but something similar for GraphQL would be nice. Maybe some pattern like {protocol}{"Request"|"Response"}({"String"|"JSON"}) e.g., "HTTPRequest", "HTTPResponseString", "GraphQLRequest", or "GraphQLResponseJSON".

[samcoe]

Great, TransportMock it is.

I also had thoughts on defining a naming scheme for responders as well. I think our responders can be smarter than they currently are. In my opinion each should be able to handle both a string and a struct, if it is not a string we will try to marshal it to a JSON string. In this scenario we don't need to figure out a naming scheme for what takes in a string or a struct. I wrote up some code below on how I am envisioning the responders. Let me know what you think.

func HTTPResponse(status int, header map[string][]string, body interface{}, cb func(*http.Request)) Responder {
	return func(req *http.Request) (*http.Response, error) {
		var b io.Reader
		if s, ok := body.(string); ok {
			b = bytes.NewBufferString(s)
		} else {
			s, _ := json.Marshal(body)
			b = bytes.NewBuffer(s)
		}

		if cb != nil {
			cb(req)
		}

		return httpResponse(status, header, b, req), nil
	}
}

func RESTResponse(body interface{}, cb func(map[string]interface{})) Responder {
	return func(req *http.Request) (*http.Response, error) {
		var b io.Reader
		if s, ok := body.(string); ok {
			b = bytes.NewBufferString(s)
		} else {
			s, _ := json.Marshal(body)
			b = bytes.NewBuffer(s)
		}

		bodyData := make(map[string]interface{})

		err := decodeJSONBody(req, &bodyData)
		if err != nil {
			return nil, err
		}

		if cb != nil {
			cb(bodyData)
		}

		return httpResponse(200, map[string][]string{}, b, req), nil
	}
}

func GQLMutation(body interface{}, cb func(map[string]interface{})) Responder {
	return func(req *http.Request) (*http.Response, error) {
		var b io.Reader
		if s, ok := body.(string); ok {
			b = bytes.NewBufferString(s)
		} else {
			s, _ := json.Marshal(body)
			b = bytes.NewBuffer(s)
		}

		var bodyData struct {
			Variables struct {
				Input map[string]interface{}
			}
		}

		err := decodeJSONBody(req, &bodyData)
		if err != nil {
			return nil, err
		}

		if cb != nil {
			cb(bodyData.Variables.Input)
		}

		return httpResponse(200, map[string][]string{}, b, req), nil
	}
}

func GQLQuery(body interface{}, cb func(string, map[string]interface{})) Responder {
	return func(req *http.Request) (*http.Response, error) {
		var b io.Reader
		if s, ok := body.(string); ok {
			b = bytes.NewBufferString(s)
		} else {
			s, _ := json.Marshal(body)
			b = bytes.NewBuffer(s)
		}

		var bodyData struct {
			Query     string
			Variables map[string]interface{}
		}

		err := decodeJSONBody(req, &bodyData)
		if err != nil {
			return nil, err
		}

		if cb != nil {
			cb(bodyData.Query, bodyData.Variables)
		}

		return httpResponse(200, map[string][]string{}, b, req), nil
	}
}

func httpResponse(status int, header map[string][]string, body io.Reader, req *http.Request) *http.Response {
	if _, ok := header["Content-Type"]; !ok {
		header["Content-Type"] = []string{"application/json; charset=utf-8"}
	}

	return &http.Response{
		StatusCode: status,
		Header:     header,
		Body:       io.NopCloser(body),
		Request:    req,
	}
}

[heaths]

Overall, I like it, but have a couple thoughts:

1. It's not always true that a REST response returns 200. Sure, someone could use the HTTP response (which are mostly RESTful anyway), but maybe don't imply a format protocol through HTTP or REST. Maybe HTTPResponse or HTTPResponseWithStatus?
2. For RESTResponse, couldn't you avoid the performance hit and memory (granted, they're tests, so maybe 🤷‍♂️) by only decoding a string body and just using the given body as the bodyData otherwise?

[samcoe]

1. True. I am open to naming changes this was just quickly what I put together. I chose the current names as they matched the various clients that go-gh offers.
2. That could be a good improvement, although the cb function signature would have to change to take an interface{} argument which I am hesitant about.

[samcoe]

@heaths As part of the process of exporting httpmock we did an investigation on other http mocking packages out there and the team concluded that moving forward with using the gock package was the right approach for us instead of continuing using our home made httpmock package. With that decision, we are also advocating using gock or another third-party package for http mocking when testing gh extensions so I am going to close this issue out.

[heaths]

@samcoe having had a change to use gock in https://github.com/heaths/gh-projects, there are some positive aspects (simple to set up and no hooks needed if using net/http), but some areas that could be improved - maybe even here or in a separate package. Or just contributed back to gock - something I'm considering but would love to brainstorm ideas.

The main issue that is lacking is trying to match a request. With your httpmock you can match a GraphQL request with just a substring - like a query name, which I do for this very purpose - but then a callback (or any way to assert) for variables. In gock all I've found is gock.Request.Body or gock.Request.BodyString which requires getting the query exactly right - newlines and all - and the variables sorted, though I guess the latter is document and cannot reasonably be changed.

Still, it'd be nice to have a way to not have to encode the entire query. Thoughts?

[mislav]

@heaths You're right that matching on the entire query string would likely lead to brittle tests. Would it be possible to define a custom Request matcher with gock that extracts the GraphQL query name and matches on that?

[heaths]

Seems one could using Request.AddMatcher. Perhaps a function could be added to go-gh that takes an operation name and variable map and returns a MatchFunc.

[samcoe]

@heaths I am not sure that we necessarily need to include this functionality in go-gh. I
went ahead and wrote a function that does what you want and it turned out to be only ~30 lines which seems like something an extension author could implement themselves. The only tricky part which isn't exactly clear is using the TeeReader for reading the body into a copy buffer so that any subsequent matchers can also read the body if they need to.

func matchGQLRequest(query string, vars map[string]string) func(*http.Request, *gock.Request) (bool, error) {
	return func(req *http.Request, ereq *gock.Request) (bool, error) {
		var bodyData struct {
			Query     string
			Variables map[string]string
		}
		bodyCopy := &bytes.Buffer{}
		r := io.TeeReader(req.Body, bodyCopy)
		req.Body = io.NopCloser(bodyCopy)
		b, err := io.ReadAll(r)
		if err != nil {
			return false, err
		}
		err = json.Unmarshal(b, &bodyData)
		if err != nil {
			return false, err
		}
		if query != "" && query != bodyData.Query {
			return false, nil
		}
		if len(vars) != 0 {
			for k, v := range vars {
				bv := bodyData.Variables[k]
				if v != bv {
					return false, nil
				}
			}
		}
		return true, nil
	}
}

It can be used like this:

func TestGQLClient(t *testing.T) {
	stubConfig(t, testConfig())
	t.Cleanup(gock.Off)

	gock.New("https://api.github.com").
		Post("/graphql").
		MatchHeader("Authorization", "token abc123").
		AddMatcher(matchGQLRequest("QUERY", map[string]string{"var": "test"})).
		Reply(200).
		JSON(`{"data":{"viewer":{"login":"hubot"}}}`)

	client, err := GQLClient(nil)
	assert.NoError(t, err)

	vars := map[string]interface{}{"var": "test"}
	res := struct{ Viewer struct{ Login string } }{}
	err = client.Do("QUERY", vars, &res)
	assert.NoError(t, err)
	assert.True(t, gock.IsDone(), printPendingMocks(gock.Pending()))
	assert.Equal(t, "hubot", res.Viewer.Login)
}

[heaths]

It was just an idea to avoid everyone having to figure that out for themselves. Perhaps dubious, but a helper function in go-gh to make it more discoverable and easier for people to use seems like a good idea. I wasn't afraid I wouldn't figure something out, just that perhaps not everyone needs to do so as well. 🙂