From bbd06db439c9c44da9d403567e97db0d0d0de2f2 Mon Sep 17 00:00:00 2001 From: Taras Madan Date: Thu, 23 Jan 2025 21:54:41 +0100 Subject: [PATCH] dashboard/app: test coverage /file link 1. Init coveragedb client once and propagate it through context to enable mocking. 2. Always init coverage handlers. It simplifies testing. 3. Read webGit and coveragedb client from ctx to make it mockable. 4. Use int for file line number and int64 for merged coverage. 5. Add tests. --- Makefile | 1 + dashboard/app/api.go | 9 +- dashboard/app/batch_coverage.go | 4 +- dashboard/app/config.go | 1 + dashboard/app/coverage.go | 74 ++++++++-- dashboard/app/coverage_test.go | 167 ++++++++++++++++++++++ dashboard/app/entities_spanner.go | 11 +- dashboard/app/handler.go | 3 + dashboard/app/main.go | 12 +- dashboard/app/util_test.go | 16 +++ pkg/cover/file.go | 4 +- pkg/cover/heatmap.go | 10 +- pkg/cover/heatmap_test.go | 16 +-- pkg/coveragedb/coveragedb.go | 40 +++--- pkg/covermerger/covermerger_test.go | 6 +- pkg/covermerger/file_line_merger.go | 2 +- pkg/covermerger/mocks/FileVersProvider.go | 64 +++++++++ pkg/covermerger/provider_monorepo.go | 10 +- pkg/covermerger/provider_web.go | 4 +- pkg/validator/validator.go | 2 +- tools/syz-cover/syz-cover.go | 3 +- 21 files changed, 373 insertions(+), 86 deletions(-) create mode 100644 dashboard/app/coverage_test.go create mode 100644 pkg/covermerger/mocks/FileVersProvider.go diff --git a/Makefile b/Makefile index 07716b436795..b57e51d0efcf 100644 --- a/Makefile +++ b/Makefile @@ -236,6 +236,7 @@ generate_go: format_cpp $(GO) generate ./executor ./pkg/ifuzz ./pkg/build ./pkg/rpcserver $(GO) generate ./vm/proxyapp $(GO) generate ./pkg/coveragedb + $(GO) generate ./pkg/covermerger generate_rpc: flatc -o pkg/flatrpc --warnings-as-errors --gen-object-api --filename-suffix "" --go --gen-onefile --go-namespace flatrpc pkg/flatrpc/flatrpc.fbs diff --git a/dashboard/app/api.go b/dashboard/app/api.go index b06c5ac07833..e1269b3d087d 100644 --- a/dashboard/app/api.go +++ b/dashboard/app/api.go @@ -26,7 +26,6 @@ import ( "github.com/google/syzkaller/pkg/asset" "github.com/google/syzkaller/pkg/auth" "github.com/google/syzkaller/pkg/coveragedb" - "github.com/google/syzkaller/pkg/coveragedb/spannerclient" "github.com/google/syzkaller/pkg/debugtracer" "github.com/google/syzkaller/pkg/email" "github.com/google/syzkaller/pkg/gcs" @@ -105,6 +104,7 @@ var maxCrashes = func() int { func handleJSON(fn JSONHandler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { c := appengine.NewContext(r) + c = SetCoverageDBClient(c, coverageDBClient) reply, err := fn(c, r) if err != nil { status := logErrorPrepareStatus(c, err) @@ -1948,12 +1948,7 @@ func apiSaveCoverage(c context.Context, payload io.Reader) (interface{}, error) sss = service.List() log.Infof(c, "found %d subsystems for %s namespace", len(sss), descr.Namespace) } - client, err := spannerclient.NewClient(c, appengine.AppID(context.Background())) - if err != nil { - return 0, fmt.Errorf("coveragedb.NewClient() failed: %s", err.Error()) - } - defer client.Close() - rowsCreated, err := coveragedb.SaveMergeResult(c, client, descr, jsonDec, sss) + rowsCreated, err := coveragedb.SaveMergeResult(c, GetCoverageDBClient(c), descr, jsonDec, sss) if err != nil { log.Errorf(c, "error storing coverage for ns %s, date %s: %v", descr.Namespace, descr.DateTo.String(), err) diff --git a/dashboard/app/batch_coverage.go b/dashboard/app/batch_coverage.go index 6d53496a7087..aba85cf596da 100644 --- a/dashboard/app/batch_coverage.go +++ b/dashboard/app/batch_coverage.go @@ -43,7 +43,7 @@ func handleBatchCoverage(w http.ResponseWriter, r *http.Request) { if err != nil { log.Errorf(ctx, "failed nsDataAvailable(%s): %s", ns, err) } - periodsMerged, rowsMerged, err := coveragedb.NsDataMerged(ctx, "syzkaller", ns) + periodsMerged, rowsMerged, err := coveragedb.NsDataMerged(ctx, coverageDBClient, ns) if err != nil { log.Errorf(ctx, "failed coveragedb.NsDataMerged(%s): %s", ns, err) } @@ -154,7 +154,7 @@ func nsDataAvailable(ctx context.Context, ns string) ([]coveragedb.TimePeriod, [ func handleBatchCoverageClean(w http.ResponseWriter, r *http.Request) { ctx := context.Background() - totalDeleted, err := coveragedb.DeleteGarbage(ctx) + totalDeleted, err := coveragedb.DeleteGarbage(ctx, coverageDBClient) if err != nil { errMsg := fmt.Sprintf("failed to coveragedb.DeleteGarbage: %s", err.Error()) log.Errorf(ctx, "%s", errMsg) diff --git a/dashboard/app/config.go b/dashboard/app/config.go index 6d173cb19550..c71d09d8e88e 100644 --- a/dashboard/app/config.go +++ b/dashboard/app/config.go @@ -425,6 +425,7 @@ func installConfig(cfg *GlobalConfig) { initAPIHandlers() initKcidb() initBatchProcessors() + initCoverageDB() } var contextConfigKey = "Updated config (to be used during tests). Use only in tests!" diff --git a/dashboard/app/coverage.go b/dashboard/app/coverage.go index 7bc0d2a06660..efab2c91b6b1 100644 --- a/dashboard/app/coverage.go +++ b/dashboard/app/coverage.go @@ -8,6 +8,7 @@ import ( "fmt" "html/template" "net/http" + "os" "slices" "strconv" @@ -19,6 +20,33 @@ import ( "github.com/google/syzkaller/pkg/validator" ) +var coverageDBClient spannerclient.SpannerClient + +func initCoverageDB() { + projectID := os.Getenv("GOOGLE_CLOUD_PROJECT") + if projectID == "" { + // It is a test environment. + // Use SetCoverageDBClient to specify the coveragedb mock or emulator in every test. + return + } + var err error + coverageDBClient, err = spannerclient.NewClient(context.Background(), projectID) + if err != nil { + panic("spanner.NewClient: " + err.Error()) + } +} + +var keyCoverageDBClient = "coveragedb client key" + +func SetCoverageDBClient(ctx context.Context, client spannerclient.SpannerClient) context.Context { + return context.WithValue(ctx, &keyCoverageDBClient, client) +} + +func GetCoverageDBClient(ctx context.Context) spannerclient.SpannerClient { + client, _ := ctx.Value(&keyCoverageDBClient).(spannerclient.SpannerClient) + return client +} + type funcStyleBodyJS func( ctx context.Context, client spannerclient.SpannerClient, scope *cover.SelectScope, onlyUnique bool, sss, managers []string, @@ -37,6 +65,10 @@ func handleHeatmap(c context.Context, w http.ResponseWriter, r *http.Request, f if err != nil { return err } + nsConfig := getNsConfig(c, hdr.Namespace) + if nsConfig.Coverage == nil { + return ErrClientNotFound + } ss := r.FormValue("subsystem") manager := r.FormValue("manager") @@ -76,15 +108,9 @@ func handleHeatmap(c context.Context, w http.ResponseWriter, r *http.Request, f onlyUnique := r.FormValue("unique-only") == "1" - spannerClient, err := spannerclient.NewClient(c, "syzkaller") - if err != nil { - return fmt.Errorf("spanner.NewClient: %s", err.Error()) - } - defer spannerClient.Close() - var style template.CSS var body, js template.HTML - if style, body, js, err = f(c, spannerClient, + if style, body, js, err = f(c, GetCoverageDBClient(c), &cover.SelectScope{ Ns: hdr.Namespace, Subsystem: ss, @@ -147,24 +173,35 @@ func handleFileCoverage(c context.Context, w http.ResponseWriter, r *http.Reques } onlyUnique := r.FormValue("unique-only") == "1" mainNsRepo, _ := nsConfig.mainRepoBranch() - hitLines, hitCounts, err := coveragedb.ReadLinesHitCount(c, hdr.Namespace, targetCommit, manager, kernelFilePath, tp) + client := GetCoverageDBClient(c) + if client == nil { + return fmt.Errorf("spannerdb client is nil") + } + hitLines, hitCounts, err := coveragedb.ReadLinesHitCount(c, client, hdr.Namespace, targetCommit, kernelFilePath, manager, tp) covMap := cover.MakeCovMap(hitLines, hitCounts) if err != nil { return fmt.Errorf("coveragedb.ReadLinesHitCount(%s): %w", manager, err) } if onlyUnique { - allHitLines, allHitCounts, err := coveragedb.ReadLinesHitCount(c, hdr.Namespace, targetCommit, manager, kernelFilePath, tp) + // This request is expected to be made second by tests. + // Moving it to goroutine don't forget to change multiManagerCovDBFixture. + allHitLines, allHitCounts, err := coveragedb.ReadLinesHitCount(c, client, hdr.Namespace, targetCommit, kernelFilePath, "*", tp) if err != nil { return fmt.Errorf("coveragedb.ReadLinesHitCount(*): %w", err) } covMap = cover.UniqCoverage(cover.MakeCovMap(allHitLines, allHitCounts), covMap) } + webGit := getWebGit(c) // Get mock if available. + if webGit == nil { + webGit = covermerger.MakeWebGit(makeProxyURIProvider(nsConfig.Coverage.WebGitURI)) + } + content, err := cover.RendFileCoverage( mainNsRepo, targetCommit, kernelFilePath, - makeProxyURIProvider(nsConfig.Coverage.WebGitURI), + webGit, &covermerger.MergeResult{HitCounts: covMap}, cover.DefaultHTMLRenderConfig()) if err != nil { @@ -175,11 +212,26 @@ func handleFileCoverage(c context.Context, w http.ResponseWriter, r *http.Reques return nil } +var keyWebGit = "file content provider" + +func setWebGit(ctx context.Context, provider covermerger.FileVersProvider) context.Context { + return context.WithValue(ctx, &keyWebGit, provider) +} + +func getWebGit(ctx context.Context) covermerger.FileVersProvider { + res, _ := ctx.Value(&keyWebGit).(covermerger.FileVersProvider) + return res +} + func handleCoverageGraph(c context.Context, w http.ResponseWriter, r *http.Request) error { hdr, err := commonHeader(c, r, w, "") if err != nil { return err } + nsConfig := getNsConfig(c, hdr.Namespace) + if nsConfig.Coverage == nil { + return ErrClientNotFound + } periodType := r.FormValue("period") if periodType == "" { periodType = coveragedb.QuarterPeriod @@ -187,7 +239,7 @@ func handleCoverageGraph(c context.Context, w http.ResponseWriter, r *http.Reque if periodType != coveragedb.QuarterPeriod && periodType != coveragedb.MonthPeriod { return fmt.Errorf("only quarter and month are allowed, but received %s instead", periodType) } - hist, err := MergedCoverage(c, hdr.Namespace, periodType) + hist, err := MergedCoverage(c, GetCoverageDBClient(c), hdr.Namespace, periodType) if err != nil { return err } diff --git a/dashboard/app/coverage_test.go b/dashboard/app/coverage_test.go new file mode 100644 index 000000000000..c125264964a0 --- /dev/null +++ b/dashboard/app/coverage_test.go @@ -0,0 +1,167 @@ +// Copyright 2025 syzkaller project authors. All rights reserved. +// Use of this source code is governed by Apache 2 LICENSE that can be found in the LICENSE file. + +package main + +import ( + "strings" + "testing" + + "github.com/google/syzkaller/pkg/coveragedb" + "github.com/google/syzkaller/pkg/coveragedb/mocks" + "github.com/google/syzkaller/pkg/coveragedb/spannerclient" + "github.com/google/syzkaller/pkg/covermerger" + mergermocks "github.com/google/syzkaller/pkg/covermerger/mocks" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "google.golang.org/api/iterator" +) + +func TestFileCoverage(t *testing.T) { + tests := []struct { + name string + covDB func(t *testing.T) spannerclient.SpannerClient + fileProv covermerger.FileVersProvider + url string + wantInRes []string + }{ + { + name: "empty db", + covDB: func(t *testing.T) spannerclient.SpannerClient { return emptyCoverageDBFixture(t, 1) }, + fileProv: staticFileProvider(t), + url: "/test2/graph/coverage/file?dateto=2025-01-31&period=month&commit=c0e75905caf368e19aab585d20151500e750de89&filepath=virt/kvm/kvm_main.c", + wantInRes: []string{"1 line1"}, + }, + { + name: "regular db", + covDB: func(t *testing.T) spannerclient.SpannerClient { return coverageDBFixture(t) }, + fileProv: staticFileProvider(t), + url: "/test2/graph/coverage/file?dateto=2025-01-31&period=month&commit=c0e75905caf368e19aab585d20151500e750de89&filepath=virt/kvm/kvm_main.c", + wantInRes: []string{ + "4 1 line1", + "5 2 line2", + "6 3 line3"}, + }, + { + name: "multimanager db", + covDB: func(t *testing.T) spannerclient.SpannerClient { return multiManagerCovDBFixture(t) }, + fileProv: staticFileProvider(t), + url: "/test2/graph/coverage/file?dateto=2025-01-31&period=month&commit=c0e75905caf368e19aab585d20151500e750de89&filepath=virt/kvm/kvm_main.c&manager=special-cc-manager&unique-only=1", + wantInRes: []string{ + " 0 1 line1", // Covered, is not unique. + " 5 2 line2", // Covered and is unique. + " 3 line3", // Covered only by "*" managers. + }, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + c := NewCtx(t) + defer c.Close() + c.setCoverageMocks("test2", test.covDB(t), test.fileProv) + fileCovPage, err := c.GET(test.url) + assert.NoError(t, err) + got := string(fileCovPage) + for _, want := range test.wantInRes { + if !strings.Contains(got, want) { + t.Errorf(`"%s" wasn't found in "%s"'`, want, got) + } + } + }) + } +} + +func staticFileProvider(t *testing.T) covermerger.FileVersProvider { + m := mergermocks.NewFileVersProvider(t) + m.On("GetFileVersions", mock.Anything, mock.Anything). + Return(func(targetFilePath string, repoCommits ...covermerger.RepoCommit, + ) covermerger.FileVersions { + res := covermerger.FileVersions{} + for _, rc := range repoCommits { + res[rc] = `line1 +line2 +line3` + } + return res + }, nil) + return m +} + +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 coverageDBFixture(t *testing.T) spannerclient.SpannerClient { + mRowIt := newRowIteratorMock(t, []*coveragedb.LinesCoverage{{ + LinesInstrumented: []int64{1, 2, 3}, + HitCounts: []int64{4, 5, 6}, + }}) + + mTran := mocks.NewReadOnlyTransaction(t) + mTran.On("Query", mock.Anything, mock.Anything). + Return(mRowIt).Once() + + m := mocks.NewSpannerClient(t) + m.On("Single"). + Return(mTran).Once() + return m +} + +func multiManagerCovDBFixture(t *testing.T) spannerclient.SpannerClient { + mReadFullCoverageTran := mocks.NewReadOnlyTransaction(t) + mReadFullCoverageTran.On("Query", mock.Anything, mock.Anything). + Return(newRowIteratorMock(t, []*coveragedb.LinesCoverage{{ + LinesInstrumented: []int64{1, 2, 3}, + HitCounts: []int64{4, 5, 6}, + }})).Once() + + mReadPartialCoverageTran := mocks.NewReadOnlyTransaction(t) + mReadPartialCoverageTran.On("Query", mock.Anything, mock.Anything). + Return(newRowIteratorMock(t, []*coveragedb.LinesCoverage{{ + LinesInstrumented: []int64{1, 2}, + HitCounts: []int64{3, 5}, + }})).Once() + + m := mocks.NewSpannerClient(t) + // The order matters. Full coverage is fetched second. + m.On("Single"). + Return(mReadPartialCoverageTran).Once() + m.On("Single"). + Return(mReadFullCoverageTran).Once() + + return m +} + +func newRowIteratorMock(t *testing.T, cov []*coveragedb.LinesCoverage, +) *mocks.RowIterator { + m := mocks.NewRowIterator(t) + m.On("Stop").Once().Return() + for _, item := range cov { + mRow := mocks.NewRow(t) + mRow.On("ToStruct", mock.Anything). + Run(func(args mock.Arguments) { + arg := args.Get(0).(*coveragedb.LinesCoverage) + *arg = *item + }). + Return(nil).Once() + + m.On("Next"). + Return(mRow, nil).Once() + } + + m.On("Next"). + Return(nil, iterator.Done).Once() + return m +} diff --git a/dashboard/app/entities_spanner.go b/dashboard/app/entities_spanner.go index e3f3bad9e23d..c550ef92e217 100644 --- a/dashboard/app/entities_spanner.go +++ b/dashboard/app/entities_spanner.go @@ -6,7 +6,6 @@ package main import ( "context" "fmt" - "os" "cloud.google.com/go/civil" "cloud.google.com/go/spanner" @@ -24,14 +23,8 @@ type CoverageHistory struct { } // MergedCoverage uses dates, not time. -func MergedCoverage(ctx context.Context, ns, periodType string) (*CoverageHistory, error) { - projectID := os.Getenv("GOOGLE_CLOUD_PROJECT") - client, err := spannerclient.NewClient(ctx, projectID) - if err != nil { - return nil, fmt.Errorf("spanner.NewClient() failed: %s", err.Error()) - } - defer client.Close() - +func MergedCoverage(ctx context.Context, client spannerclient.SpannerClient, ns, periodType string, +) (*CoverageHistory, error) { minDays, maxDays, err := coveragedb.MinMaxDays(periodType) if err != nil { return nil, fmt.Errorf("coveragedb.MinMaxDays: %w", err) diff --git a/dashboard/app/handler.go b/dashboard/app/handler.go index e737c2b0b4ce..27817ab8c82e 100644 --- a/dashboard/app/handler.go +++ b/dashboard/app/handler.go @@ -32,6 +32,9 @@ func handlerWrapper(fn contextHandler) http.Handler { func handleContext(fn contextHandler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { c := appengine.NewContext(r) + if coverageDBClient != nil { // Nil in prod. + c = SetCoverageDBClient(c, coverageDBClient) + } c = context.WithValue(c, ¤tURLKey, r.URL.RequestURI()) authorizedUser, _ := userAccessLevel(currentUser(c), "", getConfig(c)) if !authorizedUser { diff --git a/dashboard/app/main.go b/dashboard/app/main.go index d765c8df96a8..071dc854438f 100644 --- a/dashboard/app/main.go +++ b/dashboard/app/main.go @@ -65,13 +65,11 @@ func initHTTPHandlers() { http.Handle("/"+ns+"/graph/fuzzing", handlerWrapper(handleGraphFuzzing)) http.Handle("/"+ns+"/graph/crashes", handlerWrapper(handleGraphCrashes)) http.Handle("/"+ns+"/graph/found-bugs", handlerWrapper(handleFoundBugsGraph)) - if nsConfig.Coverage != nil { - http.Handle("/"+ns+"/graph/coverage/file", handlerWrapper(handleFileCoverage)) - http.Handle("/"+ns+"/graph/coverage", handlerWrapper(handleCoverageGraph)) - http.Handle("/"+ns+"/graph/coverage_heatmap", handlerWrapper(handleCoverageHeatmap)) - if nsConfig.Subsystems.Service != nil { - http.Handle("/"+ns+"/graph/coverage_subsystems_heatmap", handlerWrapper(handleSubsystemsCoverageHeatmap)) - } + http.Handle("/"+ns+"/graph/coverage/file", handlerWrapper(handleFileCoverage)) + http.Handle("/"+ns+"/graph/coverage", handlerWrapper(handleCoverageGraph)) + http.Handle("/"+ns+"/graph/coverage_heatmap", handlerWrapper(handleCoverageHeatmap)) + if nsConfig.Subsystems.Service != nil { + http.Handle("/"+ns+"/graph/coverage_subsystems_heatmap", handlerWrapper(handleSubsystemsCoverageHeatmap)) } http.Handle("/"+ns+"/repos", handlerWrapper(handleRepos)) http.Handle("/"+ns+"/bug-summaries", handlerWrapper(handleBugSummaries)) diff --git a/dashboard/app/util_test.go b/dashboard/app/util_test.go index d2ed77e32a67..be7841b27fb0 100644 --- a/dashboard/app/util_test.go +++ b/dashboard/app/util_test.go @@ -28,6 +28,8 @@ import ( "github.com/google/go-cmp/cmp" "github.com/google/syzkaller/dashboard/api" "github.com/google/syzkaller/dashboard/dashapi" + "github.com/google/syzkaller/pkg/coveragedb/spannerclient" + "github.com/google/syzkaller/pkg/covermerger" "github.com/google/syzkaller/pkg/email" "github.com/google/syzkaller/pkg/subsystem" "google.golang.org/appengine/v2/aetest" @@ -226,6 +228,20 @@ func (c *Ctx) setSubsystems(ns string, list []*subsystem.Subsystem, rev int) { } } +func (c *Ctx) setCoverageMocks(ns string, dbClientMock spannerclient.SpannerClient, + fileProvMock covermerger.FileVersProvider) { + c.transformContext = func(ctx context.Context) context.Context { + newConfig := replaceNamespaceConfig(ctx, ns, func(cfg *Config) *Config { + ret := *cfg + ret.Coverage = &CoverageConfig{WebGitURI: "test-git"} + return &ret + }) + ctxWithSpanner := SetCoverageDBClient(ctx, dbClientMock) + ctxWithSpannerAndFileProvider := setWebGit(ctxWithSpanner, fileProvMock) + return contextWithConfig(ctxWithSpannerAndFileProvider, newConfig) + } +} + func (c *Ctx) setKernelRepos(ns string, list []KernelRepo) { c.transformContext = func(c context.Context) context.Context { newConfig := replaceNamespaceConfig(c, ns, func(cfg *Config) *Config { diff --git a/pkg/cover/file.go b/pkg/cover/file.go index c56f882045c6..ea6c326f0a05 100644 --- a/pkg/cover/file.go +++ b/pkg/cover/file.go @@ -40,10 +40,10 @@ func DefaultHTMLRenderConfig() *CoverageRenderConfig { } } -func RendFileCoverage(repo, forCommit, filePath string, proxy covermerger.FuncProxyURI, +func RendFileCoverage(repo, forCommit, filePath string, fileProvider covermerger.FileVersProvider, mr *covermerger.MergeResult, renderConfig *CoverageRenderConfig) (string, error) { repoCommit := covermerger.RepoCommit{Repo: repo, Commit: forCommit} - files, err := covermerger.MakeWebGit(proxy).GetFileVersions(filePath, repoCommit) + files, err := fileProvider.GetFileVersions(filePath, repoCommit) if err != nil { return "", fmt.Errorf("failed to GetFileVersions: %w", err) } diff --git a/pkg/cover/heatmap.go b/pkg/cover/heatmap.go index 04b7e6914b77..4cc212fadac6 100644 --- a/pkg/cover/heatmap.go +++ b/pkg/cover/heatmap.go @@ -116,13 +116,13 @@ type fileCoverageWithDetails struct { Subsystems []string } -type fileCoverageWithLineInfo struct { +type FileCoverageWithLineInfo struct { fileCoverageWithDetails LinesInstrumented []int64 HitCounts []int64 } -func (fc *fileCoverageWithLineInfo) CovMap() map[int]int64 { +func (fc *FileCoverageWithLineInfo) CovMap() map[int]int64 { return MakeCovMap(fc.LinesInstrumented, fc.HitCounts) } @@ -223,12 +223,12 @@ func readCoverage(iterManager spannerclient.RowIterator) ([]*fileCoverageWithDet func readCoverageUniq(full, mgr spannerclient.RowIterator, ) ([]*fileCoverageWithDetails, error) { eg, ctx := errgroup.WithContext(context.Background()) - fullCh := make(chan *fileCoverageWithLineInfo) + fullCh := make(chan *FileCoverageWithLineInfo) eg.Go(func() error { defer close(fullCh) return readIterToChan(ctx, full, fullCh) }) - partCh := make(chan *fileCoverageWithLineInfo) + partCh := make(chan *FileCoverageWithLineInfo) eg.Go(func() error { defer close(partCh) return readIterToChan(ctx, mgr, partCh) @@ -309,7 +309,7 @@ func UniqCoverage(fullCov, partCov map[int]int64) map[int]int64 { return res } -func readIterToChan[K fileCoverageWithLineInfo | fileCoverageWithDetails]( +func readIterToChan[K FileCoverageWithLineInfo | fileCoverageWithDetails]( ctx context.Context, iter spannerclient.RowIterator, ch chan<- *K) error { for { row, err := iter.Next() diff --git a/pkg/cover/heatmap_test.go b/pkg/cover/heatmap_test.go index ad1f9bf645ff..0f66256e4e8d 100644 --- a/pkg/cover/heatmap_test.go +++ b/pkg/cover/heatmap_test.go @@ -65,7 +65,7 @@ func TestFilesCoverageWithDetails(t *testing.T) { client: func() spannerclient.SpannerClient { return fullCoverageDBFixture( t, - []*fileCoverageWithLineInfo{ + []*FileCoverageWithLineInfo{ { fileCoverageWithDetails: fileCoverageWithDetails{ Filepath: "file1", @@ -98,7 +98,7 @@ func TestFilesCoverageWithDetails(t *testing.T) { client: func() spannerclient.SpannerClient { return fullCoverageDBFixture( t, - []*fileCoverageWithLineInfo{ + []*FileCoverageWithLineInfo{ { fileCoverageWithDetails: fileCoverageWithDetails{ Filepath: "file1", @@ -109,7 +109,7 @@ func TestFilesCoverageWithDetails(t *testing.T) { HitCounts: []int64{1, 1, 1}, }, }, - []*fileCoverageWithLineInfo{ + []*FileCoverageWithLineInfo{ { fileCoverageWithDetails: fileCoverageWithDetails{ Filepath: "file1", @@ -141,7 +141,7 @@ func TestFilesCoverageWithDetails(t *testing.T) { client: func() spannerclient.SpannerClient { return fullCoverageDBFixture( t, - []*fileCoverageWithLineInfo{ + []*FileCoverageWithLineInfo{ { fileCoverageWithDetails: fileCoverageWithDetails{ Filepath: "file1", @@ -152,7 +152,7 @@ func TestFilesCoverageWithDetails(t *testing.T) { HitCounts: []int64{3, 4, 5, 6, 7}, }, }, - []*fileCoverageWithLineInfo{ + []*FileCoverageWithLineInfo{ { fileCoverageWithDetails: fileCoverageWithDetails{ Filepath: "file1", @@ -212,7 +212,7 @@ func emptyCoverageDBFixture(t *testing.T, times int) spannerclient.SpannerClient } func fullCoverageDBFixture( - t *testing.T, full, partial []*fileCoverageWithLineInfo, + t *testing.T, full, partial []*FileCoverageWithLineInfo, ) spannerclient.SpannerClient { mPartialTran := mocks.NewReadOnlyTransaction(t) mPartialTran.On("Query", mock.Anything, mock.Anything). @@ -230,7 +230,7 @@ func fullCoverageDBFixture( return m } -func newRowIteratorMock(t *testing.T, events []*fileCoverageWithLineInfo, +func newRowIteratorMock(t *testing.T, events []*FileCoverageWithLineInfo, ) *mocks.RowIterator { m := mocks.NewRowIterator(t) m.On("Stop").Once().Return() @@ -238,7 +238,7 @@ func newRowIteratorMock(t *testing.T, events []*fileCoverageWithLineInfo, mRow := mocks.NewRow(t) mRow.On("ToStruct", mock.Anything). Run(func(args mock.Arguments) { - arg := args.Get(0).(*fileCoverageWithLineInfo) + arg := args.Get(0).(*FileCoverageWithLineInfo) *arg = *item }). Return(nil).Once() diff --git a/pkg/coveragedb/coveragedb.go b/pkg/coveragedb/coveragedb.go index 22e84d8ebd23..792d4ec79256 100644 --- a/pkg/coveragedb/coveragedb.go +++ b/pkg/coveragedb/coveragedb.go @@ -9,7 +9,6 @@ import ( "errors" "fmt" "io" - "os" "sync/atomic" "time" @@ -74,6 +73,9 @@ type MergedCoverageRecord struct { func SaveMergeResult(ctx context.Context, client spannerclient.SpannerClient, descr *HistoryRecord, dec *json.Decoder, sss []*subsystem.Subsystem) (int, error) { + if client == nil { + return 0, fmt.Errorf("nil spannerclient") + } var rowsCreated int ssMatcher := subsystem.MakePathMatcher(sss) ssCache := make(map[string][]string) @@ -117,7 +119,7 @@ func SaveMergeResult(ctx context.Context, client spannerclient.SpannerClient, de return rowsCreated, nil } -type linesCoverage struct { +type LinesCoverage struct { LinesInstrumented []int64 HitCounts []int64 } @@ -147,15 +149,8 @@ where } } -func ReadLinesHitCount(ctx context.Context, ns, commit, file, manager string, tp TimePeriod, +func ReadLinesHitCount(ctx context.Context, client spannerclient.SpannerClient, ns, commit, file, manager string, tp TimePeriod, ) ([]int64, []int64, error) { - projectID := os.Getenv("GOOGLE_CLOUD_PROJECT") - client, err := spannerclient.NewClient(ctx, projectID) - if err != nil { - return nil, nil, fmt.Errorf("spanner.NewClient: %w", err) - } - defer client.Close() - stmt := linesCoverageStmt(ns, file, commit, manager, tp) iter := client.Single().Query(ctx, stmt) defer iter.Stop() @@ -167,10 +162,13 @@ func ReadLinesHitCount(ctx context.Context, ns, commit, file, manager string, tp if err != nil { return nil, nil, fmt.Errorf("iter.Next: %w", err) } - var r linesCoverage + var r LinesCoverage if err = row.ToStruct(&r); err != nil { return nil, nil, fmt.Errorf("failed to row.ToStruct() spanner DB: %w", err) } + if _, err := iter.Next(); err != iterator.Done { + return nil, nil, fmt.Errorf("more than 1 line is available") + } return r.LinesInstrumented, r.HitCounts, nil } @@ -230,13 +228,11 @@ func fileSubsystems(filePath string, ssMatcher *subsystem.PathMatcher, ssCache m return sss } -func NsDataMerged(ctx context.Context, projectID, ns string) ([]TimePeriod, []int64, error) { - client, err := spannerclient.NewClient(ctx, projectID) - if err != nil { - return nil, nil, fmt.Errorf("spanner.NewClient() failed: %s", err.Error()) +func NsDataMerged(ctx context.Context, client spannerclient.SpannerClient, ns string, +) ([]TimePeriod, []int64, error) { + if client == nil { + return nil, nil, fmt.Errorf("nil spannerclient") } - defer client.Close() - stmt := spanner.Statement{ SQL: ` select @@ -285,13 +281,11 @@ func NsDataMerged(ctx context.Context, projectID, ns string) ([]TimePeriod, []in // Note that in case of an error during batch deletion, some files may be deleted but not counted in the total. // // Returns the number of orphaned file entries successfully deleted. -func DeleteGarbage(ctx context.Context) (int64, error) { +func DeleteGarbage(ctx context.Context, client spannerclient.SpannerClient) (int64, error) { batchSize := 10_000 - client, err := spannerclient.NewClient(ctx, os.Getenv("GOOGLE_CLOUD_PROJECT")) - if err != nil { - return 0, fmt.Errorf("coveragedb.NewClient: %w", err) + if client == nil { + return 0, fmt.Errorf("nil spannerclient") } - defer client.Close() iter := client.Single().Query(ctx, spanner.Statement{ SQL: `SELECT session, filepath @@ -328,7 +322,7 @@ func DeleteGarbage(ctx context.Context) (int64, error) { } } goSpannerDelete(ctx, batch, eg, client, &totalDeleted) - if err = eg.Wait(); err != nil { + if err := eg.Wait(); err != nil { return 0, fmt.Errorf("spanner.Delete: %w", err) } return totalDeleted.Load(), nil diff --git a/pkg/covermerger/covermerger_test.go b/pkg/covermerger/covermerger_test.go index 36f42fdc1bd2..ab2b1efc5b1d 100644 --- a/pkg/covermerger/covermerger_test.go +++ b/pkg/covermerger/covermerger_test.go @@ -95,7 +95,7 @@ func TestMergerdCoverageRecords(t *testing.T) { FilePath: "file.c", MergeResult: &MergeResult{ FileExists: true, - HitCounts: map[int]int{ + HitCounts: map[int]int64{ 1: 5, 2: 7, }, @@ -367,8 +367,8 @@ type fileVersProviderMock struct { } func (m *fileVersProviderMock) GetFileVersions(targetFilePath string, repoCommits ...RepoCommit, -) (fileVersions, error) { - res := make(fileVersions) +) (FileVersions, error) { + res := make(FileVersions) for _, repoCommit := range repoCommits { filePath := filepath.Join(m.Workdir, "repos", repoCommit.Commit, targetFilePath) if bytes, err := os.ReadFile(filePath); err == nil { diff --git a/pkg/covermerger/file_line_merger.go b/pkg/covermerger/file_line_merger.go index ebc747f47abc..817099a60786 100644 --- a/pkg/covermerger/file_line_merger.go +++ b/pkg/covermerger/file_line_merger.go @@ -5,7 +5,7 @@ package covermerger import "github.com/google/syzkaller/pkg/log" -func makeFileLineCoverMerger(fvs fileVersions, base RepoCommit) FileCoverageMerger { +func makeFileLineCoverMerger(fvs FileVersions, base RepoCommit) FileCoverageMerger { baseFile := "" baseFileExists := false for repoCommit, fv := range fvs { diff --git a/pkg/covermerger/mocks/FileVersProvider.go b/pkg/covermerger/mocks/FileVersProvider.go new file mode 100644 index 000000000000..69fadfa425c7 --- /dev/null +++ b/pkg/covermerger/mocks/FileVersProvider.go @@ -0,0 +1,64 @@ +// Code generated by mockery v2.45.1. DO NOT EDIT. + +package mocks + +import ( + covermerger "github.com/google/syzkaller/pkg/covermerger" + mock "github.com/stretchr/testify/mock" +) + +// FileVersProvider is an autogenerated mock type for the FileVersProvider type +type FileVersProvider struct { + mock.Mock +} + +// GetFileVersions provides a mock function with given fields: targetFilePath, repoCommits +func (_m *FileVersProvider) GetFileVersions(targetFilePath string, repoCommits ...covermerger.RepoCommit) (covermerger.FileVersions, error) { + _va := make([]interface{}, len(repoCommits)) + for _i := range repoCommits { + _va[_i] = repoCommits[_i] + } + var _ca []interface{} + _ca = append(_ca, targetFilePath) + _ca = append(_ca, _va...) + ret := _m.Called(_ca...) + + if len(ret) == 0 { + panic("no return value specified for GetFileVersions") + } + + var r0 covermerger.FileVersions + var r1 error + if rf, ok := ret.Get(0).(func(string, ...covermerger.RepoCommit) (covermerger.FileVersions, error)); ok { + return rf(targetFilePath, repoCommits...) + } + if rf, ok := ret.Get(0).(func(string, ...covermerger.RepoCommit) covermerger.FileVersions); ok { + r0 = rf(targetFilePath, repoCommits...) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(covermerger.FileVersions) + } + } + + if rf, ok := ret.Get(1).(func(string, ...covermerger.RepoCommit) error); ok { + r1 = rf(targetFilePath, repoCommits...) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// NewFileVersProvider creates a new instance of FileVersProvider. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +// The first argument is typically a *testing.T value. +func NewFileVersProvider(t interface { + mock.TestingT + Cleanup(func()) +}) *FileVersProvider { + mock := &FileVersProvider{} + mock.Mock.Test(t) + + t.Cleanup(func() { mock.AssertExpectations(t) }) + + return mock +} diff --git a/pkg/covermerger/provider_monorepo.go b/pkg/covermerger/provider_monorepo.go index e6c67d1d1901..73a08786a1c6 100644 --- a/pkg/covermerger/provider_monorepo.go +++ b/pkg/covermerger/provider_monorepo.go @@ -3,6 +3,8 @@ package covermerger +//go:generate ../../tools/mockery.sh --name FileVersProvider -r + import ( "fmt" "path/filepath" @@ -15,7 +17,7 @@ import ( type FileVersProvider interface { GetFileVersions(targetFilePath string, repoCommits ...RepoCommit, - ) (fileVersions, error) + ) (FileVersions, error) } type monoRepo struct { @@ -24,10 +26,10 @@ type monoRepo struct { repo vcs.Repo } -type fileVersions map[RepoCommit]string +type FileVersions map[RepoCommit]string func (mr *monoRepo) GetFileVersions(targetFilePath string, repoCommits ...RepoCommit, -) (fileVersions, error) { +) (FileVersions, error) { mr.mu.RLock() if !mr.allRepoCommitsPresent(repoCommits) { mr.mu.RUnlock() @@ -35,7 +37,7 @@ func (mr *monoRepo) GetFileVersions(targetFilePath string, repoCommits ...RepoCo mr.mu.RLock() } defer mr.mu.RUnlock() - res := make(fileVersions) + res := make(FileVersions) for _, repoCommit := range repoCommits { fileBytes, err := mr.repo.Object(targetFilePath, repoCommit.Commit) // It is ok if some file doesn't exist. It means we have repo FS diff. diff --git a/pkg/covermerger/provider_web.go b/pkg/covermerger/provider_web.go index 43bfee7e0832..554ee3f97fa8 100644 --- a/pkg/covermerger/provider_web.go +++ b/pkg/covermerger/provider_web.go @@ -18,8 +18,8 @@ type webGit struct { } func (mr *webGit) GetFileVersions(targetFilePath string, repoCommits ...RepoCommit, -) (fileVersions, error) { - res := make(fileVersions) +) (FileVersions, error) { + res := make(FileVersions) for _, repoCommit := range repoCommits { fileBytes, err := mr.loadFile(targetFilePath, repoCommit.Repo, repoCommit.Commit) // It is ok if some file doesn't exist. It means we have repo FS diff. diff --git a/pkg/validator/validator.go b/pkg/validator/validator.go index b9031302aa7d..12224963ec59 100644 --- a/pkg/validator/validator.go +++ b/pkg/validator/validator.go @@ -74,7 +74,7 @@ var ( CommitHash = makeCombinedStrFunc("not a hash", AlphaNumeric, makeStrLenFunc("len is not 40", 40)) KernelFilePath = makeStrReFunc("not a kernel file path", "^[./_a-zA-Z0-9-]*$") NamespaceName = makeStrReFunc("not a namespace name", "^[a-zA-Z0-9_.-]{4,32}$") - ManagerName = makeStrReFunc("not a manager name", "^ci[a-z0-9-]*$") + ManagerName = makeStrReFunc("not a manager name", "^[a-z0-9-]*$") DashClientName = makeStrReFunc("not a dashboard client name", "^[a-zA-Z0-9_.-]{4,100}$") DashClientKey = makeStrReFunc("not a dashboard client key", "^([a-zA-Z0-9]{16,128})|("+regexp.QuoteMeta(auth.OauthMagic)+".*)$") diff --git a/tools/syz-cover/syz-cover.go b/tools/syz-cover/syz-cover.go index bbdb4bd96be4..4a72b9bf6479 100644 --- a/tools/syz-cover/syz-cover.go +++ b/tools/syz-cover/syz-cover.go @@ -38,6 +38,7 @@ import ( "github.com/google/syzkaller/pkg/cover" "github.com/google/syzkaller/pkg/cover/backend" "github.com/google/syzkaller/pkg/coveragedb" + "github.com/google/syzkaller/pkg/covermerger" "github.com/google/syzkaller/pkg/log" "github.com/google/syzkaller/pkg/mgrconfig" "github.com/google/syzkaller/pkg/osutil" @@ -92,7 +93,7 @@ func toolFileCover() { *flagRepo, *flagCommit, *flagForFile, - nil, // no proxy - get files directly from WebGits + covermerger.MakeWebGit(nil), // get files directly from WebGits mr, config, )