Merge cockroachdb#122087

122087: testutils/serverutils: reduce test noise about mt interfaces r=dt a=dt Release note: none. Epic: none. Co-authored-by: David Taylor <[email protected]>
rimadeodhar · Apr 17, 2024 · 0ac28a3 · 0ac28a3
2 parents e14b480 + d8c8a4d
commit 0ac28a3
Show file tree

Hide file tree

Showing 4 changed files with 22 additions and 147 deletions.
diff --git a/pkg/testutils/serverutils/BUILD.bazel b/pkg/testutils/serverutils/BUILD.bazel
@@ -143,13 +143,11 @@ go_test(
     ],
     embed = [":serverutils"],
     deps = [
-        "//pkg/base",
         "//pkg/security/securityassets",
         "//pkg/security/securitytest",
         "//pkg/server",
         "//pkg/testutils",
         "//pkg/util/leaktest",
-        "//pkg/util/log",
         "@com_github_stretchr_testify//require",
     ],
 )
diff --git a/pkg/testutils/serverutils/conditional_wrap.go b/pkg/testutils/serverutils/conditional_wrap.go
@@ -14,7 +14,6 @@ import (
 	"fmt"
 	"runtime"
 	"strings"
-	"sync"
 
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/util/envutil"
@@ -31,8 +30,8 @@ to:
 See also: https://go.crdb.dev/p/testserver-and-cluster-virtualization
 `
 
-// When this env var is set, all the suspicious API calls are reported in test logs.
-var reportAllCalls = envutil.EnvOrDefaultBool("COCKROACH_TEST_SERVER_SHIM_VERBOSE", false)
+// When this env var is set, suspicious API calls are reported in test logs.
+var reportCalls = envutil.EnvOrDefaultBool("COCKROACH_TEST_SERVER_SHIM_VERBOSE", false)
 
 func wrapTestServer(
 	raw TestServerInterfaceRaw, opts base.DefaultTestTenantOptions,
@@ -180,53 +179,19 @@ func TestingSetWrapperLogger(w TestServerInterface, logFn func(string, ...interf
 func makeBenignNotifyFn(
 	logFn *func(string, ...interface{}), ifname, accessor string, showTip bool,
 ) func(methodName string) {
-	reportFn := func(fn func()) { fn() }
-	if !reportAllCalls {
-		var once sync.Once
-		reportFn = once.Do
+	if !reportCalls {
+		return func(_ string) {}
 	}
 	return func(methodName string) {
-		reportFn(func() {
-			(*logFn)("\n%s\n\tNOTICE: .%s() called via implicit interface %s;\nHINT: consider using .%s().%s() instead.\n",
-				GetExternalCaller(),
-				methodName, ifname, accessor, methodName)
-			if showTip {
-				(*logFn)("TIP: %s", tipText)
-			}
-		})
-	}
-}
-
-// makeSeriousNotifyFn constructs a function that strongly recommends
-// the user use an explicit interface accessor, the first time a
-// method is called on the given interface.
-//
-// The logging function is passed via a pointer because we want to
-// keep the ability to override it after the forwarder has been
-// instantiated (for example StartServerOnlyE injects the t.Log
-// function as a logger.
-func makeSeriousNotifyFn(
-	logFn *func(string, ...interface{}), ifname, accessor1, accessor2 string,
-) func(methodName string) {
-	reportFn := func(fn func()) { fn() }
-	if !reportAllCalls {
-		var once sync.Once
-		reportFn = once.Do
-	}
-	return func(methodName string) {
-		reportFn(func() {
-			(*logFn)("\n%s\n\tWARNING: risky use of implicit %s via .%s()\n"+
-				"See: https://go.crdb.dev/p/testserver-and-cluster-virtualization\n"+
-				"HINT: clarify intent using .%s().%s() or .%s().%s() instead.\n",
-				GetExternalCaller(),
-				ifname, methodName, accessor1, methodName, accessor2, methodName)
+		(*logFn)("\n%s\n\tNOTICE: .%s() called via implicit interface %s;\nHINT: consider using .%s().%s() instead.\n",
+			GetExternalCaller(),
+			methodName, ifname, accessor, methodName)
+		if showTip {
 			(*logFn)("TIP: %s", tipText)
-		})
+		}
 	}
 }
 
-var _ = makeSeriousNotifyFn // silence unused linter
-
 // GetExternalCaller returns the file:line of the first function in
 // the call stack outside of this package. It is used as prefix for
 // informational messages about potential interface misuse.

diff --git a/pkg/testutils/serverutils/conditional_wrap_internal_test.go b/pkg/testutils/serverutils/conditional_wrap_internal_test.go
@@ -27,59 +27,32 @@ func TestBenignNotifyFn(t *testing.T) {
 	var buf strings.Builder
 	logFn = func(format string, args ...interface{}) { fmt.Fprintf(&buf, format, args...) }
 
-	defer func(old bool) { reportAllCalls = old }(reportAllCalls)
+	defer func(old bool) { reportCalls = old }(reportCalls)
 
-	testutils.RunTrueAndFalse(t, "reportAllCalls", func(t *testing.T, tb bool) {
-		reportAllCalls = tb
+	testutils.RunTrueAndFalse(t, "reportCalls", func(t *testing.T, tb bool) {
+		reportCalls = tb
 
 		testutils.RunTrueAndFalse(t, "showTip", func(t *testing.T, showTip bool) {
 			buf.Reset()
 			fn := makeBenignNotifyFn(&logFn, "IN", "ACC", showTip)
 			fn("foo")
 			fn("foo")
 			result := buf.String()
-			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 {
-				require.NotContains(t, result, "TIP:")
+			if reportCalls {
+				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 {
+					require.NotContains(t, result, "TIP:")
+				}
 			}
 
-			if reportAllCalls {
+			if reportCalls {
 				require.Equal(t, 2, strings.Count(result, "NOTICE:"))
 			} else {
-				require.Equal(t, 1, strings.Count(result, "NOTICE:"))
+				require.Equal(t, 0, strings.Count(result, "NOTICE:"))
 			}
 		})
 	})
 }
-
-func TestSeriousNotifyFn(t *testing.T) {
-	defer leaktest.AfterTest(t)()
-
-	var logFn func(string, ...interface{})
-	var buf strings.Builder
-	logFn = func(format string, args ...interface{}) { fmt.Fprintf(&buf, format, args...) }
-
-	defer func(old bool) { reportAllCalls = old }(reportAllCalls)
-
-	testutils.RunTrueAndFalse(t, "reportAllCalls", func(t *testing.T, tb bool) {
-		reportAllCalls = tb
-
-		buf.Reset()
-		fn := makeSeriousNotifyFn(&logFn, "IN", "ACC1", "ACC2")
-		fn("foo")
-		fn("foo")
-		result := buf.String()
-		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 {
-			require.Equal(t, 2, strings.Count(result, "WARNING:"))
-		} else {
-			require.Equal(t, 1, strings.Count(result, "WARNING:"))
-		}
-	})
-}
diff --git a/pkg/testutils/serverutils/conditional_wrap_test.go b/pkg/testutils/serverutils/conditional_wrap_test.go
@@ -11,20 +11,15 @@
 package serverutils_test
 
 import (
-	"bytes"
-	"context"
-	"fmt"
 	"os"
 	"strings"
 	"testing"
 
-	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/security/securityassets"
 	"github.com/cockroachdb/cockroach/pkg/security/securitytest"
 	"github.com/cockroachdb/cockroach/pkg/server"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
-	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/stretchr/testify/require"
 )
 
@@ -64,59 +59,3 @@ func otherFn() string {
 }
 
 var externalC = func() string { return serverutils.GetExternalCaller() }()
-
-func TestInterfaceNotices(t *testing.T) {
-	defer leaktest.AfterTest(t)()
-
-	runCalls := func(s serverutils.TestServerInterface) {
-		_ = s.AppStopper()
-		_ = s.NodeID()
-		_ = s.StartedDefaultTestTenant()
-	}
-
-	testCases := []struct {
-		name     string
-		opts     base.DefaultTestTenantOptions
-		scenario func(t *testing.T, exec func(func(serverutils.TestServerInterface)) string)
-	}{
-		{
-			name: "default",
-			opts: (base.DefaultTestTenantOptions{}),
-			scenario: func(t *testing.T, exec func(func(serverutils.TestServerInterface)) string) {
-				result := exec(runCalls)
-				require.Contains(t, result, "NOTICE: .AppStopper() called via implicit interface ApplicationLayerInterface")
-				require.Contains(t, result, "NOTICE: .NodeID() called via implicit interface StorageLayerInterface")
-				require.Contains(t, result, "NOTICE: .StartedDefaultTestTenant() called via implicit interface TenantControlInterface")
-			},
-		},
-		{
-			name: "systemonly",
-			opts: base.TestIsSpecificToStorageLayerAndNeedsASystemTenant,
-			scenario: func(t *testing.T, exec func(func(serverutils.TestServerInterface)) string) {
-				result := exec(runCalls)
-				require.NotContains(t, result, ".AppStopper()")
-				require.NotContains(t, result, ".NodeID()")
-				require.Contains(t, result, "NOTICE: .StartedDefaultTestTenant() called via implicit interface TenantControlInterface")
-			},
-		},
-	}
-
-	for _, tc := range testCases {
-		t.Run(tc.name, func(t *testing.T) {
-			defer log.Scope(t).Close(t)
-
-			s := serverutils.StartServerOnly(t, base.TestServerArgs{DefaultTestTenant: tc.opts})
-			defer s.Stopper().Stop(context.Background())
-
-			var buf bytes.Buffer
-			serverutils.TestingSetWrapperLogger(s, func(format string, args ...interface{}) { fmt.Fprintf(&buf, format, args...) })
-
-			exec := func(fn func(serverutils.TestServerInterface)) string {
-				buf.Reset()
-				fn(s)
-				return buf.String()
-			}
-			tc.scenario(t, exec)
-		})
-	}
-}