chore(ci): verbose and more flexible PR check (ChainSafe#1919)

.github/PULL_REQUEST/pull_request.go

@@ -1,6 +1,7 @@
 package main
 
 import (
+	"fmt"
 	"os"
 
 	"github.com/ChainSafe/gossamer/lib/utils"
@@ -11,6 +12,7 @@ func main() {
 	body := os.Getenv("RAW_BODY")
 	err := utils.CheckPRDescription(title, body)
 	if err != nil {
+		fmt.Println(err.Error())
 		os.Exit(1)
 	}
 }

.github/PULL_REQUEST_TEMPLATE.md

@@ -34,10 +34,10 @@ supported format for automatically closing issues (ie, closes #123, fixes #123)
 
 -
 
-## Primary Reviewer 
+## Primary Reviewer
 
-<!-- 
-Please indicate one of the code owners that are required to review prior to merging changes (e.g. @noot) 
+<!--
+Please indicate one of the code owners that are required to review prior to merging changes (e.g. @noot)
 -->
 
-- 
+-

lib/utils/pull_request.go

@@ -1,44 +1,61 @@
 package utils
 
 import (
+	"errors"
 	"fmt"
 	"regexp"
 	"strings"
 )
 
-const (
-	titleRegex = `[A-Za-z]+\([A-Za-z/]+\):.+[A-Za-z]+`
-	bodyRegex  = `## Changes.*- .*[A-Za-z0-9].*## Tests.*[A-Za-z].*## Issues.*- .*[A-Za-z0-9].*## Primary Reviewer.*- @.+[A-Za-z0-9].*`
+var (
+	// ErrTitlePatternNotValid indicates the title does not match the expected pattern.
+	ErrTitlePatternNotValid = errors.New("title pattern is not valid")
+	// ErrBodySectionNotFound indicates one of the required body section was not found.
+	ErrBodySectionNotFound = errors.New("body section not found")
+	// ErrBodySectionMisplaced indicates one of the required body section was misplaced in the body.
+	ErrBodySectionMisplaced = errors.New("body section misplaced")
 )
 
-// CheckPRDescription matches the PR title and body according to the PR template.
+var (
+	titleRegexp   = regexp.MustCompile(`^[A-Za-z]+\([A-Za-z/]+\):.+[A-Za-z]+$`)
+	commentRegexp = regexp.MustCompile(`<!--(.|\n)*?-->`)
+)
+
+// CheckPRDescription verifies the PR title and body match the expected format.
 func CheckPRDescription(title, body string) error {
-	match, err := regexp.MatchString(titleRegex, title)
-	if err != nil || !match {
-		return fmt.Errorf("title pattern is not valid: %w match %t", err, match)
+	if !titleRegexp.MatchString(title) {
+		return fmt.Errorf("%w: for regular expression %s: '%s'",
+			ErrTitlePatternNotValid, titleRegexp.String(), title)
 	}
 
-	var bodyData string
-	// Remove comment from PR body.
-	for {
-		start := strings.Index(body, "<!--")
-		end := strings.Index(body, "-->")
-		if start < 0 || end < 0 {
-			break
-		}
+	body = commentRegexp.ReplaceAllString(body, "")
 
-		bodyData = bodyData + body[:start]
-		body = body[end+4:]
-	}
-	bodyData = bodyData + body
+	// Required subheading sections in order
+	requiredSections := []string{"Changes", "Tests", "Issues", "Primary Reviewer"}
 
-	lineSplit := strings.Split(bodyData, "\n")
-	joinedLine := strings.Join(lineSplit, "")
+	previousIndex := -1
+	previousSection := ""
+	for i, requiredSection := range requiredSections {
+		textToFind := "## " + requiredSection
+		if i > 0 {
+			// no new line required before the first section
+			textToFind = "\n" + textToFind
+		}
+		if i < len(requiredSections)-1 {
+			// no new line required for last section
+			textToFind += "\n"
+		}
 
-	// Regex for body data
-	match, err = regexp.MatchString(bodyRegex, joinedLine)
-	if err != nil || !match {
-		return fmt.Errorf("body pattern is not valid: %w match %t", err, match)
+		index := strings.Index(body, textToFind)
+		if index == -1 {
+			return fmt.Errorf("%w: %q", ErrBodySectionNotFound, textToFind)
+		} else if i > 0 && index < previousIndex {
+			return fmt.Errorf("%w: section %q cannot be before section %q",
+				ErrBodySectionMisplaced, requiredSection, previousSection)
+		}
+		previousIndex = index
+		previousSection = requiredSection
 	}
+
 	return nil
 }

lib/utils/pull_request_test.go

@@ -1,55 +1,96 @@
 package utils
 
 import (
+	"errors"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
-func Test_PR_Checks(t *testing.T) {
-	tests := []struct {
+func Test_CheckPRDescription(t *testing.T) {
+	t.Parallel()
+
+	testCases := map[string]struct {
 		title string
 		body  string
-		valid bool
+		err   error
 	}{
-		{
-			title: "",
-			body:  "",
-			valid: false,
+		"all empty": {
+			err: errors.New("title pattern is not valid: for regular expression ^[A-Za-z]+\\([A-Za-z/]+\\):.+[A-Za-z]+$: ''"),
 		},
-		{
-			title: "abc(abc): abc",
-			body:  "",
-			valid: false,
+		"invalid title": {
+			title: "category: something",
+			err:   errors.New("title pattern is not valid: for regular expression ^[A-Za-z]+\\([A-Za-z/]+\\):.+[A-Za-z]+$: 'category: something'"),
 		},
-		{
-			title: `feat(dot/rpc): implement chain_subscribeAllHeads RPC`,
-			body:  `## Changes\n\n<!--\nPlease provide a brief but specific list of changes made, describe the changes\nin functionality rather than the changes in code.\n-->\n\n- changes for demo :123\n\n## Tests\n\n<!--\nDetails on how to run tests relevant to the changes within this pull request.\n-->\n\n- tests for demo:123{}\n\n## Issues\n\n<!--\nPlease link any issues that this pull request is related to and use the GitHub\nsupported format for automatically closing issues (ie, closes #123, fixes #123)\n-->\n\n- issues for demo:43434\n\n## Primary Reviewer\n\n<!--\nPlease indicate one of the code owners that are required to review prior to merging changes (e.g. @noot)\n-->\n\n- @noot for demo:12`,
-			valid: true,
+		"empty body only": {
+			title: "category(subcategory): something",
+			err:   errors.New("body section not found: \"## Changes\\n\""),
 		},
-		{
-			title: "abc(): abc",
-			body:  "",
-			valid: false,
+		"invalid body": {
+			title: "category(subcategory): something",
+			body:  "##Changes ## Tests ## Issues ## Primary Reviewer",
+			err:   errors.New("body section not found: \"## Changes\\n\""),
 		},
-		{
-			title: "(abc): abc",
-			body:  "",
-			valid: false,
+		"misplaced section": {
+			title: "category(subcategory): something",
+			body:  "## Changes\n## Tests\n## Primary Reviewer\n## Issues\n",
+			err:   errors.New("body section misplaced: section \"Primary Reviewer\" cannot be before section \"Issues\""),
 		},
-		{
-			title: "abc(abc):",
-			body:  "",
-			valid: false,
+		"minimal valid": {
+			title: "category(subcategory): something",
+			body:  "## Changes\n## Tests\n## Issues\n## Primary Reviewer",
+		},
+		"valid example": {
+			title: `feat(dot/rpc): implement chain_subscribeAllHeads RPC`,
+			body: `## Changes
+
+<!--
+Please provide a brief but specific list of changes made, describe the changes
+in functionality rather than the changes in code.
+-->
+
+- changes for demo :123
+
+## Tests
+
+<!--
+Details on how to run tests relevant to the changes within this pull request.
+-->
+
+- tests for demo:123{}
+
+## Issues
+
+<!--
+Please link any issues that this pull request is related to and use the GitHub
+supported format for automatically closing issues (ie, closes #123, fixes #123)
+-->
+
+- issues for demo:43434
+
+## Primary Reviewer
+
+<!--Please indicate one of the code owners that are required to review prior to merging changes (e.g. @noot)
+-->
+
+- @noot for demo:12
+`,
 		},
 	}
 
-	for _, test := range tests {
-		err := CheckPRDescription(test.title, test.body)
-		if test.valid {
-			require.NoError(t, err, "title", test.title, "body", test.body)
-		} else {
-			require.Error(t, err, "title", test.title, "body", test.body)
-		}
+	for name, testCase := range testCases {
+		testCase := testCase
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
+			err := CheckPRDescription(testCase.title, testCase.body)
+			if testCase.err != nil {
+				require.Error(t, err)
+				assert.Equal(t, testCase.err.Error(), err.Error())
+			} else {
+				assert.NoError(t, err)
+			}
+		})
 	}
 }