2479: PR marked as ready with jcheck error #1714

zhaosongzs · 2025-04-17T17:32:48Z

Currently, when determining if a PR is ready, if the PR doesn't require a review, it also ignores all jcheck failures. Instead, only reviewers check should be ignored in this case.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace

Issue

SKARA-2479: PR marked as ready with jcheck error (Bug - P4)

Reviewers

Erik Joelsson (@erikj79 - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/skara.git pull/1714/head:pull/1714
$ git checkout pull/1714

Update a local copy of the PR:
$ git checkout pull/1714
$ git pull https://git.openjdk.org/skara.git pull/1714/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1714

View PR using the GUI difftool:
$ git pr show -t 1714

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/skara/pull/1714.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-04-17T17:33:03Z

👋 Welcome back zsong! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-04-17T17:33:12Z

@zhaosongzs This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

🔍 One or more changes in this pull request modifies files in areas of the source code that often require two reviewers. Please consider if this is the case for this pull request, and if so, await a second reviewer to approve this pull request before you integrate it.

After integration, the commit message for the final commit will be:

2479: PR marked as ready with jcheck error

Reviewed-by: erikj

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

mlbridge · 2025-04-17T17:39:09Z

Webrevs

erikj79 · 2025-04-17T17:51:39Z

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java

+            var onlyReviewersCheckFailed = visitor.errorFailedChecksMessages().stream()
+                    .allMatch(message -> message.contains("Too few reviewers"));


This looks awkward. Can we extend the Visitor with an explicit method for this instead? The role of the Visitor implementation is to gather the jcheck errors and present them in a way that makes sense to the caller, in this case the pr bot classes.

erikj79 · 2025-04-17T19:40:51Z

bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestCheckIssueVisitor.java

@@ -72,6 +72,14 @@ List<String> errorFailedChecksMessages() {
        return errorFailedChecks.values().stream().flatMap(List::stream).toList();
    }

+    List<String> errorFailedCheckMessagesWithoutReviewersCheck() {


This is a mouthful. I was thinking something like boolean tooFewReviewers().

Do you mean that tooFewReviewers() should return true when there is only ReviewersCheck Failure?

Or does tooFewReviewers() return true when there is a ReviewersCheck Failure in the list?
If so, the logic should be like this
var readyToPostApprovalNeededComment = readyForReview && ((!reviewNeeded && visitor.errorFailedChecksMessages().size() == 1 && visitor.tooFewReviewers()) || visitor.errorFailedChecksMessages().isEmpty()) && integrationBlockers.isEmpty() && !statusMessage.contains(TEMPORARY_ISSUE_FAILURE_MARKER);

Right, I didn't think that through and this gets messy and I think it's because we aren't using the right abstraction. Can we just push something like this down to the visitor:

boolean hasErrors(boolean reviewNeeded)

Yes, sounds good

erikj79 · 2025-04-23T15:30:41Z

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java

@@ -1470,7 +1470,7 @@ private void checkStatus() {
            var commitMessage = String.join("\n", commit.message());

            var readyToPostApprovalNeededComment = readyForReview &&
-                    (!reviewNeeded || visitor.errorFailedChecksMessages().isEmpty()) &&
+                    visitor.hasErrors(reviewNeeded) &&


Shouldn't this be negated?

Thanks for catching it. You are right. I remember I added "!" but Somehow it disappeared...

zhaosongzs · 2025-04-23T20:32:16Z

Thanks for the review!
/integrate

openjdk · 2025-04-23T20:32:37Z

Going to push as commit 1107983.

openjdk · 2025-04-23T20:32:42Z

@zhaosongzs Pushed as commit 1107983.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

zhaosongzs added 2 commits April 17, 2025 09:55

SKARA-2479

51f1471

update

36b92f0

add test

46b17c1

zhaosongzs changed the title ~~Skara 2479~~ 2479 Apr 17, 2025

openjdk bot changed the title ~~2479~~ 2479: PR marked as ready with jcheck error Apr 17, 2025

zhaosongzs marked this pull request as ready for review April 17, 2025 17:34

openjdk bot added the rfr label Apr 17, 2025

erikj79 reviewed Apr 17, 2025

View reviewed changes

review comment

18043ee

erikj79 reviewed Apr 17, 2025

View reviewed changes

Merge branch 'master' into SKARA-2479

536acab

zhaosongzs marked this pull request as draft April 17, 2025 20:45

review comment

28f368b

openjdk bot removed the rfr label Apr 17, 2025

zhaosongzs marked this pull request as ready for review April 17, 2025 21:37

openjdk bot added the rfr label Apr 17, 2025

Empty commit

2ef318d

erikj79 reviewed Apr 23, 2025

View reviewed changes

review comment

041b332

erikj79 approved these changes Apr 23, 2025

View reviewed changes

openjdk bot added the ready label Apr 23, 2025

openjdk bot added the integrated label Apr 23, 2025

openjdk bot closed this Apr 23, 2025

openjdk bot removed ready rfr labels Apr 23, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

2479: PR marked as ready with jcheck error #1714

2479: PR marked as ready with jcheck error #1714

Uh oh!

zhaosongzs commented Apr 17, 2025 •

edited by openjdk bot

Loading

Uh oh!

bridgekeeper bot commented Apr 17, 2025

Uh oh!

openjdk bot commented Apr 17, 2025 •

edited

Loading

Uh oh!

mlbridge bot commented Apr 17, 2025 •

edited

Loading

Uh oh!

erikj79 Apr 17, 2025

Uh oh!

erikj79 Apr 17, 2025

Uh oh!

zhaosongzs Apr 17, 2025

Uh oh!

zhaosongzs Apr 17, 2025

Uh oh!

erikj79 Apr 17, 2025

Uh oh!

zhaosongzs Apr 17, 2025

Uh oh!

erikj79 Apr 23, 2025

Uh oh!

zhaosongzs Apr 23, 2025

Uh oh!

zhaosongzs commented Apr 23, 2025

Uh oh!

openjdk bot commented Apr 23, 2025

Uh oh!

openjdk bot commented Apr 23, 2025

Uh oh!

Uh oh!

		var onlyReviewersCheckFailed = visitor.errorFailedChecksMessages().stream()
		.allMatch(message -> message.contains("Too few reviewers"));

2479: PR marked as ready with jcheck error #1714

2479: PR marked as ready with jcheck error #1714

Uh oh!

Conversation

zhaosongzs commented Apr 17, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Apr 17, 2025

Uh oh!

openjdk bot commented Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

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!

zhaosongzs commented Apr 23, 2025

Uh oh!

openjdk bot commented Apr 23, 2025

Uh oh!

openjdk bot commented Apr 23, 2025

Uh oh!

Uh oh!

zhaosongzs commented Apr 17, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Apr 17, 2025 •

edited

Loading

mlbridge bot commented Apr 17, 2025 •

edited

Loading