Complete checks api #723

XiongKezhi · Feb 29, 2020

Description

Adding more properties to GHCheckRun (in progress) and implementing GHCheckSuite (in progress).

Before submitting a PR:

We love getting PRs, but we hate asking people for the same basic changes every time.

Push your changes to a branch other than master. Create your PR from that branch.
Add JavaDocs and other comments
Write tests that run and pass in CI. See CONTRIBUTING.md for details on how to capture snapshot data.
Run mvn -D enable-ci clean install site locally. This may reformat your code, commit those changes. If this command doesn't succeed, your change will not pass CI.

…te-checks-api

src/main/java/org/kohsuke/github/GHCheckRun.java

bitwiseman · Feb 29, 2020

@XiongKezhi
I'm about to start reviewing this but want to start by saying thank you. This is looks like a big improvement.

bitwiseman

Just so I understand it: the reason why there aren't more tests in here is because, for general reading of check runs (outside of events) we need to have a GHCheckSuite for them right?

bitwiseman · Feb 29, 2020

src/main/java/org/kohsuke/github/GHCheckRun.java

+    private String startedAt;
+    private String completedAt;


These can be Date instances I think and Jackson will handle the conversion.

I just saw the GHObject uses String for such timestamps at first and convert them into Date in getters. And I think if we want to get rid of setters, we may have to do it in this way, or we will get error messages from spot-bugs like may expose internal representation.

bitwiseman · Feb 29, 2020

src/main/java/org/kohsuke/github/GHCheckRun.java

+    private String externalId;
+    private String startedAt;
+    private String completedAt;
+    private URL htmlUrl;


I think the htmlUrl and getHtmlUrl() can be inherited from GHObject. Please confirm.

We can inherit the url and getUrl, but the getHtmlUrl() is abstract in GHObject, so I have to overwrite it.

bitwiseman · Feb 29, 2020

src/main/java/org/kohsuke/github/GHCheckRun.java

