Automatically generate JIRA issue links to Pull Request description #108

The-Huginn · 2023-07-28T12:58:00Z

Example can be found here: The-Huginn#19

Edit: Resolves Automatically add WFLY JIRA issues links #72

The-Huginn · 2023-07-28T13:01:17Z

Please review @xstefank

xstefank · 2023-07-31T08:59:45Z

src/main/java/io/xstefank/wildfly/bot/PullRequestFormatProcessor.java

+                .append("\n");
+
+        for (String jira : jiraIssues) {
+            sb.appendQuote(String.format(RuntimeConstants.BOT_JIRA_LINK_TEMPLATE, jira, jira))


\n can go into format template

xstefank · 2023-07-31T09:01:06Z

src/main/java/io/xstefank/wildfly/bot/PullRequestFormatProcessor.java

+        }
+    }
+
+    private Set<String> collectJiraIssues(GHPullRequest pullRequest, Pattern jiraPattern) {


I would recommend parseJiraIssues

xstefank · 2023-07-31T09:01:14Z

src/main/java/io/xstefank/wildfly/bot/PullRequestFormatProcessor.java

+    }
+
+    private Set<String> collectJiraIssues(GHPullRequest pullRequest, Pattern jiraPattern) {
+        Set<String> jiras = retrieveJirasFromString(pullRequest.getTitle(), jiraPattern);


similarly parse

xstefank · 2023-07-31T09:03:15Z

src/main/java/io/xstefank/wildfly/bot/util/CustomStringBuilder.java

@@ -0,0 +1,23 @@
+package io.xstefank.wildfly.bot.util;
+
+public class CustomStringBuilder {


you can find a better name

I also wonder if the appendQuote is worth a new class

xstefank · 2023-07-31T09:05:32Z

src/test/java/io/xstefank/wildfly/bot/PROpenedTest.java

@@ -298,7 +296,6 @@ void testDirectoriesMentionsNewFileInDiff() throws IOException {
                Mockito.verify(mockedPR, Mockito.times(2)).listFiles();
                Mockito.verify(mockedPR).comment("/cc @7125767235");
                Mockito.verify(mockedPR, Mockito.times(1)).listComments();
-                Mockito.verifyNoMoreInteractions(mockedPR);


wouldn't it be better to list individual invocations since we test them all here?

There would be quite a lot of them and it is possible, that it will grow in the future as well. I think at some point point we will no longer know, where all those invocations were called and adjust the tests to failures.

xstefank · 2023-07-31T09:06:54Z

src/test/java/io/xstefank/wildfly/bot/helper/MockedGHPullRequestProcessor.java

+        GHPullRequestCommitDetail.Commit mockCommit = Mockito.mock(GHPullRequestCommitDetail.Commit.class);
+        Mockito.when(mockCommitDetail.getCommit()).thenReturn(mockCommit);
+        for (String commitMessage : commitMessages) {
+            Mockito.when(mockCommit.getMessage()).thenReturn(commitMessage);


isn't this overriding one single commit?

I do not think so. At least in chained calls such as Mockito.when(...).thenReturn("1").thenReturn("2") will return at first 1 and on second invocation 2.

The-Huginn

Pushing latest updates

The-Huginn · 2023-07-31T10:07:14Z

src/test/java/io/xstefank/wildfly/bot/PROpenedTest.java

@@ -298,7 +296,6 @@ void testDirectoriesMentionsNewFileInDiff() throws IOException {
                Mockito.verify(mockedPR, Mockito.times(2)).listFiles();
                Mockito.verify(mockedPR).comment("/cc @7125767235");
                Mockito.verify(mockedPR, Mockito.times(1)).listComments();
-                Mockito.verifyNoMoreInteractions(mockedPR);


There would be quite a lot of them and it is possible, that it will grow in the future as well. I think at some point point we will no longer know, where all those invocations were called and adjust the tests to failures.

The-Huginn · 2023-07-31T10:08:48Z

src/test/java/io/xstefank/wildfly/bot/helper/MockedGHPullRequestProcessor.java

+        GHPullRequestCommitDetail.Commit mockCommit = Mockito.mock(GHPullRequestCommitDetail.Commit.class);
+        Mockito.when(mockCommitDetail.getCommit()).thenReturn(mockCommit);
+        for (String commitMessage : commitMessages) {
+            Mockito.when(mockCommit.getMessage()).thenReturn(commitMessage);


I do not think so. At least in chained calls such as Mockito.when(...).thenReturn("1").thenReturn("2") will return at first 1 and on second invocation 2.

xstefank · 2023-08-01T06:12:25Z

src/main/java/io/xstefank/wildfly/bot/PullRequestFormatProcessor.java

@@ -75,6 +84,63 @@ void onPullRequestEdited(@PullRequest.Edited @PullRequest.Opened @PullRequest.Sy

    }

+    /**
+     * Originally from https://github.com/hibernate/hibernate-github-bot
+     */


you can probably drop this as it doesn't add value

xstefank · 2023-08-01T08:08:02Z

src/main/java/io/xstefank/wildfly/bot/model/WildFlyConfigFile.java


        public List<WildFlyRule> rules;

        public Format format = new Format();

        public void setProjectKey(String name) {
-            projectPattern = Pattern.compile(String.format("\\[%s-\\d+]\\s+.*|%s-\\d+\\s+.*", name, name));
+            issuePattern = Pattern.compile(String.format("%s-\\d+", name));
+            checkPattern = Pattern.compile(String.format("(\\[%1$s-\\d+(,%1$s-\\d+)*\\]|%1$s-\\d+(,%1$s-\\d+)*)(?!%1$s-\\d+)", name));


regexes should be also constants if they are the same, easy to make mistakes

xstefank · 2023-08-01T08:09:37Z

src/main/java/io/xstefank/wildfly/bot/util/DescriptionStringBuilder.java

+        return this;
+    }
+
+    public DescriptionStringBuilder appendQuote(String message) {


this is not a Quote and I still don't understand why we need a separate class for this override

I will rename it to appendBlockQuote, as that is what it is called in markdown. which we are using it's syntax for writing comments / editing descriptions. Also it might be easier for future expansion. Currently it is also used in tests and makes the, more readable.

xstefank · 2023-08-01T08:11:31Z

src/test/java/io/xstefank/wildfly/bot/PRAppendingMessage.java

+
+    private String appendedMessage = String.format("""
+            %s\n
+            > %s%s"""  + RuntimeConstants.BOT_MESSAGE_FOOTER, RuntimeConstants.BOT_MESSAGE_HEADER, RuntimeConstants.BOT_JIRA_LINKS_HEADER, "%s");


what is the %s argument at the end?

Otherwise we lose the %s, which is used again. This appendMessage is further used for String.format and the %s contains the body for individual tests.

xstefank · 2023-08-01T08:16:33Z

src/test/java/io/xstefank/wildfly/bot/PRAppendingMessage.java

+    public void testNonEmptyBodyAppendMessage() throws IOException {
+        String body = """
+                This is my
+                testing\n


just why do you use \n in multiline string? :)

when I replace \n with just an enter key hit and try to compile the wildfly-checkstyle plugin complains

xstefank · 2023-08-01T08:20:21Z

src/test/java/io/xstefank/wildfly/bot/PRFormatOverrideMessage.java

                    Mockito.when(mockCommit.getMessage()).thenReturn(commitMessage);

-                    processEmptyPullRequestMock(mocks.pullRequest(1352150111));
+                    // We still need to manually mock this commit, as we retrieve the sha as well
+                    processPullRequestMock(mocks.pullRequest(1352150111), mockEmptyFileDetails(), mockEmptyComments(), mockCommitDetails);


you have helper methods for commits. Also the comment seems unnecessary

There is a comment, why it is needed

xstefank · 2023-08-01T08:22:33Z

src/test/java/io/xstefank/wildfly/bot/PRTitleCheckTest.java

+
+        GitHubAppTesting.given()
+                .github(mocks -> {
+                    mocks.configFileFromString("wildfly-bot.yml", wildflyConfigFile);


config name should be pulled from constants also in tests

Sure, it's just most of the test are not doing it anyways.

xstefank · 2023-08-01T08:24:23Z

src/test/java/io/xstefank/wildfly/bot/PRTitleCheckTest.java

+    }
+
+    @Test
+    void incorrectTitleCheckFailMultipleJirasBracketsSecondTest() throws IOException {


? same as previous one?

The-Huginn · 2023-08-01T09:30:42Z

I will also update other tests

xstefank · 2023-08-02T06:52:19Z

src/main/java/io/xstefank/wildfly/bot/PullRequestFormatProcessor.java

+
+        sb.append(RuntimeConstants.BOT_MESSAGE_HEADER)
+                .append("\n\n")
+                .append(blockQuoted(RuntimeConstants.BOT_JIRA_LINKS_HEADER));


header should also be "> " ?

I wanted it to be the only one with normal font to explain, what is below in blockquotes. Just as some kind of a title.

xstefank · 2023-08-02T07:48:32Z

src/main/java/io/xstefank/wildfly/bot/model/RuntimeConstants.java

+    public static final String CHECK_PATTERN = "\\[%1$s-\\d+(,%1$s-\\d+)*\\]|%1$s-\\d+(,%1$s-\\d+)*";
+
+    public static final String BOT_MESSAGE_HEADER = """
+            ____


This is not necessary if you have the next line.

This one adds a small little deliminer between the section automatically generated. A thin grey line.

xstefank · 2023-08-02T07:50:27Z

src/test/java/io/xstefank/wildfly/bot/PRAppendingMessage.java

+    private String appendedMessage = String.format("""
+            %s
+
+            %s%%s%s""", RuntimeConstants.BOT_MESSAGE_HEADER, blockQuoted(RuntimeConstants.BOT_JIRA_LINKS_HEADER), RuntimeConstants.BOT_MESSAGE_FOOTER);


out of fun we should write this as "%s\n\n%s%%s%s". (Joke :))

xstefank · 2023-08-02T07:52:03Z

src/test/java/io/xstefank/wildfly/bot/PRAppendingMessage.java

+        }).when().payloadFromString(githubJson.jsonString()).event(GHEvent.PULL_REQUEST).then().github(mocks -> {
+            StringBuilder sb = new StringBuilder();
+            Mockito.verify(mocks.pullRequest(githubJson.pullRequest())).setBody(String.format(body + "\n\n" + appendedMessage,
+                    sb.append(blockQuoted(String.format(RuntimeConstants.BOT_JIRA_LINK_TEMPLATE, "WFLY-00000")).toString())));


why do you need SB here?

to be consistent with all the tests, rather then save on this one

xstefank · 2023-08-02T07:52:56Z

src/test/java/io/xstefank/wildfly/bot/PRAppendingMessage.java

+            StringBuilder sb = new StringBuilder();
+            Mockito.verify(mocks.pullRequest(githubJson.pullRequest())).setBody(String.format(appendedMessage,
+                    sb.append(blockQuoted(String.format(RuntimeConstants.BOT_JIRA_LINK_TEMPLATE, "WFLY-00001")))
+                            .append(blockQuoted(String.format(RuntimeConstants.BOT_JIRA_LINK_TEMPLATE, "WFLY-00002")).toString())));


here it makes sense

The-Huginn · 2023-08-03T15:56:49Z

We should now link only jiras, which are not in the description and omit updating description fully if all jiras are linked

…ody.

The-Huginn force-pushed the add-jira-links branch from ff3fb9a to 374a25d Compare July 31, 2023 08:13

xstefank requested changes Jul 31, 2023

View reviewed changes

The-Huginn commented Jul 31, 2023

View reviewed changes

The-Huginn force-pushed the add-jira-links branch from 374a25d to c813b23 Compare July 31, 2023 11:45

xstefank requested changes Aug 1, 2023

View reviewed changes

The-Huginn force-pushed the add-jira-links branch 4 times, most recently from 0be76be to c6880f5 Compare August 2, 2023 07:57

xstefank requested changes Aug 2, 2023

View reviewed changes

The-Huginn force-pushed the add-jira-links branch 2 times, most recently from 9077033 to c32e4b0 Compare August 3, 2023 15:55

The-Huginn force-pushed the add-jira-links branch 2 times, most recently from 0562db3 to 229b387 Compare August 10, 2023 14:51

xstefank added the hold label Aug 11, 2023

The-Huginn force-pushed the add-jira-links branch from 229b387 to f4c28ea Compare August 15, 2023 15:08

The-Huginn force-pushed the add-jira-links branch from f4c28ea to e229ecd Compare September 14, 2023 11:21

The-Huginn force-pushed the add-jira-links branch from e229ecd to e2ba997 Compare November 22, 2023 11:48

The-Huginn requested a review from xstefank November 22, 2023 13:18

Automatically generate JIRA issue links to Pull Request description b…

632f08b

…ody.

The-Huginn force-pushed the add-jira-links branch from e2ba997 to 632f08b Compare January 15, 2024 09:51

xstefank approved these changes Jan 26, 2024

View reviewed changes

xstefank merged commit 73c1c86 into wildfly:main Jan 26, 2024
1 check passed

xstefank removed the hold label Jan 26, 2024

		@@ -0,0 +1,23 @@
		package io.xstefank.wildfly.bot.util;

		public class CustomStringBuilder {

Automatically generate JIRA issue links to Pull Request description #108

Automatically generate JIRA issue links to Pull Request description #108

Conversation

The-Huginn commented Jul 28, 2023 • edited Loading

The-Huginn commented Jul 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

The-Huginn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

The-Huginn commented Aug 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

The-Huginn commented Aug 3, 2023

The-Huginn commented Jul 28, 2023 •

edited

Loading