diff options
| author | Taras Madan <tarasmadan@google.com> | 2025-01-09 15:45:40 +0100 |
|---|---|---|
| committer | Taras Madan <tarasmadan@google.com> | 2025-01-27 10:05:21 +0000 |
| commit | 0868754a9d325ba9011e1cb74510f68d4b627c79 (patch) | |
| tree | ce1f63a84afa095227d308ccfff258e2d1dea225 /pkg/cover | |
| parent | d99a33ad01eb09190a8680d743f8d520e459ef0f (diff) | |
dashboard/app: show manager unique coverage
1. Make heatmap testable, move out the spanner client instantiation.
2. Generate spannerdb.ReadOnlyTransaction mocks.
3. Generate spannerdb.RowIterator mocks.
4. Generate spannerdb.Row mocks.
5. Prepare spannerdb fixture.
6. Fixed html control name + value.
7. Added multiple tests.
8. Show line coverage from selected manager.
9. Propagate coverage url params to file coverage url.
Diffstat (limited to 'pkg/cover')
| -rw-r--r-- | pkg/cover/file.go | 2 | ||||
| -rw-r--r-- | pkg/cover/heatmap.go | 206 | ||||
| -rw-r--r-- | pkg/cover/heatmap_test.go | 240 | ||||
| -rw-r--r-- | pkg/cover/templates/heatmap.html | 4 |
4 files changed, 419 insertions, 33 deletions
diff --git a/pkg/cover/file.go b/pkg/cover/file.go index a0561dfe7..c56f88204 100644 --- a/pkg/cover/file.go +++ b/pkg/cover/file.go @@ -101,7 +101,7 @@ func GetMergeResult(c context.Context, ns, repo, forCommit, sourceCommit, filePa func rendResult(content string, coverage *covermerger.MergeResult, renderConfig *CoverageRenderConfig) string { if coverage == nil { coverage = &covermerger.MergeResult{ - HitCounts: map[int]int{}, + HitCounts: map[int]int64{}, LineDetails: map[int][]*covermerger.FileRecord{}, } } diff --git a/pkg/cover/heatmap.go b/pkg/cover/heatmap.go index b8e3bbfa6..04b7e6914 100644 --- a/pkg/cover/heatmap.go +++ b/pkg/cover/heatmap.go @@ -17,6 +17,7 @@ import ( "github.com/google/syzkaller/pkg/coveragedb/spannerclient" _ "github.com/google/syzkaller/pkg/subsystem/lists" "golang.org/x/exp/maps" + "golang.org/x/sync/errgroup" "google.golang.org/api/iterator" ) @@ -115,6 +116,16 @@ type fileCoverageWithDetails struct { Subsystems []string } +type fileCoverageWithLineInfo struct { + fileCoverageWithDetails + LinesInstrumented []int64 + HitCounts []int64 +} + +func (fc *fileCoverageWithLineInfo) CovMap() map[int]int64 { + return MakeCovMap(fc.LinesInstrumented, fc.HitCounts) +} + type pageColumnTarget struct { TimePeriod coveragedb.TimePeriod Commit string @@ -157,18 +168,17 @@ func filesCoverageToTemplateData(fCov []*fileCoverageWithDetails) *templateHeatm return &res } -func filesCoverageWithDetailsStmt(ns, subsystem, manager string, timePeriod coveragedb.TimePeriod) spanner.Statement { +func filesCoverageWithDetailsStmt(ns, subsystem, manager string, timePeriod coveragedb.TimePeriod, withLines bool, +) spanner.Statement { if manager == "" { manager = "*" } + selectColumns := "commit, instrumented, covered, files.filepath, subsystems" + if withLines { + selectColumns += ", linesinstrumented, hitcounts" + } stmt := spanner.Statement{ - SQL: ` -select - commit, - instrumented, - covered, - files.filepath, - subsystems + SQL: "select " + selectColumns + ` from merge_history join files on merge_history.session = files.session @@ -187,37 +197,171 @@ where stmt.SQL += " and $5=ANY(subsystems)" stmt.Params["p5"] = subsystem } + stmt.SQL += "\norder by files.filepath" return stmt } -func filesCoverageWithDetails(ctx context.Context, projectID string, scope *SelectScope, -) ([]*fileCoverageWithDetails, error) { - client, err := spannerclient.NewClient(ctx, projectID) +func readCoverage(iterManager spannerclient.RowIterator) ([]*fileCoverageWithDetails, error) { + res := []*fileCoverageWithDetails{} + ch := make(chan *fileCoverageWithDetails) + var err error + go func() { + defer close(ch) + err = readIterToChan(context.Background(), iterManager, ch) + }() + for fc := range ch { + res = append(res, fc) + } if err != nil { - return nil, fmt.Errorf("spanner.NewClient() failed: %s", err.Error()) + return nil, fmt.Errorf("readIterToChan: %w", err) } - defer client.Close() + return res, nil +} +// Unique coverage from specific manager is more expensive to get. +// We get unique coverage comparing manager and total coverage on the AppEngine side. +func readCoverageUniq(full, mgr spannerclient.RowIterator, +) ([]*fileCoverageWithDetails, error) { + eg, ctx := errgroup.WithContext(context.Background()) + fullCh := make(chan *fileCoverageWithLineInfo) + eg.Go(func() error { + defer close(fullCh) + return readIterToChan(ctx, full, fullCh) + }) + partCh := make(chan *fileCoverageWithLineInfo) + eg.Go(func() error { + defer close(partCh) + return readIterToChan(ctx, mgr, partCh) + }) res := []*fileCoverageWithDetails{} - for _, timePeriod := range scope.Periods { - stmt := filesCoverageWithDetailsStmt(scope.Ns, scope.Subsystem, scope.Manager, timePeriod) - iter := client.Single().Query(ctx, stmt) - defer iter.Stop() - for { - row, err := iter.Next() - if err == iterator.Done { - break + eg.Go(func() error { + partCov := <-partCh + for fullCov := range fullCh { + if partCov == nil || partCov.Filepath > fullCov.Filepath { + // No pair for the file in full aggregation is available. + cov := fullCov.fileCoverageWithDetails + cov.Covered = 0 + res = append(res, &cov) + continue + } + if partCov.Filepath == fullCov.Filepath { + if partCov.Commit != fullCov.Commit || + !IsComparable( + fullCov.LinesInstrumented, fullCov.HitCounts, + partCov.LinesInstrumented, partCov.HitCounts) { + return fmt.Errorf("db record for file %s doesn't match", fullCov.Filepath) + } + resItem := fullCov.fileCoverageWithDetails // Use Instrumented count from full aggregation. + resItem.Covered = 0 + for _, hc := range UniqCoverage(fullCov.CovMap(), partCov.CovMap()) { + if hc > 0 { + resItem.Covered++ + } + } + res = append(res, &resItem) + partCov = <-partCh + continue } + // Partial coverage is a subset of full coverage. + // File can't exist only in partial set. + return fmt.Errorf("currupted db, file %s can't exist", partCov.Filepath) + } + return nil + }) + if err := eg.Wait(); err != nil { + return nil, fmt.Errorf("eg.Wait: %w", err) + } + return res, nil +} + +func MakeCovMap(keys, vals []int64) map[int]int64 { + res := map[int]int64{} + for i, key := range keys { + res[int(key)] = vals[i] + } + return res +} + +func IsComparable(fullLines, fullHitCounts, partialLines, partialHitCounts []int64) bool { + if len(fullLines) != len(fullHitCounts) || + len(partialLines) != len(partialHitCounts) || + len(fullLines) < len(partialLines) { + return false + } + fullCov := MakeCovMap(fullLines, fullHitCounts) + for iPartial, ln := range partialLines { + partialHitCount := partialHitCounts[iPartial] + if fullHitCount, fullExist := fullCov[int(ln)]; !fullExist || fullHitCount < partialHitCount { + return false + } + } + return true +} + +// Returns partial hitcounts that are the only source of the full hitcounts. +func UniqCoverage(fullCov, partCov map[int]int64) map[int]int64 { + res := maps.Clone(partCov) + for ln := range partCov { + if partCov[ln] != fullCov[ln] { + res[ln] = 0 + } + } + return res +} + +func readIterToChan[K fileCoverageWithLineInfo | fileCoverageWithDetails]( + ctx context.Context, iter spannerclient.RowIterator, ch chan<- *K) error { + for { + row, err := iter.Next() + if err == iterator.Done { + break + } + if err != nil { + return fmt.Errorf("iter.Next: %w", err) + } + var r K + if err = row.ToStruct(&r); err != nil { + return fmt.Errorf("row.ToStruct: %w", err) + } + select { + case ch <- &r: + case <-ctx.Done(): + return nil + } + } + return nil +} + +func filesCoverageWithDetails( + ctx context.Context, client spannerclient.SpannerClient, scope *SelectScope, onlyUnique bool, +) ([]*fileCoverageWithDetails, error) { + var res []*fileCoverageWithDetails + for _, timePeriod := range scope.Periods { + needLinesDetails := onlyUnique + iterManager := client.Single().Query(ctx, + filesCoverageWithDetailsStmt(scope.Ns, scope.Subsystem, scope.Manager, timePeriod, needLinesDetails)) + defer iterManager.Stop() + + var err error + var periodRes []*fileCoverageWithDetails + if onlyUnique { + iterAll := client.Single().Query(ctx, + filesCoverageWithDetailsStmt(scope.Ns, scope.Subsystem, "", timePeriod, needLinesDetails)) + defer iterAll.Stop() + periodRes, err = readCoverageUniq(iterAll, iterManager) if err != nil { - return nil, fmt.Errorf("failed to iter.Next() spanner DB: %w", err) + return nil, fmt.Errorf("uniqueFilesCoverageWithDetails: %w", err) } - var r fileCoverageWithDetails - if err = row.ToStruct(&r); err != nil { - return nil, fmt.Errorf("failed to row.ToStruct() spanner DB: %w", err) + } else { + periodRes, err = readCoverage(iterManager) + if err != nil { + return nil, fmt.Errorf("readCoverage: %w", err) } + } + for _, r := range periodRes { r.TimePeriod = timePeriod - res = append(res, &r) } + res = append(res, periodRes...) } return res, nil } @@ -252,9 +396,10 @@ type SelectScope struct { Periods []coveragedb.TimePeriod } -func DoHeatMapStyleBodyJS(ctx context.Context, projectID string, scope *SelectScope, sss, managers []string, +func DoHeatMapStyleBodyJS( + ctx context.Context, client spannerclient.SpannerClient, scope *SelectScope, onlyUnique bool, sss, managers []string, ) (template.CSS, template.HTML, template.HTML, error) { - covAndDates, err := filesCoverageWithDetails(ctx, projectID, scope) + covAndDates, err := filesCoverageWithDetails(ctx, client, scope, onlyUnique) if err != nil { return "", "", "", fmt.Errorf("failed to filesCoverageWithDetails: %w", err) } @@ -264,9 +409,10 @@ func DoHeatMapStyleBodyJS(ctx context.Context, projectID string, scope *SelectSc return stylesBodyJSTemplate(templData) } -func DoSubsystemsHeatMapStyleBodyJS(ctx context.Context, projectID string, scope *SelectScope, sss, managers []string, +func DoSubsystemsHeatMapStyleBodyJS( + ctx context.Context, client spannerclient.SpannerClient, scope *SelectScope, onlyUnique bool, sss, managers []string, ) (template.CSS, template.HTML, template.HTML, error) { - covWithDetails, err := filesCoverageWithDetails(ctx, projectID, scope) + covWithDetails, err := filesCoverageWithDetails(ctx, client, scope, onlyUnique) if err != nil { panic(err) } diff --git a/pkg/cover/heatmap_test.go b/pkg/cover/heatmap_test.go index d872e31d1..ad1f9bf64 100644 --- a/pkg/cover/heatmap_test.go +++ b/pkg/cover/heatmap_test.go @@ -4,14 +4,254 @@ package cover import ( + "context" "testing" "time" "cloud.google.com/go/civil" "github.com/google/syzkaller/pkg/coveragedb" + "github.com/google/syzkaller/pkg/coveragedb/mocks" + "github.com/google/syzkaller/pkg/coveragedb/spannerclient" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "google.golang.org/api/iterator" ) +func TestFilesCoverageWithDetails(t *testing.T) { + period, _ := coveragedb.MakeTimePeriod( + civil.Date{Year: 2025, Month: 1, Day: 1}, + "day") + tests := []struct { + name string + scope *SelectScope + client func() spannerclient.SpannerClient + onlyUnique bool + want []*fileCoverageWithDetails + wantErr bool + }{ + { + name: "empty scope", + scope: &SelectScope{}, + want: nil, + wantErr: false, + }, + { + name: "single day, no filters, empty DB => no coverage", + scope: &SelectScope{ + Ns: "upstream", + Periods: []coveragedb.TimePeriod{period}, + }, + client: func() spannerclient.SpannerClient { return emptyCoverageDBFixture(t, 1) }, + want: nil, + wantErr: false, + }, + { + name: "single day, unique coverage, empty DB => no coverage", + scope: &SelectScope{ + Ns: "upstream", + Periods: []coveragedb.TimePeriod{period}, + }, + client: func() spannerclient.SpannerClient { return emptyCoverageDBFixture(t, 2) }, + onlyUnique: true, + want: nil, + wantErr: false, + }, + { + name: "single day, unique coverage, empty partial result => 0/3 covered", + scope: &SelectScope{ + Ns: "upstream", + Periods: []coveragedb.TimePeriod{period}, + }, + client: func() spannerclient.SpannerClient { + return fullCoverageDBFixture( + t, + []*fileCoverageWithLineInfo{ + { + fileCoverageWithDetails: fileCoverageWithDetails{ + Filepath: "file1", + Instrumented: 3, + Covered: 3, + }, + LinesInstrumented: []int64{1, 2, 3}, + HitCounts: []int64{1, 1, 1}, + }, + }, + nil) + }, + onlyUnique: true, + want: []*fileCoverageWithDetails{ + { + Filepath: "file1", + Instrumented: 3, + Covered: 0, + TimePeriod: period, + }, + }, + wantErr: false, + }, + { + name: "single day, unique coverage, full result match => 3/3 covered", + scope: &SelectScope{ + Ns: "upstream", + Periods: []coveragedb.TimePeriod{period}, + }, + client: func() spannerclient.SpannerClient { + return fullCoverageDBFixture( + t, + []*fileCoverageWithLineInfo{ + { + fileCoverageWithDetails: fileCoverageWithDetails{ + Filepath: "file1", + Instrumented: 3, + Covered: 3, + }, + LinesInstrumented: []int64{1, 2, 3}, + HitCounts: []int64{1, 1, 1}, + }, + }, + []*fileCoverageWithLineInfo{ + { + fileCoverageWithDetails: fileCoverageWithDetails{ + Filepath: "file1", + Instrumented: 3, + Covered: 3, + }, + LinesInstrumented: []int64{1, 2, 3}, + HitCounts: []int64{1, 1, 1}, + }, + }) + }, + onlyUnique: true, + want: []*fileCoverageWithDetails{ + { + Filepath: "file1", + Instrumented: 3, + Covered: 3, + TimePeriod: period, + }, + }, + wantErr: false, + }, + { + name: "single day, unique coverage, partial result match => 3/5 covered", + scope: &SelectScope{ + Ns: "upstream", + Periods: []coveragedb.TimePeriod{period}, + }, + client: func() spannerclient.SpannerClient { + return fullCoverageDBFixture( + t, + []*fileCoverageWithLineInfo{ + { + fileCoverageWithDetails: fileCoverageWithDetails{ + Filepath: "file1", + Instrumented: 5, + Covered: 5, + }, + LinesInstrumented: []int64{1, 2, 3, 4, 5}, + HitCounts: []int64{3, 4, 5, 6, 7}, + }, + }, + []*fileCoverageWithLineInfo{ + { + fileCoverageWithDetails: fileCoverageWithDetails{ + Filepath: "file1", + Instrumented: 4, + Covered: 3, + }, + LinesInstrumented: []int64{1, 2, 3, 5}, + HitCounts: []int64{3, 0, 5, 7}, + }, + }) + }, + onlyUnique: true, + want: []*fileCoverageWithDetails{ + { + Filepath: "file1", + Instrumented: 5, + Covered: 3, + TimePeriod: period, + }, + }, + wantErr: false, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + var testClient spannerclient.SpannerClient + if test.client != nil { + testClient = test.client() + } + got, gotErr := filesCoverageWithDetails( + context.Background(), + testClient, test.scope, test.onlyUnique) + if test.wantErr { + assert.Error(t, gotErr) + } else { + assert.NoError(t, gotErr) + } + assert.Equal(t, test.want, got) + }) + } +} + +func emptyCoverageDBFixture(t *testing.T, times int) spannerclient.SpannerClient { + mRowIterator := mocks.NewRowIterator(t) + mRowIterator.On("Stop").Return().Times(times) + mRowIterator.On("Next"). + Return(nil, iterator.Done).Times(times) + + mTran := mocks.NewReadOnlyTransaction(t) + mTran.On("Query", mock.Anything, mock.Anything). + Return(mRowIterator).Times(times) + + m := mocks.NewSpannerClient(t) + m.On("Single"). + Return(mTran).Times(times) + return m +} + +func fullCoverageDBFixture( + t *testing.T, full, partial []*fileCoverageWithLineInfo, +) spannerclient.SpannerClient { + mPartialTran := mocks.NewReadOnlyTransaction(t) + mPartialTran.On("Query", mock.Anything, mock.Anything). + Return(newRowIteratorMock(t, partial)).Once() + + mFullTran := mocks.NewReadOnlyTransaction(t) + mFullTran.On("Query", mock.Anything, mock.Anything). + Return(newRowIteratorMock(t, full)).Once() + + m := mocks.NewSpannerClient(t) + m.On("Single"). + Return(mPartialTran).Once() + m.On("Single"). + Return(mFullTran).Once() + return m +} + +func newRowIteratorMock(t *testing.T, events []*fileCoverageWithLineInfo, +) *mocks.RowIterator { + m := mocks.NewRowIterator(t) + m.On("Stop").Once().Return() + for _, item := range events { + mRow := mocks.NewRow(t) + mRow.On("ToStruct", mock.Anything). + Run(func(args mock.Arguments) { + arg := args.Get(0).(*fileCoverageWithLineInfo) + *arg = *item + }). + Return(nil).Once() + + m.On("Next"). + Return(mRow, nil).Once() + } + + m.On("Next"). + Return(nil, iterator.Done).Once() + return m +} + func TestFilesCoverageToTemplateData(t *testing.T) { tests := []struct { name string diff --git a/pkg/cover/templates/heatmap.html b/pkg/cover/templates/heatmap.html index c3fcd5db7..cc5da9bcc 100644 --- a/pkg/cover/templates/heatmap.html +++ b/pkg/cover/templates/heatmap.html @@ -115,7 +115,7 @@ <br> <label for="target-manager">Manager:</label> <br> - <label for="only-unique">Only unique:</label> + <label for="unique-only">Only unique:</label> </div> <div style="display:inline-block; vertical-align: top"> <select id="target-period" name="period"> @@ -139,7 +139,7 @@ {{ end }} </select> <br> - <input type="checkbox" id="only-unique" name="unique-only" disabled> + <input type="checkbox" id="unique-only" name="unique-only" value="1"> </div> <br> <button id="updateButton">Update</button> |
