Merge cockroachdb#123344

123344: sql: improve ExplainRedact tests r=yuzefovich a=yuzefovich

This commit improves the EXPLAIN (REDACT) tests in the following manner:
- only log successful statements with semicolon in the end
- ensure that all generated statements are sound (i.e. don't error on `EXPLAIN stmt`)
- use simple names (we can't inject the PII into the names anyway)
- include the stmt for non-internal errors.

Informs: cockroachdb#122766.
Epic: None

Release note: None

Co-authored-by: Yahor Yuzefovich <[email protected]>

pkg/ccl/testccl/sqlccl/explain_test.go

@@ -44,7 +44,6 @@ func TestExplainRedactDDL(t *testing.T) {
 
 	s, sqlDB, _ := serverutils.StartServer(t, base.TestServerArgs{})
 	defer s.Stopper().Stop(ctx)
-	defer sqlDB.Close()
 
 	query := func(sql string) (*gosql.Rows, error) {
 		return sqlDB.QueryContext(ctx, sql)
@@ -69,12 +68,12 @@ func TestExplainRedactDDL(t *testing.T) {
 	setup = append(setup, "SET CLUSTER SETTING sql.stats.automatic_collection.enabled = off;")
 	setup = append(setup, "SET statement_timeout = '5s';")
 	for _, stmt := range setup {
-		t.Log(stmt)
 		if _, err := sqlDB.ExecContext(ctx, stmt); err != nil {
 			// Ignore errors.
-			t.Log("-- ignoring error:", err)
 			continue
 		}
+		// Only log successful statements.
+		t.Log(stmt + ";")
 	}
 
 	// Check EXPLAIN (OPT, CATALOG, REDACT) for each table.
@@ -92,7 +91,6 @@ func TestExplainRedactDDL(t *testing.T) {
 	}
 	for _, table := range tables {
 		explain := "EXPLAIN (OPT, CATALOG, REDACT) SELECT * FROM " + lexbase.EscapeSQLIdent(table)
-		t.Log(explain)
 		rows, err = query(explain)
 		if err != nil {
 			// This explain should always succeed.
@@ -119,6 +117,7 @@ func TestExplainRedactDDL(t *testing.T) {
 		sqlsmith.PrefixStringConsts(pii),
 		sqlsmith.OnlySingleDMLs(),
 		sqlsmith.EnableAlters(),
+		sqlsmith.SimpleNames(),
 	)
 	if err != nil {
 		t.Fatal(err)

pkg/sql/explain_test.go

@@ -551,6 +551,7 @@ func TestExplainRedact(t *testing.T) {
 	smith, err := sqlsmith.NewSmither(sqlDB, rng,
 		sqlsmith.PrefixStringConsts(pii),
 		sqlsmith.DisableDDLs(),
+		sqlsmith.SimpleNames(),
 	)
 	if err != nil {
 		t.Fatal(err)

pkg/sql/tests/explain_test_util.go

@@ -32,11 +32,21 @@ func GenerateAndCheckRedactedExplainsForPII(
 	containsPII func(explain, output string) error,
 ) {
 	// Generate a few random statements.
-	statements := make([]string, num)
+	statements := make([]string, 0, num)
 	t.Log("generated statements:")
-	for i := range statements {
-		statements[i] = smith.Generate()
-		t.Log(statements[i])
+	for len(statements) < num {
+		stmt := smith.Generate()
+		// Try vanilla EXPLAIN on this stmt to ensure that it is sound.
+		_, err := query("EXPLAIN " + stmt)
+		if err != nil {
+			// We shouldn't see any internal errors - ignore all others.
+			if strings.Contains(err.Error(), "internal error") {
+				t.Error(err)
+			}
+		} else {
+			statements = append(statements, stmt)
+			t.Log(stmt + ";")
+		}
 	}
 
 	// Gather EXPLAIN variants to test.
@@ -98,7 +108,7 @@ func GenerateAndCheckRedactedExplainsForPII(
 									} else if !strings.Contains(msg, "syntax error") {
 										// Skip logging syntax errors, since they're expected to be
 										// common and uninteresting.
-										t.Logf("encountered non-internal error: %s\n", err)
+										t.Logf("encountered non-internal error: %s\n%s\n\n", err, explain)
 									}
 									continue
 								}