+        return app;
+    }
+
+    public Output getOutput() {


Need the javadoc for this.

bitwiseman · Mar 1, 2020

src/main/java/org/kohsuke/github/GHCheckRun.java

+        private String summary;
+        private String text;
+        private int annotationsCount;
+        private URL annotationsUrl;


Do we have a CheckRunAnnotations yet? If not we'll need that added eventually as well.

That's where I'm confused. The response (or event payload) from GitHub does not include annotation objects, but they are needed when users are making requests. So does our implementation have to make sure it supports both usages?

Most probably it should support. Would be beneficial for users in the longer term

It isn't required for this PR, I'm just noting it for later.

bitwiseman · Mar 1, 2020

By the way, you have this marked as draft but this seems like a perfectly reasonable PR to submit and complete on its own if you choose to do so.

XiongKezhi · Mar 1, 2020

By the way, you have this marked as draft but this seems like a perfectly reasonable PR to submit and complete on its own if you choose to do so.

It's because I'm still implementing the GHCheckSuite, I'll make it a normal PR after that. Appreciate your review!

XiongKezhi · Mar 1, 2020

Just so I understand it: the reason why there aren't more tests in here is because, for general reading of check runs (outside of events) we need to have a GHCheckSuite for them right?

I'm not sure which kinds of tests our project requires, and I normally do not test the getters and setters, but I assume we have to test them here because the values are bound by Jackson? Also, I'm not sure how complete our tests have to be, for example, there is a GHRepositoryin our GHEventPayload.CheckRun, do we have to test whether each value of GHRepositoryis correct? I think such a recursive style is not necessary, so I just test some basic properties like id or name of the objects in CheckRun to make sure that Jackson reaches those.

sladyn98 · Mar 1, 2020

src/main/java/org/kohsuke/github/GHCheckRun.java

+        private String summary;
+        private String text;
+        private int annotationsCount;
+        private URL annotationsUrl;


Most probably it should support. Would be beneficial for users in the longer term

sladyn98 · Mar 1, 2020

src/test/java/org/kohsuke/github/GHEventPayloadTest.java

+        GHCheckRun checkRun = event.getCheckRun();
+        assertThat(checkRun.getName(), is("Octocoders-linter"));
+        assertThat(checkRun.getHeadSha(), is("ec26c3e57ca3a959ca5aad62de7213c562f8c821"));
+        assertThat(checkRun.getStatus(), is("completed"));


@XiongKezhi I guess there are multiple states of the check run that need to be tested for, Is this the only one?

Yeah, that's one thing we can do. Before, I just changed the original JSON in order to cover more methods for convenience, but later I found more sample responses here, they are much better and maybe we can use them later.

bitwiseman · Mar 2, 2020

Just so I understand it: the reason why there aren't more tests in here is because, for general reading of check runs (outside of events) we need to have a GHCheckSuite for them right?

I'm not sure which kinds of tests our project requires, and I normally do not test the getters and setters, but I assume we have to test them here because the values are bound by Jackson? Also, I'm not sure how complete our tests have to be, for example, there is a GHRepositoryin our GHEventPayload.CheckRun, do we have to test whether each value of GHRepositoryis correct? I think such a recursive style is not necessary, so I just test some basic properties like id or name of the objects in CheckRun to make sure that Jackson reaches those.

I'm going to give you vague-ish answer. We require test coverage for any new code/features added to the project, but that is a recent decision, so there are lots of gaps in test coverage for the project overall. You do not need to do recursive tests, but we do need tests that show the objects are behaving correctly overall. You can save yourself some testing by not having public setters - then you only need to touch each of the getters at least once. Write a set of tests that your feel are reasonable and we can iterate to add more if they seem needed.

src/main/java/org/kohsuke/github/GHEventPayload.java

bitwiseman · Mar 2, 2020

src/main/java/org/kohsuke/github/GHCommit.java

+         *
+         * @return timestamp of the commit
+         */
+        public Date getTimestamp() {


I don't see it on this page: https://developer.github.com/v3/repos/commits/#get-a-single-commit

I see this:
https://developer.github.com/v3/activity/events/types/#webhook-payload-example-1

It possible the "issue" in GHCheckSuite is not a GHCommit? But instead something else?

sry, didn't notice that commit page and pay much attention to the structure. I'll add it inside GHCheckSuite.

bitwiseman

Looks good. Thanks!

oleg-nenashev · Mar 19, 2020

Nice one!

XiongKezhi and others added 6 commits February 28, 2020 15:40

add more properties to GHCheckRun

10cc79f

format code

05e8148

Fixed typo in JavaDoc of getApp

73179c1

add Output in GHCheckRun

2242174

Merge remote-tracking branch 'origin/complete-checks-api' into comple…

e96067e

…te-checks-api

parse completedAt and startedAt into Date in getters

5a8f8c3

recena reviewed Feb 29, 2020

View reviewed changes

src/main/java/org/kohsuke/github/GHCheckRun.java Outdated Show resolved Hide resolved

XiongKezhi and others added 2 commits February 29, 2020 20:15

Delete empty line in JavaDoc

57f9475

Merge branch 'master' into complete-checks-api

f9006af

bitwiseman reviewed Mar 1, 2020

View reviewed changes

XiongKezhi added 2 commits March 1, 2020 14:01

using Date for timestamps and add more JavaDoc

10b01ca

Change timestamps property back to String

3754175

sladyn98 reviewed Mar 1, 2020

View reviewed changes

XiongKezhi added 3 commits March 1, 2020 21:59

Add GHCheckSuite

16faaae

Format code

f8a8ee9

Fixed JavaDoc warning reported by maven site

f54bfd3

XiongKezhi marked this pull request as ready for review March 2, 2020 06:40

bitwiseman reviewed Mar 2, 2020

View reviewed changes

src/main/java/org/kohsuke/github/GHEventPayload.java Show resolved Hide resolved

bitwiseman reviewed Mar 2, 2020

View reviewed changes

XiongKezhi and others added 3 commits March 3, 2020 12:57

Merge branch 'master' into complete-checks-api

ce7cfc0

Add Output and more tests

768f607

Merge branch 'master' into complete-checks-api

d0b23c7

bitwiseman approved these changes Mar 4, 2020

View reviewed changes

bitwiseman merged commit c22a718 into hub4j:master Mar 4, 2020

bitwiseman mentioned this pull request Mar 4, 2020

Add support for the Checks API #520

Closed

jglick mentioned this pull request Mar 25, 2020

GHRepository.createCheckRun #753

Merged

4 tasks

XiongKezhi deleted the complete-checks-api branch March 30, 2020 16:26

Search code, repositories, users, issues, pull requests...

Complete checks api #723

Complete checks api #723

Uh oh!

Conversation

XiongKezhi commented Feb 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Before submitting a PR:

Uh oh!

Uh oh!

bitwiseman commented Feb 29, 2020

Uh oh!

bitwiseman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

XiongKezhi Mar 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bitwiseman commented Mar 1, 2020

Uh oh!

XiongKezhi commented Mar 1, 2020

Uh oh!

XiongKezhi commented Mar 1, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bitwiseman commented Mar 2, 2020

Uh oh!

Uh oh!

bitwiseman Mar 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bitwiseman left a comment

Choose a reason for hiding this comment

Uh oh!

oleg-nenashev commented Mar 19, 2020

Uh oh!

Uh oh!

XiongKezhi commented Feb 29, 2020 •

edited

Loading

XiongKezhi Mar 1, 2020 •

edited

Loading

bitwiseman Mar 2, 2020 •

edited

Loading