From cd902689ab6d05cc0804f81cfe993a79907dd648 Mon Sep 17 00:00:00 2001 From: Chi Zhang Date: Mon, 7 Oct 2019 18:12:35 -0700 Subject: [PATCH] Improve Github alerter for Mako (#748) * update mako alerter * revert a small change * address comments * revert all %w to %v --- test/mako/alerter/alerter.go | 3 +- test/mako/alerter/github/issue.go | 237 +++++++++++++++---------- test/mako/alerter/github/issue_test.go | 15 +- 3 files changed, 149 insertions(+), 106 deletions(-) diff --git a/test/mako/alerter/alerter.go b/test/mako/alerter/alerter.go index 41e358a29..56eb37aac 100644 --- a/test/mako/alerter/alerter.go +++ b/test/mako/alerter/alerter.go @@ -17,6 +17,7 @@ limitations under the License. package alerter import ( + "fmt" "log" qpb "github.com/google/mako/proto/quickstore/quickstore_go_proto" @@ -55,7 +56,7 @@ func (alerter *Alerter) HandleBenchmarkResult(testName string, output qpb.Quicks if err != nil { if output.GetStatus() == qpb.QuickstoreOutput_ANALYSIS_FAIL { var errs []error - summary := output.GetSummaryOutput() + summary := fmt.Sprintf("%s\n\nSee run chart at: %s", output.GetSummaryOutput(), output.GetRunChartLink()) if alerter.githubIssueHandler != nil { if err := alerter.githubIssueHandler.CreateIssueForTest(testName, summary); err != nil { errs = append(errs, err) diff --git a/test/mako/alerter/github/issue.go b/test/mako/alerter/github/issue.go index 29d043981..aa11a3ea5 100644 --- a/test/mako/alerter/github/issue.go +++ b/test/mako/alerter/github/issue.go @@ -38,10 +38,11 @@ const ( // issueBodyTemplate is a template for issue body issueBodyTemplate = ` ### Auto-generated issue tracking performance regression -* **Test name**: %s` +* **Test name**: %s +* **Repository name**: %s` - // newIssueCommentTemplate is a template for the comment of an issue that has been quiet for a long time - newIssueCommentTemplate = ` + // issueSummaryCommentTemplate is a template for the summary of an issue + issueSummaryCommentTemplate = ` A new regression for this test has been detected: %s` @@ -50,9 +51,9 @@ A new regression for this test has been detected: New regression has been detected, reopening this issue: %s` - // closeIssueComment is the comment of an issue when we close it + // closeIssueComment is the comment of an issue when it is closed closeIssueComment = ` -The performance regression goes way for this test, closing this issue.` +The performance regression goes away for this test, closing this issue.` ) // IssueHandler handles methods for github issues @@ -87,65 +88,75 @@ func Setup(org, repo, githubTokenPath string, dryrun bool) (*IssueHandler, error // CreateIssueForTest will try to add an issue with the given testName and description. // If there is already an issue related to the test, it will try to update that issue. func (gih *IssueHandler) CreateIssueForTest(testName, desc string) error { - org := gih.config.org - repo := gih.config.repo - dryrun := gih.config.dryrun title := fmt.Sprintf(issueTitleTemplate, testName) - issue := gih.findIssue(org, repo, title, dryrun) + issue, err := gih.findIssue(title) + if err != nil { + return fmt.Errorf("failed to find issues for test %q: %v, skipped creating new issue", testName, err) + } // If the issue hasn't been created, create one if issue == nil { - body := fmt.Sprintf(issueBodyTemplate, testName) - issue, err := gih.createNewIssue(org, repo, title, body, dryrun) + commentBody := fmt.Sprintf(issueBodyTemplate, testName, gih.config.repo) + issue, err := gih.createNewIssue(title, commentBody) if err != nil { - return err + return fmt.Errorf("failed to create a new issue for test %q: %v", testName, err) } - comment := fmt.Sprintf(newIssueCommentTemplate, desc) - if err := gih.addComment(org, repo, *issue.Number, comment, dryrun); err != nil { - return err + commentBody = fmt.Sprintf(issueSummaryCommentTemplate, desc) + if err := gih.addComment(*issue.Number, commentBody); err != nil { + return fmt.Errorf("failed to add comment for new issue %d: %v", *issue.Number, err) } - // If one issue with the same title has been closed, reopen it and add new comment - } else if *issue.State == string(ghutil.IssueCloseState) { - if err := gih.reopenIssue(org, repo, *issue.Number, dryrun); err != nil { - return err + return nil + } + + // If the issue has been created, edit it + issueNumber := *issue.Number + + // If the issue has been closed, reopen it + if *issue.State == string(ghutil.IssueCloseState) { + if err := gih.reopenIssue(issueNumber); err != nil { + return fmt.Errorf("failed to reopen issue %d: %v", issueNumber, err) } - comment := fmt.Sprintf(reopenIssueCommentTemplate, desc) - if err := gih.addComment(org, repo, *issue.Number, comment, dryrun); err != nil { - return err - } - } else { - // If the issue hasn't been updated for a long time, add a new comment - if time.Now().Sub(*issue.UpdatedAt) > daysConsideredOld*24*time.Hour { - comment := fmt.Sprintf(newIssueCommentTemplate, desc) - // TODO(Fredy-Z): edit the old comment instead of adding a new one, like flaky-test-reporter - if err := gih.addComment(org, repo, *issue.Number, comment, dryrun); err != nil { - return err - } + commentBody := fmt.Sprintf(reopenIssueCommentTemplate, desc) + if err := gih.addComment(issueNumber, commentBody); err != nil { + return fmt.Errorf("failed to add comment for reopened issue %d: %v", issueNumber, err) } } + // Edit the old comment + comments, err := gih.getComments(issueNumber) + if err != nil { + return fmt.Errorf("failed to get comments from issue %d: %v", issueNumber, err) + } + if len(comments) < 2 { + return fmt.Errorf("existing issue %d is malformed, cannot update", issueNumber) + } + commentBody := fmt.Sprintf(issueSummaryCommentTemplate, desc) + if err := gih.editComment(issueNumber, *comments[1].ID, commentBody); err != nil { + return fmt.Errorf("failed to edit the comment for issue %d: %v", issueNumber, err) + } + return nil } // createNewIssue will create a new issue, and add perfLabel for it. -func (gih *IssueHandler) createNewIssue(org, repo, title, body string, dryrun bool) (*github.Issue, error) { +func (gih *IssueHandler) createNewIssue(title, body string) (*github.Issue, error) { var newIssue *github.Issue if err := helpers.Run( - "creating issue", + fmt.Sprintf("creating issue %q in %q", title, gih.config.repo), func() error { var err error - newIssue, err = gih.client.CreateIssue(org, repo, title, body) + newIssue, err = gih.client.CreateIssue(gih.config.org, gih.config.repo, title, body) return err }, - dryrun, + gih.config.dryrun, ); nil != err { return nil, err } if err := helpers.Run( - "adding perf label", + fmt.Sprintf("adding perf label for issue %q in %q", title, gih.config.repo), func() error { - return gih.client.AddLabelsToIssue(org, repo, *newIssue.Number, []string{perfLabel}) + return gih.client.AddLabelsToIssue(gih.config.org, gih.config.repo, *newIssue.Number, []string{perfLabel}) }, - dryrun, + gih.config.dryrun, ); nil != err { return nil, err } @@ -155,74 +166,112 @@ func (gih *IssueHandler) createNewIssue(org, repo, title, body string, dryrun bo // CloseIssueForTest will try to close the issue for the given testName. // If there is no issue related to the test or the issue is already closed, the function will do nothing. func (gih *IssueHandler) CloseIssueForTest(testName string) error { - org := gih.config.org - repo := gih.config.repo - dryrun := gih.config.dryrun title := fmt.Sprintf(issueTitleTemplate, testName) - issue := gih.findIssue(org, repo, title, dryrun) - if issue == nil || *issue.State == string(ghutil.IssueCloseState) { + issue, err := gih.findIssue(title) + // If no issue has been found, or the issue has already been closed, do nothing. + if issue == nil || err != nil || *issue.State == string(ghutil.IssueCloseState) { return nil } issueNumber := *issue.Number - if err := helpers.Run( - "add comment for the issue to close", - func() error { - _, cErr := gih.client.CreateComment(org, repo, issueNumber, closeIssueComment) - return cErr - }, - dryrun, - ); err != nil { - return err + if err := gih.addComment(issueNumber, closeIssueComment); err != nil { + return fmt.Errorf("failed to add comment for the issue %d to close: %v", issueNumber, err) } - return helpers.Run( - "closing issue", - func() error { - return gih.client.CloseIssue(org, repo, issueNumber) - }, - dryrun, - ) -} - -// reopenIssue will reopen the given issue. -func (gih *IssueHandler) reopenIssue(org, repo string, issueNumber int, dryrun bool) error { - return helpers.Run( - "reopen the issue", - func() error { - return gih.client.ReopenIssue(org, repo, issueNumber) - }, - dryrun, - ) -} - -// findIssue will return the issue in the given repo if it exists. -func (gih *IssueHandler) findIssue(org, repo, title string, dryrun bool) *github.Issue { - var issues []*github.Issue - helpers.Run( - "list issues in the repo", - func() error { - var err error - issues, err = gih.client.ListIssuesByRepo(org, repo, []string{perfLabel}) - return err - }, - dryrun, - ) - for _, issue := range issues { - if *issue.Title == title { - return issue - } + if err := gih.closeIssue(issueNumber); err != nil { + return fmt.Errorf("failed to close the issue %d: %v", issueNumber, err) } return nil } -// addComment will add comment for the given issue. -func (gih *IssueHandler) addComment(org, repo string, issueNumber int, commentBody string, dryrun bool) error { +// reopenIssue will reopen the given issue. +func (gih *IssueHandler) reopenIssue(issueNumber int) error { return helpers.Run( - "add comment for issue", + fmt.Sprintf("reopening issue %d in %q", issueNumber, gih.config.repo), func() error { - _, err := gih.client.CreateComment(org, repo, issueNumber, commentBody) - return err + return gih.client.ReopenIssue(gih.config.org, gih.config.repo, issueNumber) }, - dryrun, + gih.config.dryrun, + ) +} + +// closeIssue will close the given issue. +func (gih *IssueHandler) closeIssue(issueNumber int) error { + return helpers.Run( + fmt.Sprintf("closing issue %d in %q", issueNumber, gih.config.repo), + func() error { + return gih.client.CloseIssue(gih.config.org, gih.config.repo, issueNumber) + }, + gih.config.dryrun, + ) +} + +// findIssue will return the issue in the given repo if it exists. +func (gih *IssueHandler) findIssue(title string) (*github.Issue, error) { + var issues []*github.Issue + if err := helpers.Run( + fmt.Sprintf("listing issues in %q", gih.config.repo), + func() error { + var err error + issues, err = gih.client.ListIssuesByRepo(gih.config.org, gih.config.repo, []string{perfLabel}) + return err + }, + gih.config.dryrun, + ); err != nil { + return nil, err + } + var existingIssue *github.Issue + for _, issue := range issues { + if *issue.Title == title { + // If the issue has been closed a long time ago, ignore this issue. + if issue.GetState() == string(ghutil.IssueCloseState) && + time.Now().Sub(*issue.UpdatedAt) > daysConsideredOld*24*time.Hour { + continue + } + + existingIssue = issue + break + } + } + + return existingIssue, nil +} + +// getComments will get comments for the given issue. +func (gih *IssueHandler) getComments(issueNumber int) ([]*github.IssueComment, error) { + var comments []*github.IssueComment + if err := helpers.Run( + fmt.Sprintf("getting comments for issue %d in %q", issueNumber, gih.config.repo), + func() error { + var err error + comments, err = gih.client.ListComments(gih.config.org, gih.config.repo, issueNumber) + return err + }, + gih.config.dryrun, + ); err != nil { + return comments, err + } + return comments, nil +} + +// addComment will add comment for the given issue. +func (gih *IssueHandler) addComment(issueNumber int, commentBody string) error { + return helpers.Run( + fmt.Sprintf("adding comment %q for issue %d in %q", commentBody, issueNumber, gih.config.repo), + func() error { + _, err := gih.client.CreateComment(gih.config.org, gih.config.repo, issueNumber, commentBody) + return err + }, + gih.config.dryrun, + ) +} + +// editComment will edit the comment to the new body. +func (gih *IssueHandler) editComment(issueNumber int, commentID int64, commentBody string) error { + return helpers.Run( + fmt.Sprintf("editting comment to %q for issue %d in %q", commentBody, issueNumber, gih.config.repo), + func() error { + return gih.client.EditComment(gih.config.org, gih.config.repo, commentID, commentBody) + }, + gih.config.dryrun, ) } diff --git a/test/mako/alerter/github/issue_test.go b/test/mako/alerter/github/issue_test.go index d070a5e61..4912aca36 100644 --- a/test/mako/alerter/github/issue_test.go +++ b/test/mako/alerter/github/issue_test.go @@ -42,8 +42,8 @@ func TestNewIssueWillBeAdded(t *testing.T) { t.Fatalf("expected to create a new issue %v, but failed", testName) } issueTitle := fmt.Sprintf(issueTitleTemplate, testName) - issueFound := gih.findIssue(gih.config.org, gih.config.repo, issueTitle, false) - if issueFound == nil { + issueFound, err := gih.findIssue(issueTitle) + if issueFound == nil || err != nil { t.Fatalf("expected to find the new created issue %v, but failed to", testName) } } @@ -60,18 +60,15 @@ func TestClosedIssueWillBeReopened(t *testing.T) { if err := gih.CreateIssueForTest(testName, testDesc); err != nil { t.Fatalf("expected to update the existed issue %v, but failed", testName) } - updatedIssue := gih.findIssue(org, repo, issueTitle, gih.config.dryrun) - if updatedIssue == nil || *updatedIssue.State != string(ghutil.IssueOpenState) { + updatedIssue, err := gih.findIssue(issueTitle) + if updatedIssue == nil || err != nil || *updatedIssue.State != string(ghutil.IssueOpenState) { t.Fatalf("expected to reopen the closed issue %v, but failed", testName) } } func TestIssueCanBeClosed(t *testing.T) { - org := gih.config.org - repo := gih.config.repo testName := "test closing existed issue" testDesc := "test closing existed issue desc" - issueTitle := fmt.Sprintf(issueTitleTemplate, testName) if err := gih.CreateIssueForTest(testName, testDesc); err != nil { t.Fatalf("expected to create a new issue %v, but failed", testName) } @@ -79,8 +76,4 @@ func TestIssueCanBeClosed(t *testing.T) { if err := gih.CloseIssueForTest(testName); err != nil { t.Fatalf("tried to close the existed issue %v, but got an error %v", testName, err) } - updatedIssue := gih.findIssue(org, repo, issueTitle, gih.config.dryrun) - if updatedIssue == nil || *updatedIssue.State != string(ghutil.IssueCloseState) { - t.Fatalf("expected to close the issue %v, but failed", testName) - } }