From 346bbc9a9a98c85fbb583ad0bbcb6b70f3217fe9 Mon Sep 17 00:00:00 2001 From: Herko Lategan Date: Thu, 23 Mar 2023 11:16:11 +0000 Subject: [PATCH] testutils: move default test tenant message In order to reduce logging noise but still inform test authors of the default test tenant, the message has been moved to where there is a `testing.TB` interface. Epic: CRDB-18499 --- pkg/cli/testutils.go | 4 +-- pkg/cmd/reduce/reduce/reducesql/BUILD.bazel | 7 ++++- pkg/cmd/reduce/reduce/reducesql/main_test.go | 25 ++++++++++++++++++ .../reduce/reduce/reducesql/reducesql_test.go | 26 +++++++++---------- pkg/server/BUILD.bazel | 2 -- pkg/server/server_test.go | 8 +++--- pkg/server/status_test.go | 2 +- pkg/server/testserver.go | 12 --------- pkg/testutils/serverutils/test_server_shim.go | 18 +++++++++++-- pkg/testutils/testcluster/testcluster.go | 4 +++ 10 files changed, 70 insertions(+), 38 deletions(-) create mode 100644 pkg/cmd/reduce/reduce/reducesql/main_test.go diff --git a/pkg/cli/testutils.go b/pkg/cli/testutils.go index 6dedcd76b628..f472485a906e 100644 --- a/pkg/cli/testutils.go +++ b/pkg/cli/testutils.go @@ -152,7 +152,7 @@ func newCLITestWithArgs(params TestCLIParams, argsFn func(args *base.TestServerA if params.NoNodelocal { args.ExternalIODir = "" } - s, err := serverutils.StartServerRaw(args) + s, err := serverutils.StartServerRaw(params.T, args) if err != nil { c.fail(err) } @@ -208,7 +208,7 @@ func (c *TestCLI) stopServer() { func (c *TestCLI) RestartServer(params TestCLIParams) { c.stopServer() log.Info(context.Background(), "restarting server") - s, err := serverutils.StartServerRaw(base.TestServerArgs{ + s, err := serverutils.StartServerRaw(params.T, base.TestServerArgs{ Insecure: params.Insecure, SSLCertsDir: c.certsDir, StoreSpecs: params.StoreSpecs, diff --git a/pkg/cmd/reduce/reduce/reducesql/BUILD.bazel b/pkg/cmd/reduce/reduce/reducesql/BUILD.bazel index 31bcf3fac4aa..63a722bcb93b 100644 --- a/pkg/cmd/reduce/reduce/reducesql/BUILD.bazel +++ b/pkg/cmd/reduce/reduce/reducesql/BUILD.bazel @@ -17,7 +17,10 @@ go_library( go_test( name = "reducesql_test", size = "small", - srcs = ["reducesql_test.go"], + srcs = [ + "main_test.go", + "reducesql_test.go", + ], args = ["-test.timeout=55s"], data = glob(["testdata/**"]), deps = [ @@ -26,8 +29,10 @@ go_test( "//pkg/cmd/reduce/reduce", "//pkg/security/username", "//pkg/server", + "//pkg/testutils/serverutils", "//pkg/testutils/skip", "@com_github_jackc_pgx_v4//:pgx", + "@com_github_stretchr_testify//require", ], ) diff --git a/pkg/cmd/reduce/reduce/reducesql/main_test.go b/pkg/cmd/reduce/reduce/reducesql/main_test.go new file mode 100644 index 000000000000..88c1d4c7bc38 --- /dev/null +++ b/pkg/cmd/reduce/reduce/reducesql/main_test.go @@ -0,0 +1,25 @@ +// Copyright 2023 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package reducesql_test + +import ( + "os" + "testing" + + "github.com/cockroachdb/cockroach/pkg/server" + "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" +) + +func TestMain(m *testing.M) { + serverutils.InitTestServerFactory(server.TestServerFactory) + code := m.Run() + os.Exit(code) +} diff --git a/pkg/cmd/reduce/reduce/reducesql/reducesql_test.go b/pkg/cmd/reduce/reduce/reducesql/reducesql_test.go index 8296cd7a101d..67e085f0a237 100644 --- a/pkg/cmd/reduce/reduce/reducesql/reducesql_test.go +++ b/pkg/cmd/reduce/reduce/reducesql/reducesql_test.go @@ -21,9 +21,10 @@ import ( "github.com/cockroachdb/cockroach/pkg/cmd/reduce/reduce" "github.com/cockroachdb/cockroach/pkg/cmd/reduce/reduce/reducesql" "github.com/cockroachdb/cockroach/pkg/security/username" - "github.com/cockroachdb/cockroach/pkg/server" + "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" "github.com/cockroachdb/cockroach/pkg/testutils/skip" "github.com/jackc/pgx/v4" + "github.com/stretchr/testify/require" ) var printUnknown = flag.Bool("unknown", false, "print unknown types during walk") @@ -33,24 +34,23 @@ func TestReduceSQL(t *testing.T) { skip.IgnoreLint(t, "unnecessary") reducesql.LogUnknown = *printUnknown - reduce.Walk(t, "testdata", reducesql.Pretty, isInterestingSQL, reduce.ModeInteresting, + isInterestingSQLWrapper := func(contains string) reduce.InterestingFn { + return isInterestingSQL(t, contains) + } + + reduce.Walk(t, "testdata", reducesql.Pretty, isInterestingSQLWrapper, reduce.ModeInteresting, nil /* chunkReducer */, reducesql.SQLPasses) } -func isInterestingSQL(contains string) reduce.InterestingFn { +func isInterestingSQL(t *testing.T, contains string) reduce.InterestingFn { return func(ctx context.Context, f string) (bool, func()) { args := base.TestServerArgs{ Insecure: true, } - ts, err := server.TestServerFactory.New(args) - if err != nil { - panic(err) - } - serv := ts.(*server.TestServer) + + serv, err := serverutils.StartServerRaw(t, args) + require.NoError(t, err) defer serv.Stopper().Stop(ctx) - if err := serv.Start(context.Background()); err != nil { - panic(err) - } options := url.Values{} options.Add("sslmode", "disable") @@ -62,9 +62,7 @@ func isInterestingSQL(contains string) reduce.InterestingFn { } db, err := pgx.Connect(ctx, url.String()) - if err != nil { - panic(err) - } + require.NoError(t, err) _, err = db.Exec(ctx, f) if err == nil { return false, nil diff --git a/pkg/server/BUILD.bazel b/pkg/server/BUILD.bazel index 318a14533226..634dd7f98e1e 100644 --- a/pkg/server/BUILD.bazel +++ b/pkg/server/BUILD.bazel @@ -263,7 +263,6 @@ go_library( "//pkg/storage/enginepb", "//pkg/storage/fs", "//pkg/testutils/serverutils", - "//pkg/testutils/skip", "//pkg/ts", "//pkg/ts/catalog", "//pkg/ui", @@ -289,7 +288,6 @@ go_library( "//pkg/util/log/eventpb", "//pkg/util/log/logcrash", "//pkg/util/log/logpb", - "//pkg/util/log/severity", "//pkg/util/metric", "//pkg/util/mon", "//pkg/util/netutil", diff --git a/pkg/server/server_test.go b/pkg/server/server_test.go index e0666ad4272b..9870d9c34616 100644 --- a/pkg/server/server_test.go +++ b/pkg/server/server_test.go @@ -73,7 +73,7 @@ import ( func TestSelfBootstrap(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - s, err := serverutils.StartServerRaw(base.TestServerArgs{}) + s, err := serverutils.StartServerRaw(t, base.TestServerArgs{}) if err != nil { t.Fatal(err) } @@ -88,7 +88,7 @@ func TestPanicRecovery(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - s, err := serverutils.StartServerRaw(base.TestServerArgs{}) + s, err := serverutils.StartServerRaw(t, base.TestServerArgs{}) require.NoError(t, err) defer s.Stopper().Stop(context.Background()) ts := s.(*TestServer) @@ -129,7 +129,7 @@ func TestHealthCheck(t *testing.T) { cfg := zonepb.DefaultZoneConfig() cfg.NumReplicas = proto.Int32(1) - s, err := serverutils.StartServerRaw(base.TestServerArgs{ + s, err := serverutils.StartServerRaw(t, base.TestServerArgs{ Knobs: base.TestingKnobs{ Server: &TestingKnobs{ DefaultZoneConfigOverride: &cfg, @@ -419,7 +419,7 @@ func TestListenerFileCreation(t *testing.T) { dir, cleanupFn := testutils.TempDir(t) defer cleanupFn() - s, err := serverutils.StartServerRaw(base.TestServerArgs{ + s, err := serverutils.StartServerRaw(t, base.TestServerArgs{ StoreSpecs: []base.StoreSpec{{ Path: dir, }}, diff --git a/pkg/server/status_test.go b/pkg/server/status_test.go index e42e8f6e0bdd..899a37976482 100644 --- a/pkg/server/status_test.go +++ b/pkg/server/status_test.go @@ -267,7 +267,7 @@ func TestStatusEngineStatsJson(t *testing.T) { dir, cleanupFn := testutils.TempDir(t) defer cleanupFn() - s, err := serverutils.StartServerRaw(base.TestServerArgs{ + s, err := serverutils.StartServerRaw(t, base.TestServerArgs{ StoreSpecs: []base.StoreSpec{{ Path: dir, }}, diff --git a/pkg/server/testserver.go b/pkg/server/testserver.go index 0716a5601576..6e7e33df4166 100644 --- a/pkg/server/testserver.go +++ b/pkg/server/testserver.go @@ -52,14 +52,12 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" "github.com/cockroachdb/cockroach/pkg/storage" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" - "github.com/cockroachdb/cockroach/pkg/testutils/skip" "github.com/cockroachdb/cockroach/pkg/ts" "github.com/cockroachdb/cockroach/pkg/upgrade/upgradebase" "github.com/cockroachdb/cockroach/pkg/util" "github.com/cockroachdb/cockroach/pkg/util/admission" "github.com/cockroachdb/cockroach/pkg/util/hlc" "github.com/cockroachdb/cockroach/pkg/util/log" - "github.com/cockroachdb/cockroach/pkg/util/log/severity" "github.com/cockroachdb/cockroach/pkg/util/metric" addrutil "github.com/cockroachdb/cockroach/pkg/util/netutil/addr" "github.com/cockroachdb/cockroach/pkg/util/retry" @@ -579,16 +577,6 @@ func (ts *TestServer) maybeStartDefaultTestTenant(ctx context.Context) error { if len(ts.testTenants) == 0 { ts.testTenants = make([]serverutils.TestTenantInterface, 1) ts.testTenants[0] = tenant - - if !skip.UnderBench() { - // Now that we've started the first tenant, log this fact for easier - // debugging. Skip the logging if we're running a benchmark (because - // these INFO messages break the benchstat utility). - log.Shout(context.Background(), severity.INFO, - "Running test with the default test tenant. "+ - "If you are only seeing a test case failure when this message appears, there may be a "+ - "problem with your test case running within tenants.") - } } else { // We restrict the creation of multiple default tenants because if // we allow for more than one to be created, it's not clear what we diff --git a/pkg/testutils/serverutils/test_server_shim.go b/pkg/testutils/serverutils/test_server_shim.go index 1197f9004914..dbdab90e8f55 100644 --- a/pkg/testutils/serverutils/test_server_shim.go +++ b/pkg/testutils/serverutils/test_server_shim.go @@ -44,6 +44,12 @@ import ( "github.com/cockroachdb/errors" ) +// DefaultTestTenantMessage is a message that is printed when a test is run +// with the default test tenant. This is useful for debugging test failures. +const DefaultTestTenantMessage = "Running test with the default test tenant. " + + "If you are only seeing a test case failure when this message appears, there may be a " + + "problem with your test case running within tenants." + // TenantModeFlagName is the exported name of the tenantMode flag, for use // in other packages. const TenantModeFlagName = "tenantMode" @@ -330,6 +336,11 @@ func StartServer( if err := s.Start(context.Background()); err != nil { t.Fatalf("%+v", err) } + + if s.StartedDefaultTestTenant() { + t.Log(DefaultTestTenantMessage) + } + goDB := OpenDBConn( t, s.ServingSQLAddr(), params.UseDatabase, params.Insecure, s.Stopper()) @@ -397,9 +408,9 @@ func OpenDBConn( } // StartServerRaw creates and starts a TestServer. -// Generally StartServer() should be used. However this function can be used +// Generally StartServer() should be used. However, this function can be used // directly when opening a connection to the server is not desired. -func StartServerRaw(args base.TestServerArgs) (TestServerInterface, error) { +func StartServerRaw(t testing.TB, args base.TestServerArgs) (TestServerInterface, error) { server, err := NewServer(args) if err != nil { return nil, err @@ -407,6 +418,9 @@ func StartServerRaw(args base.TestServerArgs) (TestServerInterface, error) { if err := server.Start(context.Background()); err != nil { return nil, err } + if server.StartedDefaultTestTenant() { + t.Log(DefaultTestTenantMessage) + } return server, nil } diff --git a/pkg/testutils/testcluster/testcluster.go b/pkg/testutils/testcluster/testcluster.go index 157133edda17..40c224c229e8 100644 --- a/pkg/testutils/testcluster/testcluster.go +++ b/pkg/testutils/testcluster/testcluster.go @@ -396,6 +396,10 @@ func (tc *TestCluster) Start(t testing.TB) { } } + if tc.StartedDefaultTestTenant() { + t.Log(serverutils.DefaultTestTenantMessage) + } + if tc.clusterArgs.ParallelStart { for i := 0; i < nodes; i++ { if err := <-errCh; err != nil {