From df67fca537b9808d82495192247e4f0083e739b7 Mon Sep 17 00:00:00 2001 From: haya14busa Date: Tue, 18 Jun 2024 21:58:43 +0900 Subject: [PATCH] fix test flakiness due to problem that proto serialization is not canonical --- scripts/decode-metacomment/main.go | 20 ++++++++++++++ service/github/github.go | 26 ++++++++++++++---- service/github/github_test.go | 44 ++++++++++++++++++------------ 3 files changed, 66 insertions(+), 24 deletions(-) create mode 100644 scripts/decode-metacomment/main.go diff --git a/scripts/decode-metacomment/main.go b/scripts/decode-metacomment/main.go new file mode 100644 index 00000000..f75edb69 --- /dev/null +++ b/scripts/decode-metacomment/main.go @@ -0,0 +1,20 @@ +package main + +import ( + "fmt" + "log" + "os" + + "github.com/reviewdog/reviewdog/service/github" +) + +func main() { + if len(os.Args) == 1 { + log.Fatal("require one argument") + } + meta, err := github.DecodeMetaComment(os.Args[1]) + if err != nil { + log.Fatalf("failed to decode meta comment: %v", err) + } + fmt.Printf("%v\n", meta) +} diff --git a/service/github/github.go b/service/github/github.go index 70187e4b..dd76a918 100644 --- a/service/github/github.go +++ b/service/github/github.go @@ -327,16 +327,11 @@ func extractMetaComment(body string) *metacomment.MetaComment { for _, line := range strings.Split(body, "\n") { if after, found := strings.CutPrefix(line, prefix); found { if metastring, foundSuffix := strings.CutSuffix(after, " -->"); foundSuffix { - b, err := base64.StdEncoding.DecodeString(metastring) + meta, err := DecodeMetaComment(metastring) if err != nil { log.Printf("failed to decode MetaComment: %v", err) continue } - meta := &metacomment.MetaComment{} - if err := proto.Unmarshal(b, meta); err != nil { - log.Printf("failed to unmarshal MetaComment: %v", err) - continue - } return meta } } @@ -344,6 +339,19 @@ func extractMetaComment(body string) *metacomment.MetaComment { return nil } +func DecodeMetaComment(metaBase64 string) (*metacomment.MetaComment, error) { + b, err := base64.StdEncoding.DecodeString(metaBase64) + if err != nil { + return nil, err + } + meta := &metacomment.MetaComment{} + if err := proto.Unmarshal(b, meta); err != nil { + // log.Printf("failed to unmarshal MetaComment: %v", err) + return nil, err + } + return meta, nil +} + // Diff returns a diff of PullRequest. func (g *PullRequest) Diff(ctx context.Context) ([]byte, error) { opt := github.RawOptions{Type: github.Diff} @@ -551,6 +559,12 @@ func getSourceLine(sourceLines map[int]string, line int) (string, error) { func fingerprint(d *rdf.Diagnostic) (string, error) { h := fnv.New64a() + // Ideally, we should not use proto.Marshal since Proto Serialization Is Not + // Canonical. + // https://protobuf.dev/programming-guides/serialization-not-canonical/ + // + // However, I left it as-is for now considering the same reviewdog binary + // should re-calcurate and compare fingerprint for almost all cases. data, err := proto.Marshal(d) if err != nil { return "", err diff --git a/service/github/github_test.go b/service/github/github_test.go index 15fc7ca1..51626fe4 100644 --- a/service/github/github_test.go +++ b/service/github/github_test.go @@ -8,6 +8,7 @@ import ( "net/url" "os" "path/filepath" + "regexp" "strings" "testing" @@ -257,7 +258,7 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) { Path: github.String("reviewdog.go"), Side: github.String("RIGHT"), Line: github.Int(15), - Body: github.String(commentutil.BodyPrefix + "new comment" + "\n\n"), + Body: github.String(commentutil.BodyPrefix + "new comment" + "\n\n"), }, { Path: github.String("reviewdog.go"), @@ -265,7 +266,7 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) { StartSide: github.String("RIGHT"), StartLine: github.Int(15), Line: github.Int(16), - Body: github.String(commentutil.BodyPrefix + "multiline new comment" + "\n\n"), + Body: github.String(commentutil.BodyPrefix + "multiline new comment" + "\n\n"), }, { Path: github.String("reviewdog.go"), @@ -281,7 +282,7 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) { "line3", "```", "", - "", + "", }, "\n") + "\n"), }, { @@ -295,7 +296,7 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) { "line2", "```", "", - "", + "", }, "\n") + "\n"), }, { @@ -308,7 +309,7 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) { "invalid lines suggestion comment", invalidSuggestionPre + "GitHub comment range and suggestion line range must be same. L15-L16 v.s. L16-L17" + invalidSuggestionPost, "", - "", + "", }, "\n") + "\n"), }, { @@ -325,7 +326,7 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) { "line3", "```", "", - "", + "", }, "\n") + "\n"), }, { @@ -343,7 +344,7 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) { "```", invalidSuggestionPre + "GitHub comment range and suggestion line range must be same. L14-L16 v.s. L14-L14" + invalidSuggestionPost, "", - "", + "", }, "\n") + "\n"), }, { @@ -356,7 +357,7 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) { "non-line based suggestion comment (no source lines)", invalidSuggestionPre + "source lines are not available" + invalidSuggestionPost, "", - "", + "", }, "\n") + "\n"), }, { @@ -369,7 +370,7 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) { "haya14busa", "```", "", - "", + "", }, "\n") + "\n"), }, { @@ -384,7 +385,7 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) { "haya14busa (multi-line)", "```", "", - "", + "", }, "\n") + "\n"), }, { @@ -399,7 +400,7 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) { "line 15 (content at line 15)", "```", "", - "", + "", }, "\n") + "\n"), }, { @@ -412,7 +413,7 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) { "haya14busa", "```", "", - "", + "", }, "\n") + "\n"), }, { @@ -431,7 +432,7 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) { "haya14busa", "```", "", - "", + "", }, "\n") + "\n"), }, { @@ -444,7 +445,7 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) { "haya14busa", "```", "", - "", + "", }, "\n") + "\n"), }, { @@ -461,7 +462,7 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) { "```", "````", "", - "", + "", }, "\n") + "\n"), }, { @@ -476,7 +477,7 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) { "```", "````", "", - "", + "", }, "\n") + "\n"), }, { @@ -492,7 +493,7 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) { "`````", "``````", "", - "", + "", }, "\n") + "\n"), }, { @@ -511,11 +512,18 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) { "", "related loc test (2)", "https://test/repo/path/blob/sha/service/github/reviewdog2.go#L14", - "", + "", "", }, "\n")), }, } + // Replace __reviewdog__ comment so that the test pass regardless of environments. + // Proto serialization is not canonical, and test could break unless + // replacing the metacomment string. + for i := 0; i < len(req.Comments); i++ { + metaCommentRe := regexp.MustCompile(`__reviewdog__:\S+`) + req.Comments[i].Body = github.String(metaCommentRe.ReplaceAllString(*req.Comments[i].Body, `__reviewdog__:xxxxxxxxxx`)) + } if diff := pretty.Compare(want, req.Comments); diff != "" { t.Errorf("req.Comments diff: (-got +want)\n%s", diff) }