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

Add support for team pr review requests #532

Merged
merged 12 commits into from
Oct 6, 2019

Conversation

farmdawgnation
Copy link
Contributor

This PR adds support for team pr review requests. Added a test that covers individual review requests but I'm pretty sure the github-api-test-org will need a team added for me to test team review requests.

@farmdawgnation
Copy link
Contributor Author

Hey @bitwiseman - any chance of getting some PR review here? :)

@bitwiseman
Copy link
Member

@farmdawgnation
I know it's a bit to ask but I've just added a mocking framework that will let us run tests in offline mode in CI.

I've resolve the merge conflict, but haven't done a build or further test.

Could you try the instructions in CONTRIBUTING.md for creating new tests and snapshots of github response data? It is still a little rough around the edges, but I think it will be a big step forward for testing and stability. I've invited you to be a collaborator in the test repo that we're using. I think that's all you'll need from me. Thanks in advance.

If you don't have time, I'll get back to it later this week.

GHUser kohsuke2 = gitHub.getUser("kohsuke2");
p.requestReviewers(Collections.singletonList(kohsuke2));
assertFalse(p.getRequestedReviewers().isEmpty());
}
Copy link
Member

Choose a reason for hiding this comment

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

@farmdawgnation
Good test for requestReviewers, next you'll do a test for requestTeamReviewers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I will.

@farmdawgnation
Copy link
Contributor Author

Could you try the instructions in CONTRIBUTING.md for creating new tests and snapshots of github response data? It is still a little rough around the edges, but I think it will be a big step forward for testing and stability. I've invited you to be a collaborator in the test repo that we're using. I think that's all you'll need from me. Thanks in advance.

I can give it a shot, but I may first attempt to write a test using the existing format for time's sake.

@farmdawgnation
Copy link
Contributor Author

@bitwiseman I've gotten the wiremock data in place for the existing test I wrote and tests now pass locally for me. However, because I was added as an outside collaborator on a single repository it seems I don't have the ability to see what teams exist in github-api-test-org or tag them in reviews so I still can't write my test for the team pr reviews feature.

Screen Shot 2019-09-21 at 10 33 08 AM

Could you please add a team that has the kohsuke2 user in it and make it so that I can see it?

@farmdawgnation
Copy link
Contributor Author

Hm my tests are failing in CI, but they work locally. Seeing this in the CI log:

org.kohsuke.github.HttpException: Server returned HTTP response code: -1, message: 'null' for URL: http://localhost:63049/repos/github-api-test-org/github-api/pulls/298
	at org.kohsuke.github.PullRequestTest.testPullRequestReviewRequests(PullRequestTest.java:118)
Caused by: java.net.ConnectException: Connection refused (Connection refused)
	at org.kohsuke.github.PullRequestTest.testPullRequestReviewRequests(PullRequestTest.java:118)

What did I do wrong here?

@bitwiseman
Copy link
Member

@farmdawgnation
Sorry for slow response.
I've invited you to be an owner on github-api-test-org. That will give you rights to do whatever you need to.

Try deleting your wiremock data, rebasing/merging from master, and running again. I've made a bunch of fixes and improvements to the framework.

@bitwiseman
Copy link
Member

@farmdawgnation
FYI, I intend to get a new version published in the next week or two. I look forward to including this change.

@farmdawgnation
Copy link
Contributor Author

Word, I'll try to get it in :)

@farmdawgnation
Copy link
Contributor Author

Yay! tests pass!

@bitwiseman bitwiseman merged commit f0dc7d5 into hub4j:master Oct 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants