Merge #104618

104618: test: add support for `TEST_UNDECLARED_OUTPUTS_DIR` r=rickystewart a=rickystewart

To date, we have put temporary files from tests in $TMPDIR. We have a patch to `rules_go` that copies the value of the $TEST_TMPDIR (the variable that Bazel provides) over to $TMPDIR for use in CI. Some tests (especially those using TestLogScope) have behavior where they leave files behind after the test completes *if the test fails*, thereby allowing people to look at the left-over files for debugging.

As we transition to remote execution, this will no longer work, since the $TMPDIR is on some remote machine somewhere, and Bazel will just clean the $TMPDIR up after the test completes regardless of its exit status.

Bazel provides the variable `TEST_UNDECLARED_OUTPUTS_DIR` for the same purpose: it gives us a place to put unstructured output from tests. To prepare for remote execution, we make the following changes:

1. Update `TestLogScope` to use `TEST_UNDECLARED_OUTPUTS_DIR` where appropriate.
2. Add a new function `datapathutils.DebuggableTempDir()` which returns either `TEST_UNDECLARED_OUTPUTS_DIR` or os.TempDir() as appropriate.

Since the outputs.zip behavior is kind of awkward, we guard this behind the environment variable `REMOTE_EXEC`. We must be sure to set this variable whenever we run tests remotely.

Epic: CRDB-17165
Release note: None

Co-authored-by: Ricky Stewart <ricky@cockroachlabs.com>

pkg/testutils/datapathutils/data_path.go

@@ -12,6 +12,7 @@ package datapathutils
 
 import (
 	"fmt"
+	"os"
 	"path"
 	"path/filepath"
 	"runtime"
@@ -22,6 +23,30 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+// DebuggableTempDir returns a temporary directory that is appropriate for
+// retaining leftover artifacts in case the temp fails for debugging.
+// If the test is not executing remotely, or the test is not built with Bazel,
+// then os.TempDir() is returned (the person executing the test should set up
+// their $TMPDIR appropriately so they know where to find these leftover
+// artifacts). If the test is executing remotely, then we instead use
+// TEST_UNDECLARED_OUTPUTS_DIR, and if any files are left-over in this directory
+// after the test exits, then Bazel will package them up into outputs.zip.
+// This function is specifically for when we want to capture left-over artifacts
+// after the test completes; if you are always going to clean up the files you
+// create either way, testutils.TempDir() may be more convenient.
+func DebuggableTempDir() string {
+	if bazel.BuiltWithBazel() {
+		isRemote := os.Getenv("REMOTE_EXEC")
+		if len(isRemote) > 0 {
+			outputs := os.Getenv("TEST_UNDECLARED_OUTPUTS_DIR")
+			if len(outputs) > 0 {
+				return outputs
+			}
+		}
+	}
+	return os.TempDir()
+}
+
 // TestDataPath returns a path to an asset in the testdata directory. It knows
 // to access the right path when executing under bazel.
 //

pkg/testutils/lint/lint_test.go

@@ -479,12 +479,14 @@ func TestLint(t *testing.T) {
 					":!testutils/data_path.go",
 					":!util/log/tracebacks.go",
 					":!util/sdnotify/sdnotify_unix.go",
-					":!util/grpcutil",                     // GRPC_GO_* variables
-					":!roachprod",                         // roachprod requires AWS environment variables
-					":!cli/env.go",                        // The CLI needs the PGHOST variable.
-					":!cli/start.go",                      // The CLI needs the GOMEMLIMIT variable.
-					":!internal/codeowners/codeowners.go", // For BAZEL_TEST.
-					":!internal/team/team.go",             // For BAZEL_TEST.
+					":!util/grpcutil",                        // GRPC_GO_* variables
+					":!roachprod",                            // roachprod requires AWS environment variables
+					":!cli/env.go",                           // The CLI needs the PGHOST variable.
+					":!cli/start.go",                         // The CLI needs the GOMEMLIMIT variable.
+					":!internal/codeowners/codeowners.go",    // For BAZEL_TEST.
+					":!internal/team/team.go",                // For BAZEL_TEST.
+					":!util/log/test_log_scope.go",           // For TEST_UNDECLARED_OUTPUT_DIR, REMOTE_EXEC
+					":!testutils/datapathutils/data_path.go", // For TEST_UNDECLARED_OUTPUT_DIR, REMOTE_EXEC
 				},
 			},
 		} {

pkg/util/log/test_log_scope.go

@@ -15,6 +15,7 @@ import (
 	"io"
 	"os"
 	"runtime"
+	"strings"
 
 	"github.com/cockroachdb/cockroach/pkg/cli/exit"
 	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
@@ -32,9 +33,10 @@ import (
 // test, and enforces that logging output is not written to this
 // directory beyond the lifetime of the scope.
 type TestLogScope struct {
-	logDir    string
-	cleanupFn func()
-	previous  struct {
+	logDir               string
+	cleanupFn            func()
+	undeclaredOutputsDir string
+	previous             struct {
 		appliedConfig           string
 		stderrSinkInfoTemplate  sinkInfo
 		stderrSinkInfo          *sinkInfo
@@ -140,7 +142,11 @@ func newLogScope(t tShim, mostlyInline bool) (sc *TestLogScope) {
 	logging.mu.Unlock()
 
 	err := func() error {
-		tempDir, err := os.MkdirTemp("", "log"+fileutil.EscapeFilename(t.Name()))
+		isRemote := os.Getenv("REMOTE_EXEC")
+		if len(isRemote) > 0 {
+			sc.undeclaredOutputsDir = os.Getenv("TEST_UNDECLARED_OUTPUTS_DIR")
+		}
+		tempDir, err := os.MkdirTemp(sc.undeclaredOutputsDir, "log"+fileutil.EscapeFilename(t.Name()))
 		if err != nil {
 			return err
 		}
@@ -158,7 +164,7 @@ func newLogScope(t tShim, mostlyInline bool) (sc *TestLogScope) {
 			return err
 		}
 
-		t.Logf("test logs captured to: %s", sc.logDir)
+		t.Logf("test logs captured to: %s", sc.printableLogDirectory())
 		return nil
 	}()
 	if err != nil {
@@ -353,6 +359,15 @@ func (l *TestLogScope) SetupSingleFileLogging() (cleanup func()) {
 	return cleanup
 }
 
+// printableLogDirectory returns a human-readable version of the log directory
+// suitable for printing to the console.
+func (l *TestLogScope) printableLogDirectory() string {
+	if len(l.undeclaredOutputsDir) > 0 {
+		return "outputs.zip/" + strings.TrimPrefix(l.logDir, l.undeclaredOutputsDir+"/")
+	}
+	return l.logDir
+}
+
 // GetDirectory retrieves the log directory for this scope.
 func (l *TestLogScope) GetDirectory() string {
 	return l.logDir
@@ -405,7 +420,7 @@ func (l *TestLogScope) Close(t tShim) {
 						"Details cannot be printed yet because we are still unwinding.\n"+
 						"Hopefully the test harness prints the panic below, otherwise check the test logs.\n\n")
 				}
-				fmt.Fprintln(OrigStderr, "test logs left over in:", l.logDir)
+				fmt.Fprintln(OrigStderr, "test logs left over in:", l.printableLogDirectory())
 			} else {
 				// Clean up.
 				if err := os.RemoveAll(l.logDir); err != nil {