Merge #109993

109993: serverutils: better doc the TestServerInterface r=stevendanna,herkolategan a=knz This PR goes hand-in-hand with the following new doc page, which should be reviewed as well: https://cockroachlabs.atlassian.net/wiki/spaces/CRDB/pages/3155427384/TestServer+and+TestCluster Epic: CRDB-18499 Release note: None Co-authored-by: Raphael 'kena' Poss <[email protected]>
cockroachdb · Sep 5, 2023 · 6e0dfb1 · 6e0dfb1
2 parents 8ee0bc7 + 6beb14c
commit 6e0dfb1
Show file tree

Hide file tree

Showing 3 changed files with 25 additions and 24 deletions.
diff --git a/pkg/testutils/serverutils/api.go b/pkg/testutils/serverutils/api.go
@@ -65,8 +65,15 @@ type TestServerInterface interface {
 
 	// ApplicationLayerInterface is implemented by TestServerInterface
 	// for backward-compatibility with existing test code.
-	// New code should spell out their intent clearly by calling
-	// the .ApplicationLayer() or .SystemLayer() methods directly.
+	//
+	// It is CURRENTLY equivalent to .SystemLayer() however this is
+	// a misdesign and results in poor test semantics.
+	//
+	// See: https://go.crdb.dev/p/testserver-api-problem
+	//
+	// New tests should spell out their intent clearly by calling the
+	// .ApplicationLayer() (preferred) or .SystemLayer() methods
+	// directly.
 	ApplicationLayerInterface
 
 	// TenantControlInterface is implemented by TestServerInterface
@@ -78,7 +85,7 @@ type TestServerInterface interface {
 	// ApplicationLayer returns the interface to the application layer that is
 	// exercised by the test. Depending on how the test server is started
 	// and (optionally) randomization, this can be either the SQL layer
-	// of a secondary tenant or that of the system tenant.
+	// of a virtual cluster or that of the system interface.
 	ApplicationLayer() ApplicationLayerInterface
 
 	// SystemLayer returns the interface to the application layer
@@ -137,8 +144,7 @@ type TestServerController interface {
 
 // ApplicationLayerInterface defines accessors to the application
 // layer of a test server. Tests written against this interface are
-// effectively agnostic to whether they use a secondary tenant or not.
-// This interface is implemented by server.Test{Tenant,Server}.
+// effectively agnostic to whether they use a virtual cluster or not.
 type ApplicationLayerInterface interface {
 	// Readiness returns true when the server is ready, that is,
 	// when it is accepting connections and it is not draining.
@@ -458,7 +464,7 @@ type ApplicationLayerInterface interface {
 // start the SQL and HTTP service for secondary tenants (virtual
 // clusters).
 type TenantControlInterface interface {
-	// StartSharedProcessTenant starts the service for a secondary tenant
+	// StartSharedProcessTenant starts the service for a virtual cluster
 	// using the special configuration we define for shared-process deployments.
 	//
 	// args.TenantName must be specified. If a tenant with that name already
@@ -473,7 +479,7 @@ type TenantControlInterface interface {
 		ctx context.Context, args base.TestSharedProcessTenantArgs,
 	) (ApplicationLayerInterface, *gosql.DB, error)
 
-	// StartTenant starts the service for a secondary tenant using the special
+	// StartTenant starts the service for a virtual cluster using the special
 	// configuration we define for separate-process deployments. This incidentally
 	// is also the configuration we use in CC Serverless.
 	//

diff --git a/pkg/testutils/serverutils/conditional_wrap.go b/pkg/testutils/serverutils/conditional_wrap.go
@@ -21,21 +21,12 @@ import (
 )
 
 const tipText = `consider replacing the test server initialization from:
-
-    ts, db, kvDB := serverutils.StartServer(t, ...)
-    // or:
-    tc := serverutils.StartCluster(t, ...)
-    ts := tc.Server(0)
-
-To:
-
-    srv, db, kvDB := serverutils.StartServer(t, ...)
-    defer srv.Stop(...)
+    ts, ... := serverutils.StartServer(t, ...)
+    defer ts.Stopper().Stop(...)
+to:
+    srv, ... := serverutils.StartServer(t, ...)
+    defer srv.Stopper().Stop(...)
     ts := srv.ApplicationLayer()
-    // or:
-    tc := serverutils.StartCluster(t, ...)
-    ts := tc.Server(0).ApplicationLayer()
-
 `
 
 // When this env var is set, all the suspicious API calls are reported in test logs.
@@ -224,7 +215,9 @@ func makeSeriousNotifyFn(
 	}
 	return func(methodName string) {
 		reportFn(func() {
-			(*logFn)("\n%s\n\tWARNING: risky use of implicit %s via .%s()\nHINT: clarify intent using .%s().%s() or .%s().%s() instead.\n",
+			(*logFn)("\n%s\n\tWARNING: risky use of implicit %s via .%s()\n"+
+				"See: https://go.crdb.dev/p/testserver-api-problem\n"+
+				"HINT: clarify intent using .%s().%s() or .%s().%s() instead.\n",
 				GetExternalCaller(),
 				ifname, methodName, accessor1, methodName, accessor2, methodName)
 			(*logFn)("TIP: %s", tipText)

diff --git a/pkg/testutils/serverutils/conditional_wrap_internal_test.go b/pkg/testutils/serverutils/conditional_wrap_internal_test.go
@@ -38,7 +38,8 @@ func TestBenignNotifyFn(t *testing.T) {
 			fn("foo")
 			fn("foo")
 			result := buf.String()
-			require.Contains(t, result, "NOTICE: .foo() called via implicit interface IN;\nHINT: consider using .ACC().foo() instead")
+			require.Contains(t, result, "NOTICE: .foo() called via implicit interface IN")
+			require.Contains(t, result, "HINT: consider using .ACC().foo() instead")
 			if showTip {
 				require.Contains(t, result, "TIP:")
 			} else {
@@ -71,7 +72,8 @@ func TestSeriousNotifyFn(t *testing.T) {
 		fn("foo")
 		fn("foo")
 		result := buf.String()
-		require.Contains(t, result, "WARNING: risky use of implicit IN via .foo()\nHINT: clarify intent using .ACC1().foo() or .ACC2().foo() instead")
+		require.Contains(t, result, "WARNING: risky use of implicit IN via .foo()")
+		require.Contains(t, result, "HINT: clarify intent using .ACC1().foo() or .ACC2().foo() instead")
 		require.Contains(t, result, "TIP:")
 
 		if reportAllCalls {