From eec85da60b9ae37e7bfc959b3eb946907abec5e7 Mon Sep 17 00:00:00 2001 From: Taras Madan Date: Fri, 13 Dec 2024 11:38:58 +0100 Subject: pkg/covermerger: always store details Storing all the details about coverage data source we're able to better explain the origin. This origin data is currently used to get "manager" name. --- pkg/cover/file.go | 1 - pkg/covermerger/covermerger.go | 3 +- pkg/covermerger/covermerger_test.go | 98 +++++++++++++++++++++++++++----- pkg/covermerger/file_line_merger.go | 15 ++--- tools/syz-covermerger/syz_covermerger.go | 1 - 5 files changed, 90 insertions(+), 28 deletions(-) diff --git a/pkg/cover/file.go b/pkg/cover/file.go index 368607ffe..ff2b7a0ec 100644 --- a/pkg/cover/file.go +++ b/pkg/cover/file.go @@ -59,7 +59,6 @@ func GetMergeResult(c context.Context, ns, repo, forCommit, sourceCommit, filePa Commit: forCommit, }, FileVersProvider: covermerger.MakeWebGit(proxy), - StoreDetails: true, } fromDate, toDate := tp.DatesFromTo() diff --git a/pkg/covermerger/covermerger.go b/pkg/covermerger/covermerger.go index 8fc058ddd..394b606de 100644 --- a/pkg/covermerger/covermerger.go +++ b/pkg/covermerger/covermerger.go @@ -63,7 +63,7 @@ func batchFileData(c *Config, targetFilePath string, records []*FileRecord) (*Me if err != nil { return nil, fmt.Errorf("failed to getFileVersions: %w", err) } - merger := makeFileLineCoverMerger(fvs, c.Base, c.StoreDetails) + merger := makeFileLineCoverMerger(fvs, c.Base) for _, record := range records { merger.Add(record) } @@ -113,7 +113,6 @@ type Config struct { skipRepoClone bool Base RepoCommit FileVersProvider FileVersProvider - StoreDetails bool } func isSchema(fields, schema []string) bool { diff --git a/pkg/covermerger/covermerger_test.go b/pkg/covermerger/covermerger_test.go index a8b2a3ca8..b63ae36be 100644 --- a/pkg/covermerger/covermerger_test.go +++ b/pkg/covermerger/covermerger_test.go @@ -23,6 +23,7 @@ func TestAggregateStreamData(t *testing.T) { simpleAggregation string baseRepo string baseCommit string + checkDetails bool } tests := []Test{ { @@ -42,11 +43,13 @@ samp_time,1,360,arch,b1,ci-mock,git://repo,master,commit1,delete_code.c,func1,2, "delete_code.c": { "HitCounts":{}, - "FileExists": true + "FileExists": true, + "LineDetails":{} } }`, - baseRepo: "git://repo", - baseCommit: "commit2", + baseRepo: "git://repo", + baseCommit: "commit2", + checkDetails: true, }, { name: "file deleted", @@ -59,8 +62,9 @@ samp_time,1,360,arch,b1,ci-mock,git://repo,master,commit1,delete_file.c,func1,2, "FileExists": false } }`, - baseRepo: "git://repo", - baseCommit: "commit2", + baseRepo: "git://repo", + baseCommit: "commit2", + checkDetails: true, }, { name: "covered line changed", @@ -72,11 +76,26 @@ samp_time,1,360,arch,b1,ci-mock,git://repo,master,commit1,change_line.c,func1,3, "change_line.c": { "HitCounts":{"3": 1}, - "FileExists": true + "FileExists": true, + "LineDetails": + { + "3": + [ + { + "FilePath":"change_line.c", + "Repo":"git://repo", + "Commit":"commit1", + "StartLine":3, + "HitCount":1, + "Manager":"ci-mock" + } + ] + } } }`, - baseRepo: "git://repo", - baseCommit: "commit2", + baseRepo: "git://repo", + baseCommit: "commit2", + checkDetails: true, }, { name: "add line", @@ -87,11 +106,26 @@ samp_time,1,360,arch,b1,ci-mock,git://repo,master,commit1,add_line.c,func1,2,0,2 "add_line.c": { "HitCounts":{"2": 1}, - "FileExists": true + "FileExists": true, + "LineDetails": + { + "2": + [ + { + "FilePath":"add_line.c", + "Repo":"git://repo", + "Commit":"commit1", + "StartLine":2, + "HitCount":1, + "Manager":"ci-mock" + } + ] + } } }`, - baseRepo: "git://repo", - baseCommit: "commit2", + baseRepo: "git://repo", + baseCommit: "commit2", + checkDetails: true, }, { name: "instrumented lines w/o coverage are reported", @@ -103,11 +137,37 @@ samp_time,1,360,arch,b1,ci-mock,git://repo,master,commit2,not_changed.c,func1,4, "not_changed.c": { "HitCounts":{"3": 0, "4": 0}, - "FileExists": true + "FileExists": true, + "LineDetails": + { + "3": + [ + { + "FilePath":"not_changed.c", + "Repo":"git://repo", + "Commit":"commit1", + "StartLine":3, + "HitCount":0, + "Manager":"ci-mock" + } + ], + "4": + [ + { + "FilePath":"not_changed.c", + "Repo":"git://repo", + "Commit":"commit2", + "StartLine":4, + "HitCount":0, + "Manager":"ci-mock" + } + ] + } } }`, - baseRepo: "git://repo", - baseCommit: "commit2", + baseRepo: "git://repo", + baseCommit: "commit2", + checkDetails: true, }, } for _, test := range tests { @@ -124,14 +184,24 @@ samp_time,1,360,arch,b1,ci-mock,git://repo,master,commit2,not_changed.c,func1,4, }, strings.NewReader(test.bqTable), ) + assert.Nil(t, err) var expectedAggregation FilesMergeResults assert.Nil(t, json.Unmarshal([]byte(test.simpleAggregation), &expectedAggregation)) + if !test.checkDetails { + ignoreLineDetailsInTest(aggregation) + } assert.Equal(t, expectedAggregation, aggregation) }) } } +func ignoreLineDetailsInTest(results FilesMergeResults) { + for _, mr := range results { + mr.LineDetails = nil + } +} + type fileVersProviderMock struct { Workdir string } diff --git a/pkg/covermerger/file_line_merger.go b/pkg/covermerger/file_line_merger.go index 8d4c7f7d7..5d9c0ab7a 100644 --- a/pkg/covermerger/file_line_merger.go +++ b/pkg/covermerger/file_line_merger.go @@ -5,8 +5,7 @@ package covermerger import "github.com/google/syzkaller/pkg/log" -func makeFileLineCoverMerger( - fvs fileVersions, base RepoCommit, storeDetails bool) FileCoverageMerger { +func makeFileLineCoverMerger(fvs fileVersions, base RepoCommit) FileCoverageMerger { baseFile := "" baseFileExists := false for repoCommit, fv := range fvs { @@ -21,16 +20,14 @@ func makeFileLineCoverMerger( } a := &FileLineCoverMerger{ MergeResult: &MergeResult{ - HitCounts: make(map[int]int), - FileExists: true, + HitCounts: make(map[int]int), + FileExists: true, + LineDetails: make(map[int][]*FileRecord), }, baseFile: baseFile, matchers: make(map[RepoCommit]*LineToLineMatcher), lostFrames: map[RepoCommit]int64{}, } - if storeDetails { - a.MergeResult.LineDetails = make(map[int][]*FileRecord) - } for repoBranch, fv := range fvs { a.matchers[repoBranch] = makeLineToLineMatcher(fv, baseFile) } @@ -53,9 +50,7 @@ func (a *FileLineCoverMerger) Add(record *FileRecord) { } if targetLine := a.matchers[record.RepoCommit].SameLinePos(record.StartLine); targetLine != -1 { a.HitCounts[targetLine] += record.HitCount - if a.LineDetails != nil { - a.LineDetails[targetLine] = append(a.LineDetails[targetLine], record) - } + a.LineDetails[targetLine] = append(a.LineDetails[targetLine], record) } } diff --git a/tools/syz-covermerger/syz_covermerger.go b/tools/syz-covermerger/syz_covermerger.go index 260a57281..61a25dc1f 100644 --- a/tools/syz-covermerger/syz_covermerger.go +++ b/tools/syz-covermerger/syz_covermerger.go @@ -56,7 +56,6 @@ func main() { Commit: *flagCommit, }, FileVersProvider: makeProvider(), - StoreDetails: true, } var dateFrom, dateTo civil.Date var err error -- cgit mrf-deployment