RFR: 394: Merge bot conflict resolution interaction requires separate credentials

[Jorn Vernee]

On Fri, 8 May 2020 14:32:57 GMT, Robin Westberg <rwestberg at openjdk.org> wrote:

> Hi all,
> 
> Please review this change that allows the pull request bot to recognize specially formatted command comments made by
> "itself" (most likely another bot using the same credentials).
> Best regards,
> Robin

Looks good. Some minor comments left inline. The one about the test bot running early in the test might be the only
problematic thing (but I don't think it matters in practice).

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandWorkItem.java line 42:

> 41:     private final Pattern commandReplyPattern = Pattern.compile("<!-- Jmerge command reply message
> \\((\\S+)\\) -->"); 42:     private final String selfCommandMarker = "<!-- Valid self-command -->";
> 43:

FWIW, these fields could also be made `static` as well.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandWorkItem.java line 91:

> 90:                        .map(comment -> new AbstractMap.SimpleEntry<>(comment,
> commandPattern.matcher(comment.body()))) 91:                        .filter(entry ->
> !entry.getKey().author().equals(self) || entry.getKey().body().endsWith(selfCommandMarker)) 92:
> .filter(entry -> entry.getValue().find())

Could you move this above the `.map(...)` call, so that you can replace `entry.getKey()` with `comment`? (just like
before)

bots/pr/src/test/java/org/openjdk/skara/bots/pr/CommandTests.java line 137:

> 136:             TestBotRunner.runPeriodicItems(mergeBot);
> 137:             assertEquals(1, pr.comments().size());
> 138:

Should to `runPeriodicItems` call right after adding the "/help" comment be there? Asking because it looks like you
want to check the number of comments before and after running the bot below that (but at that point the bot has already
run).

bots/pr/src/test/java/org/openjdk/skara/bots/pr/CommandTests.java line 149:

> 148:                          .filter(comment -> comment.body().contains("help"))
> 149:                           .count();
> 150:             assertEquals(1, help);

Spurious whitespace on last line
Suggestion:

            var help = pr.comments().stream()
                         .filter(comment -> comment.body().contains("Available commands"))
                         .filter(comment -> comment.body().contains("help"))
                         .count();

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

Marked as reviewed by jvernee (Reviewer).

PR: https://git.openjdk.java.net/skara/pull/618