[lingsamuel]

Fixes #797

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
• In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

[lingsamuel]

I signed it!

[googlebot]

CLAs look good, thanks!

[sahildua2305]

@lingsamuel while your changes look ok, I have reservations about why we need to have custom media type for this endpoint.

Especially, we have test for Apps.ListUserRepos here which isn't failing.

@shurcooL what do you think?

[sahildua2305]

Just tested, I'm fine with the changes. @lingsamuel thanks for proactively submitting a PR to fix the issue. LGTM.

@shurcooL however, this actually exposes the weakness of our tests when it comes to header restrictions on some endpoints. For example - test for Apps.ListUserRepos passes even without the header restrictions. Maybe we can enforce the headers in mux requests setup.

[dmitshur]

@lingsamuel Can you also modify the test for this endpoint, specifically in file apps_installation_test.go , after line 47, add:

testHeader(t, r, "Accept", mediaTypeIntegrationPreview)

Similar to:

go-github/github/apps_installation_test.go

Line 22 in a89ea1c

testHeader(t, r, "Accept", mediaTypeIntegrationPreview)

That will ensure the test fails should the method fail to set the header in the future.

@sahildua2305 I'm not sure what you mean. The current Apps.ListUserRepos test passes because it doesn't have restrictions on the headers, which is normal for a method we didn't think needed any custom headers. Perhaps you can clarify on what you expected to happen instead?

[dmitshur]

This part looks good.

[sahildua2305]

@shurcooL if I remove this line in test for ListRepos method, I'll expect the test to fail because it has the header restriction.

go-github/github/apps_installation_test.go

Line 22 in a89ea1c

testHeader(t, r, "Accept", mediaTypeIntegrationPreview)

However, on my local setup, the tests still pass. Are you able to reproduce this?

[dmitshur]

The testHeader is a helper that checks that a header is set. If you remove that line, you're removing the check, so the test will of course pass.

The way you can check that testHeader works is by removing the req.Header.Set("Accept", mediaTypeIntegrationPreview) line from AppsService.ListRepos method, then its test (with testHeader) will fail.

[sahildua2305]

Ah okay! Thanks for explaining, @shurcooL. My understanding of the testHeader method was wrong.

[lingsamuel]

@shurcooL All done.

By the way, I think @sahildua2305 means the test should be failed if we can't get right data. Maybe he thought that the test is passed so the function will work correctly.

[dmitshur]

LGTM. Thank you.

[sahildua2305]

@lingsamuel yes, that’s what I was thinking. Moreover, I assumed that testHeader was confirming that the request is successful only with the header mentioned.

[dmitshur]

Moreover, I assumed that testHeader was confirming that the request is successful only with the header mentioned.

It does do that. It's just many tests don't call that helper.

Specifically, testHeader checks for an exact header value match:

func testHeader(t *testing.T, r *http.Request, header string, want string) {
	if got := r.Header.Get(header); got != want {
		t.Errorf("Header.Get(%q) returned %q, want %q", header, got, want)
	}
}

So, if you want, you can use it like this in a test to ensure that the default Accept header value is set:

testHeader(t, r, "Accept", mediaTypeV3)

I'm not sure if there are benefits to doing that, but I haven't thought about it too much. If you think there are worthwhile improvements possible, feel free to open a new issue about it.

(P.S. This is a good example of where helpers make code less readable. I'm sure you wouldn't misunderstand what testHeader does if its code was written directly in the test, rather than being in a helper.)

[lingsamuel]

@shurcooL Oh no, I think @sahildua2305 means, testHeader should ensure that we can't get the right data if we don't set the header.
Not just check if the header exists.

[dmitshur]

Sorry, I don’t understand. If you think there’s a worthwhile opportunity to improve tests, feel free to open a new issue and describe it in more detail. I am satisfied with the current tests as they are, and don’t see any ways to make them significantly better.