Skip to content

Add support for team pr review requests#532

Merged
bitwiseman merged 12 commits intohub4j:masterfrom
farmdawgnation:team-pr-review-requests
Oct 6, 2019
Merged

Add support for team pr review requests#532
bitwiseman merged 12 commits intohub4j:masterfrom
farmdawgnation:team-pr-review-requests

Conversation

@farmdawgnation
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

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

@bitwiseman
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

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

@farmdawgnation
Copy link
Copy Markdown
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