Test stub design discussion

[feiranchen]

I might be overlooking or misunderstanding but I have a couple of comments below regarding the commit https://github.com/octokit/go-octokit/commit/a0e207f2f2bd904a9af8a7a5ade4e355f0d50b6a. Please let me know what you think of the following and correct me if I'm wrong.

I would suggest renaming params to respHeaderParams to eliminate confusion.

Further, I think we are missing something like this:

w.WriteHeader(http.StatusUnauthorized)

Maybe we should have an argument for the status code. NOTE: If not, explicitly called, the following status code is automatically filled in the header.

w.WriteHeader(http.StatusOK)

One more thing, if we want to do some testing on the header of the request, shall we take a map of wanted values for this purpose? Here is one example of such use case.

  testHeader(t, r, "Accept", defaultMediaType)
  testHeader(t, r, "User-Agent", userAgent)
  testHeader(t, r, "Authorization", "token token")

I think there are more that need to be tested and expanded, such as support for validation for post and path request bodies.

I have changed the stubRequest signature and content to demonstrate where I'm going with this. stubGet currently has dummy place holders. @pengwynn

[feiranchen]

The test helper is ready for review. @pengwynn Please let me know what you think. I have explained my intentions in the comments under this pull request. I will document it in comments once we confirm a final design.

[pengwynn]

Thanks for opening this @feiranchen. It helps to make the discussion easier to follow. I like the renaming of the params argument to make it clearer. :sparkles:

I'm not sold on the rest of the changes because I think it adds some complexity to the vast majority of the call sites where it's not needed. I think if we need a test to wire up more HTTP details, we'd just drop down and do it inline, without passing a lot of context to the helper.

[feiranchen]

Thanks for your feedback! So as the current code base stands, http GET has a good test helper, but http POST and PATCH are not able to use the test helper at all. That was the motivation of this change - to design a generic test helper that everyone can use. I noticed that the tests for the request header, body, method etc all need to be wrapped in the callback. Because of this I don't see an obvious way of having all tests use some sort of "base" helper, and simply add additional lines of code after calling the "base" helper and finish the testing effort.

From what I understand, get methods don't need all the fields. However both PUT and PATCH will always need to test all of method, header as well as body. As a result, the test files like the following one looks inconsistent. As TestIssuesService_Create and TestIssuesService_Update are the only ones that don't use test helpers: https://github.com/octokit/go-octokit/blob/19d6ec2214395534e105b3af0efa28a38c5a63cc/octokit/issues_test.go#L47 I personally think that in order to understand what's going on here, developers have to read through the helper themselves.

Maybe a good solution would be to implement wrappers like getTestHelper and patchTestHelper, so that those GET requests don't have to fill all the unnecessary arguments. Exactly what stubGet is doing now: https://github.com/octokit/go-octokit/blob/19d6ec2214395534e105b3af0efa28a38c5a63cc/octokit/octokit_test.go#L126

Another suggestion: I think we should clearly define what the helper do and doesn't do, so developers don't have to read through every line before they decide if they will need to add additional lines. As such, I'm thinking of separating the 2 steps "Testing the incoming request", and "Construct the outgoing response" to 2 separate helper methods. What do you think?

[pengwynn]

@feiranchen Thanks for exploring this.

Were you wanting to create stubPatch and stubPost on top of this to see how much benefit they provide over just doing mux.HandleFunc()?

[feiranchen]

@pengwynn Yes I'd love to do that if you think it can be a good idea.

[pengwynn]

@feiranchen Yeah go for it!

[feiranchen]

Methods renamed. Implemented proposed test stubs. Refactored Gists to demonstrate usage. All inputs are welcome!

Thanks.