From 274eecd3cafd6cfaf58edadf0787eebad24c0acc Mon Sep 17 00:00:00 2001 From: Quentin McGaw Date: Wed, 27 Oct 2021 14:55:10 -0400 Subject: [PATCH] chore(ci): verbose and more flexible PR check (#1919) --- .github/PULL_REQUEST/pull_request.go | 2 + .github/PULL_REQUEST_TEMPLATE.md | 8 +- lib/utils/pull_request.go | 67 ++++++++++------ lib/utils/pull_request_test.go | 109 ++++++++++++++++++--------- 4 files changed, 123 insertions(+), 63 deletions(-) diff --git a/.github/PULL_REQUEST/pull_request.go b/.github/PULL_REQUEST/pull_request.go index 9b1d69c124..2da6c94514 100644 --- a/.github/PULL_REQUEST/pull_request.go +++ b/.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) } } diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 62573c6f71..898685a80d 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -34,10 +34,10 @@ supported format for automatically closing issues (ie, closes #123, fixes #123) - -## Primary Reviewer +## Primary Reviewer - -- +- diff --git a/lib/utils/pull_request.go b/lib/utils/pull_request.go index f14f81c988..2a285319bf 100644 --- a/lib/utils/pull_request.go +++ b/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(``) +) + +// 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, "") - 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 } diff --git a/lib/utils/pull_request_test.go b/lib/utils/pull_request_test.go index 8cd7cf4cc5..8f3cba4cc3 100644 --- a/lib/utils/pull_request_test.go +++ b/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\n\n- changes for demo :123\n\n## Tests\n\n\n\n- tests for demo:123{}\n\n## Issues\n\n\n\n- issues for demo:43434\n\n## Primary Reviewer\n\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 + + + +- changes for demo :123 + +## Tests + + + +- tests for demo:123{} + +## Issues + + + +- issues for demo:43434 + +## Primary Reviewer + + + +- @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) + } + }) } }