RFR: 2454: Skara mistakenly marked a non-clean backport as clean [v2]

[Erik Joelsson]

On Fri, 7 Mar 2025 22:39:22 GMT, Zhao Song <zsong at openjdk.org> wrote:

>> A user reported that Skara bot mistakenly marked a non-clean backport as clean. After investigation, I found that the PR was too large for the remote host to return the entire diff. The PR contained 8082 changed files, but when Skara bot requested the diff, only 3000 files were returned. Therefore, Skara bot was unable to compare some patches, so mistakenly mark the pr as clean.
>> 
>> In this patch, Skara bot will now check if the PR is too large to evaluate. If it is, Skara bot will not evaluate if this PR is clean and will add a warning comment to the PR instead.
>
> Zhao Song has updated the pull request incrementally with one additional commit since the last revision:
> 
>   rename method

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java line 1280:

> 1278:         var message = "⚠️  @" + pr.author().username() +
> 1279:                 " This backport pull request is too large for skara bot to get the entire diff from the remote repository. " +
> 1280:                 "Therefore, skara bot can't evaluate whether this backport is clean or not." +

The current language style used by the bot does not have it refer to itself as "skara bot".
Suggestion:

                " This backport pull request is too large to be automatically evaluated as clean." +

forge/src/main/java/org/openjdk/skara/forge/github/GitHubPullRequest.java line 48:

> 46: 
> 47:     private List<Label> labels = null;
> 48:     private Optional<Boolean> diffLimited = Optional.empty();

We should not use `Optional` in fields. If we need a tri-state for this boolean, then it's better to use `Boolean` and check for null.

forge/src/main/java/org/openjdk/skara/forge/github/GitHubPullRequest.java line 837:

> 835:         }
> 836:         return diffLimited.get();
> 837:     }

I think it would be better to add a boolean field `complete` to `Diff`, instead of having this be a property of a PullRequest. Then we avoid this awkward requirement for ordering calls on PullRequest.

test/src/main/java/org/openjdk/skara/test/TestPullRequest.java line 357:

> 355:     public boolean diffLimited() {
> 356:         return false;
> 357:     }

You could write an automatic test for PullRequestBot if you give the test the ability to control if the diff returned from TestPullRequest is limited or not.

-------------

PR Review Comment: https://git.openjdk.org/skara/pull/1708#discussion_r1987378357
PR Review Comment: https://git.openjdk.org/skara/pull/1708#discussion_r1987350620
PR Review Comment: https://git.openjdk.org/skara/pull/1708#discussion_r1987362236
PR Review Comment: https://git.openjdk.org/skara/pull/1708#discussion_r1987368500