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

Set accept header for function Apps.ListUserRepos #798

Merged
merged 5 commits into from Dec 2, 2017

Conversation

Projects
None yet
4 participants
@lingsamuel
Contributor

lingsamuel commented Nov 29, 2017

Fixes #797

lingsamuel added some commits Nov 29, 2017

Set header in function ListUserRepos
Or GitHub may return something contains:
Message: "If you would like to help us test the Integrations API during its preview period, you must specify a custom media type in the 'Accept' header. Please see the docs for full details.",
Merge pull request #1 from futurenda/set-accept-header-for-ListUserRepos
Set accept header in function ListUserRepos
@googlebot

This comment has been minimized.

Show comment
Hide comment
@googlebot

googlebot Nov 29, 2017

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.

googlebot commented Nov 29, 2017

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.

@googlebot googlebot added the cla: no label Nov 29, 2017

@lingsamuel

This comment has been minimized.

Show comment
Hide comment
@lingsamuel

lingsamuel Nov 29, 2017

Contributor

I signed it!

Contributor

lingsamuel commented Nov 29, 2017

I signed it!

@googlebot

This comment has been minimized.

Show comment
Hide comment
@googlebot

googlebot Nov 29, 2017

CLAs look good, thanks!

googlebot commented Nov 29, 2017

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Nov 29, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@sahildua2305

sahildua2305 Nov 30, 2017

Collaborator

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.

Collaborator

sahildua2305 commented Nov 30, 2017

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

dmitshur requested changes Dec 1, 2017 edited

@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:

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?

@@ -67,6 +67,9 @@ func (s *AppsService) ListUserRepos(ctx context.Context, id int, opt *ListOption
return nil, nil, err
}
// TODO: remove custom Accept header when this API fully launches.
req.Header.Set("Accept", mediaTypeIntegrationPreview)

This comment has been minimized.

@dmitshur

dmitshur Dec 1, 2017

Member

This part looks good.

@dmitshur

dmitshur Dec 1, 2017

Member

This part looks good.

@sahildua2305

This comment has been minimized.

Show comment
Hide comment
@sahildua2305

sahildua2305 Dec 1, 2017

Collaborator

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

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

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

Collaborator

sahildua2305 commented Dec 1, 2017

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

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

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

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Dec 1, 2017

Member

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.

Member

dmitshur commented Dec 1, 2017

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

This comment has been minimized.

Show comment
Hide comment
@sahildua2305

sahildua2305 Dec 1, 2017

Collaborator

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

Collaborator

sahildua2305 commented Dec 1, 2017

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

lingsamuel added some commits Dec 2, 2017

Merge pull request #2 from futurenda/add-test-for-list-user-repos
Add header test for Apps.ListUserRepos
@lingsamuel

This comment has been minimized.

Show comment
Hide comment
@lingsamuel

lingsamuel Dec 2, 2017

Contributor

@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.

Contributor

lingsamuel commented Dec 2, 2017

@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.

@dmitshur dmitshur merged commit b527232 into google:master Dec 2, 2017

1 check passed

cla/google All necessary CLAs are signed
@sahildua2305

This comment has been minimized.

Show comment
Hide comment
@sahildua2305

sahildua2305 Dec 2, 2017

Collaborator

@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.

Collaborator

sahildua2305 commented Dec 2, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Dec 2, 2017

Member

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.)

Member

dmitshur commented Dec 2, 2017

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

This comment has been minimized.

Show comment
Hide comment
@lingsamuel

lingsamuel Dec 4, 2017

Contributor

@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.

Contributor

lingsamuel commented Dec 4, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Dec 4, 2017

Member

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.

Member

dmitshur commented Dec 4, 2017

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.

nbareil pushed a commit to nbareil/go-github that referenced this pull request May 1, 2018

Set Accept header in AppsService.ListUserRepos. (google#798)
Without this custom media type in the Accept header, GitHub API responds
with:

	{
	  "message": "If you would like to help us test the Integrations API during its preview period, you must specify a custom media type in the 'Accept' header. Please see the docs for full details.",
	  "documentation_url": "https://developer.github.com/v3"
	}

The custom media type requirement is documented at
https://developer.github.com/changes/2016-09-14-Integrations-Early-Access/.

Fixes google#797.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment