From 81095845401a2ac64bc5ee43a4f9b013207d3b7e Mon Sep 17 00:00:00 2001 From: Jonathan Amsterdam Date: Thu, 9 Apr 2026 16:55:25 -0400 Subject: internal/api: test ServePackage with db Test the ServePackage function against a real database, as well as the fake data source. - Modules and packages must be redistributable, or the DB strips the documentation. - Documentation must contain source code. We must temporarily skip the dependency tests, to reduce the diffs on this change. The test is still important, because it asserts that the pkgsite command has only a few dependencies. To re-establish it, we will move the API tests to internal/testing/api. Change-Id: I67243ea2345c5e735edbdd6ee5bf2349a6b1eb60 Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/765502 kokoro-CI: kokoro LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com Reviewed-by: Ethan Lee Reviewed-by: Hyang-Ah Hana Kim --- internal/api/api_test.go | 102 ++++++++++++++++++--------- internal/log/log.go | 5 +- internal/log/log_test.go | 6 +- internal/tests/deps/cmd_pkgsite_deps_test.go | 1 + 4 files changed, 75 insertions(+), 39 deletions(-) diff --git a/internal/api/api_test.go b/internal/api/api_test.go index 64b891c7..1c233a7c 100644 --- a/internal/api/api_test.go +++ b/internal/api/api_test.go @@ -11,13 +11,17 @@ import ( "errors" "net/http" "net/http/httptest" + "os" "strconv" "testing" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" "golang.org/x/pkgsite/internal" + "golang.org/x/pkgsite/internal/derrors" + "golang.org/x/pkgsite/internal/log" "golang.org/x/pkgsite/internal/osv" + "golang.org/x/pkgsite/internal/postgres" "golang.org/x/pkgsite/internal/testing/fakedatasource" "golang.org/x/pkgsite/internal/testing/sample" "golang.org/x/pkgsite/internal/vuln" @@ -40,8 +44,46 @@ func (ds fallbackDataSource) GetUnitMeta(ctx context.Context, path, requestedMod } func TestServePackage(t *testing.T) { + t.Run("fake", func(t *testing.T) { + ds := fakedatasource.New() + mustInsert := func(ctx context.Context, m *internal.Module, _ bool) { + ds.MustInsertModule(ctx, m) + } + testServePackage(t, ds, mustInsert) + }) + t.Run("db", func(t *testing.T) { + orig := log.GetLevel() + t.Cleanup(func() { log.SetLevel(orig.String()) }) + log.SetLevel("Info") + const testDB = "pkgsite_api" + db, err := postgres.SetupTestDB(testDB) + if err != nil { + if errors.Is(err, derrors.NotFound) && os.Getenv("GO_DISCOVERY_TESTDB") != "true" { + t.Skipf("could not connect to DB (see doc/postgres.md to set up): %v", err) + } + t.Fatalf("setting up DB: %v", err) + } + defer db.Close() + mustInsert := func(ctx context.Context, m *internal.Module, latest bool) { + for _, u := range m.Units { + if !u.IsRedistributable { + t.Logf("unit %s is not redistributable; DB will strip documentation", u.Path) + } + } + if latest { + postgres.MustInsertModule(ctx, t, db, m) + } else { + postgres.MustInsertModuleNotLatest(ctx, t, db, m) + } + } + testServePackage(t, db, mustInsert) + }) +} + +func testServePackage(t *testing.T, ds internal.DataSource, mustInsertModule func(context.Context, *internal.Module, bool)) { + // mustInsert must not be called from subtests, because it captures + // a parent testing.T. ctx := context.Background() - ds := fakedatasource.New() const ( pkgPath = "example.com/a/b" @@ -50,7 +92,7 @@ func TestServePackage(t *testing.T) { version = "v1.2.3" ) - ds.MustInsertModule(ctx, &internal.Module{ + mustInsertModule(ctx, &internal.Module{ ModuleInfo: internal.ModuleInfo{ ModulePath: "example.com", Version: version, @@ -61,48 +103,46 @@ func TestServePackage(t *testing.T) { UnitMeta: internal.UnitMeta{ Path: "example.com/pkg", ModuleInfo: internal.ModuleInfo{ - ModulePath: "example.com", - Version: version, - LatestVersion: "v1.2.4", + ModulePath: "example.com", + Version: version, + LatestVersion: "v1.2.4", + IsRedistributable: true, }, Name: "pkg", }, - Documentation: []*internal.Documentation{sample.Documentation("linux", "amd64", sample.DocContents)}, - Licenses: sample.LicenseMetadata(), - Imports: []string{pkgPath}, + Documentation: []*internal.Documentation{sample.Documentation("linux", "amd64", sample.DocContents)}, + Licenses: sample.LicenseMetadata(), + Imports: []string{pkgPath}, + IsRedistributable: true, }}, - }) + }, false) for _, mp := range []string{modulePath1, modulePath2} { u := &internal.Unit{ UnitMeta: internal.UnitMeta{ Path: pkgPath, ModuleInfo: internal.ModuleInfo{ - ModulePath: mp, - Version: version, - LatestVersion: version, + ModulePath: mp, + Version: version, + LatestVersion: version, + IsRedistributable: true, }, Name: "b", }, - Documentation: []*internal.Documentation{ - { - GOOS: "linux", - GOARCH: "amd64", - Synopsis: "Synopsis for " + mp, - }, - }, + Documentation: []*internal.Documentation{sample.Documentation("linux", "amd64", sample.DocContents)}, + IsRedistributable: true, } - ds.MustInsertModule(ctx, &internal.Module{ + mustInsertModule(ctx, &internal.Module{ ModuleInfo: internal.ModuleInfo{ ModulePath: mp, Version: version, LatestVersion: version, }, Units: []*internal.Unit{u}, - }) + }, true) } - ds.MustInsertModule(ctx, &internal.Module{ + mustInsertModule(ctx, &internal.Module{ ModuleInfo: internal.ModuleInfo{ ModulePath: "example.com", Version: "v1.2.4", @@ -118,15 +158,12 @@ func TestServePackage(t *testing.T) { }, Name: "pkg", }, - Documentation: []*internal.Documentation{{ - GOOS: "linux", - GOARCH: "amd64", - Synopsis: "Basic synopsis", - }}, + Documentation: []*internal.Documentation{sample.Documentation("linux", "amd64", sample.DocContents)}, + IsRedistributable: true, }}, - }) + }, true) - ds.MustInsertModule(ctx, &internal.Module{ + mustInsertModule(ctx, &internal.Module{ ModuleInfo: internal.ModuleInfo{ ModulePath: "example.com/ex", Version: "v1.0.0", @@ -142,6 +179,7 @@ func TestServePackage(t *testing.T) { }, Name: "pkg", }, + IsRedistributable: true, Documentation: []*internal.Documentation{sample.DocumentationWithExamples("linux", "amd64", "", ` import "fmt" func Example() { @@ -150,7 +188,7 @@ func TestServePackage(t *testing.T) { } `)}, }}, - }) + }, true) for _, test := range []struct { name string @@ -194,7 +232,7 @@ func TestServePackage(t *testing.T) { Path: pkgPath, ModulePath: modulePath1, ModuleVersion: version, - Synopsis: "Synopsis for " + modulePath1, + Synopsis: "This is a package synopsis for GOOS=linux, GOARCH=amd64", IsLatest: true, GOOS: "linux", GOARCH: "amd64", @@ -222,7 +260,7 @@ func TestServePackage(t *testing.T) { Path: "example.com/pkg", ModulePath: "example.com", ModuleVersion: "v1.2.4", - Synopsis: "Basic synopsis", + Synopsis: "This is a package synopsis for GOOS=linux, GOARCH=amd64", IsLatest: true, GOOS: "linux", GOARCH: "amd64", diff --git a/internal/log/log.go b/internal/log/log.go index 60c9371a..8238cae2 100644 --- a/internal/log/log.go +++ b/internal/log/log.go @@ -67,7 +67,7 @@ func SetLevel(v string) { currentLevel = toLevel(v) } -func getLevel() Severity { +func GetLevel() Severity { mu.Lock() defer mu.Unlock() return currentLevel @@ -87,7 +87,6 @@ func (stdlibLogger) Log(ctx context.Context, s Severity, payload any) { extra = " (" + strings.Join(extras, ", ") + ")" } log.Printf("%s%s: %+v", s, extra, payload) - } func (stdlibLogger) Flush() {} @@ -151,7 +150,7 @@ func Fatal(ctx context.Context, arg any) { } func doLog(ctx context.Context, s Severity, payload any) { - if getLevel() > s { + if GetLevel() > s { return } mu.Lock() diff --git a/internal/log/log_test.go b/internal/log/log_test.go index 791bd775..d2ef9d53 100644 --- a/internal/log/log_test.go +++ b/internal/log/log_test.go @@ -19,7 +19,7 @@ const ( // Do not run in parallel. It overrides currentLevel. func TestSetLogLevel(t *testing.T) { - oldLevel := getLevel() + oldLevel := GetLevel() defer func() { currentLevel = oldLevel }() tests := []struct { @@ -38,7 +38,7 @@ func TestSetLogLevel(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { SetLevel(test.newLevel) - gotLevel := getLevel() + gotLevel := GetLevel() if test.wantLevel != gotLevel { t.Errorf("Error: want=%s, got=%s", test.wantLevel, gotLevel) } @@ -69,7 +69,6 @@ func TestLogLevel(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - test.logFunc(context.Background(), test.logMsg) logs := logger.(*mockLogger).logs got := strings.Contains(logs, test.logMsg) @@ -101,7 +100,6 @@ func TestDefaultLogLevel(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - test.logFunc(context.Background(), test.logMsg) logs := logger.(*mockLogger).logs diff --git a/internal/tests/deps/cmd_pkgsite_deps_test.go b/internal/tests/deps/cmd_pkgsite_deps_test.go index c82c4074..82c8e176 100644 --- a/internal/tests/deps/cmd_pkgsite_deps_test.go +++ b/internal/tests/deps/cmd_pkgsite_deps_test.go @@ -32,6 +32,7 @@ var additionalAllowedTestModDeps = map[string]bool{ } func TestCmdPkgsiteDeps(t *testing.T) { + t.Skip("temporarily skipped until https://go.dev/cl/765502 is merged; then we will move API tests to testing/api") testenv.MustHaveExecPath(t, "go") // First, list all dependencies of pkgsite. -- cgit v1.3