Skip to content

Commit

Permalink
fix test flakiness due to problem that proto serialization is not can…
Browse files Browse the repository at this point in the history 
…onical
  • Loading branch information
@haya14busa
haya14busa committed Jun 18, 2024
1 parent 9734bbe commit df67fca
Show file tree
Hide file tree
Showing 3 changed files with 66 additions and 24 deletions.
20 changes: 20 additions & 0 deletions scripts/decode-metacomment/main.go
View file
Original file line number Diff line number Diff line change
@@ -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)
}
26 changes: 20 additions & 6 deletions service/github/github.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -327,23 +327,31 @@ 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
}
}
}
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}
Expand Down Expand Up @@ -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
Expand Down
44 changes: 26 additions & 18 deletions service/github/github_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"net/url"
"os"
"path/filepath"
"regexp"
"strings"
"testing"

Expand Down Expand Up @@ -257,15 +258,15 @@ 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<!-- __reviewdog__:ChA4NjA2Mzk3ZGNhMGViOWMw -->\n"),
Body: github.String(commentutil.BodyPrefix + "new comment" + "\n<!-- __reviewdog__:xxxxxxxxxx -->\n"),
},
{
Path: github.String("reviewdog.go"),
Side: github.String("RIGHT"),
StartSide: github.String("RIGHT"),
StartLine: github.Int(15),
Line: github.Int(16),
Body: github.String(commentutil.BodyPrefix + "multiline new comment" + "\n<!-- __reviewdog__:ChBjNGFmMTRlYWJkYjBhNTU1 -->\n"),
Body: github.String(commentutil.BodyPrefix + "multiline new comment" + "\n<!-- __reviewdog__:xxxxxxxxxx -->\n"),
},
{
Path: github.String("reviewdog.go"),
Expand All @@ -281,7 +282,7 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
"line3",
"```",
"",
"<!-- __reviewdog__:ChBlZTZhOWZjZjFkMDQ2OTgx -->",
"<!-- __reviewdog__:xxxxxxxxxx -->",
}, "\n") + "\n"),
},
{
Expand All @@ -295,7 +296,7 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
"line2",
"```",
"",
"<!-- __reviewdog__:ChBlOGViMDE3YTQ3MmIwMDc0 -->",
"<!-- __reviewdog__:xxxxxxxxxx -->",
}, "\n") + "\n"),
},
{
Expand All @@ -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,
"",
"<!-- __reviewdog__:ChAyNjJlYTIzNWUxYzgzZjBj -->",
"<!-- __reviewdog__:xxxxxxxxxx -->",
}, "\n") + "\n"),
},
{
Expand All @@ -325,7 +326,7 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
"line3",
"```",
"",
"<!-- __reviewdog__:ChAyMTYwNmIyODI4MTBkMmUx -->",
"<!-- __reviewdog__:xxxxxxxxxx -->",
}, "\n") + "\n"),
},
{
Expand All @@ -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,
"",
"<!-- __reviewdog__:ChA3ZDIwNzM1MTFhMmRmMTVh -->",
"<!-- __reviewdog__:xxxxxxxxxx -->",
}, "\n") + "\n"),
},
{
Expand All @@ -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,
"",
"<!-- __reviewdog__:ChA3ODliZTFlZTYzMDM3Njhj -->",
"<!-- __reviewdog__:xxxxxxxxxx -->",
}, "\n") + "\n"),
},
{
Expand All @@ -369,7 +370,7 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
"haya14busa",
"```",
"",
"<!-- __reviewdog__:ChA2NzE4OTdhYjExNzZlZjAw -->",
"<!-- __reviewdog__:xxxxxxxxxx -->",
}, "\n") + "\n"),
},
{
Expand All @@ -384,7 +385,7 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
"haya14busa (multi-line)",
"```",
"",
"<!-- __reviewdog__:ChAzNzYyZTE3N2Q3OWNlYjA3 -->",
"<!-- __reviewdog__:xxxxxxxxxx -->",
}, "\n") + "\n"),
},
{
Expand All @@ -399,7 +400,7 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
"line 15 (content at line 15)",
"```",
"",
"<!-- __reviewdog__:ChBmMTFmMjhjNTEzY2ZkNDMw -->",
"<!-- __reviewdog__:xxxxxxxxxx -->",
}, "\n") + "\n"),
},
{
Expand All @@ -412,7 +413,7 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
"haya14busa",
"```",
"",
"<!-- __reviewdog__:ChBmNDNmODgyMWMwYTYzNWRl -->",
"<!-- __reviewdog__:xxxxxxxxxx -->",
}, "\n") + "\n"),
},
{
Expand All @@ -431,7 +432,7 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
"haya14busa",
"```",
"",
"<!-- __reviewdog__:ChA1ZjAzYTQ4MzU1MWRhZjYw -->",
"<!-- __reviewdog__:xxxxxxxxxx -->",
}, "\n") + "\n"),
},
{
Expand All @@ -444,7 +445,7 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
"haya14busa",
"```",
"",
"<!-- __reviewdog__:ChA3YWMyOGVkN2MzN2Y4MTY1 -->",
"<!-- __reviewdog__:xxxxxxxxxx -->",
}, "\n") + "\n"),
},
{
Expand All @@ -461,7 +462,7 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
"```",
"````",
"",
"<!-- __reviewdog__:ChA5MWQ0YTY4N2RjZDJlOGZj -->",
"<!-- __reviewdog__:xxxxxxxxxx -->",
}, "\n") + "\n"),
},
{
Expand All @@ -476,7 +477,7 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
"```",
"````",
"",
"<!-- __reviewdog__:ChAzNTA4YTViNTc1NjViY2Y0 -->",
"<!-- __reviewdog__:xxxxxxxxxx -->",
}, "\n") + "\n"),
},
{
Expand All @@ -492,7 +493,7 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
"`````",
"``````",
"",
"<!-- __reviewdog__:ChA3ZGVhN2U3Zjk2NDVmOTVk -->",
"<!-- __reviewdog__:xxxxxxxxxx -->",
}, "\n") + "\n"),
},
{
Expand All @@ -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",
"<!-- __reviewdog__:ChBlMDY2NzMwMmE3ZDFlZTBk -->",
"<!-- __reviewdog__:xxxxxxxxxx -->",
"",
}, "\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)
}
Expand Down

0 comments on commit df67fca

Please sign in to comment.