bazel: bazci appropriately captures sharded test/log output

Some tests, like `//pkg/server:server_test`, are sharded, meaning that
their `test.log`/`test.xml` files are split into many different folders
in the `bazel-testlogs` directory, like:

    bazel-testlogs/pkg/server/server_test/shard_1_of_16/test.log

Before this patch, `bazci` would only look in the `server_test`
directory for the `test.{log,xml}`, and would therefore miss these. Now,
`bazci` globs and mirrors the directory structure of the source test
directory.

Also add unit tests to cover this case.

Resolves #63960.

Release note: None

pkg/cmd/bazci/testdata/bazel-testlogs/pkg/server/server_test/shard_1_of_16/test.xml

@@ -0,0 +1,15 @@
+<testsuites>
+ <testsuite errors="0" failures="0" skipped="0" tests="11" time="16.885" name="github.com/cockroachdb/cockroach/pkg/server">
+ <testcase classname="server" name="TestAdminAPITableStats" time="2.690"></testcase>
+ <testcase classname="server" name="TestAdminAPIZoneDetails" time="0.690"></testcase>
+ <testcase classname="server" name="TestAllVersionsAgree" time="2.090"></testcase>
+ <testcase classname="server" name="TestEndpointTelemetryBasic" time="0.310"></testcase>
+ <testcase classname="server" name="TestGraphite" time="0.670"></testcase>
+ <testcase classname="server" name="TestInitializeFromConfig" time="5.940"></testcase>
+ <testcase classname="server" name="TestMetricsRecording" time="0.630"></testcase>
+ <testcase classname="server" name="TestNodeJoin" time="1.110"></testcase>
+ <testcase classname="server" name="TestPlainHTTPServer" time="0.570"></testcase>
+ <testcase classname="server" name="TestRemoteDebugModeSetting" time="0.870"></testcase>
+ <testcase classname="server" name="TestSettingsRefresh" time="0.770"></testcase>
+ </testsuite>
+</testsuites>

pkg/cmd/bazci/testdata/bazel-testlogs/pkg/server/server_test/shard_2_of_16/test.xml

@@ -0,0 +1,15 @@
+<testsuites>
+ <testsuite errors="0" failures="0" skipped="0" tests="11" time="26.365" name="github.com/cockroachdb/cockroach/pkg/server">
+ <testcase classname="server" name="TestAdminAPIUsers" time="0.410"></testcase>
+ <testcase classname="server" name="TestClusterVersionMixedVersionTooOld" time="0.740"></testcase>
+ <testcase classname="server" name="TestCorruptedClusterID" time="0.130"></testcase>
+ <testcase classname="server" name="TestDecommissionSelf" time="2.300"></testcase>
+ <testcase classname="server" name="TestDummyInitializeNodeFromBundle" time="7.330"></testcase>
+ <testcase classname="server" name="TestLivenessAPI" time="2.750"></testcase>
+ <testcase classname="server" name="TestMakeIdleMonitor" time="2.270"></testcase>
+ <testcase classname="server" name="TestMetricsEndpoint" time="0.610"></testcase>
+ <testcase classname="server" name="TestSecureHTTPRedirect" time="0.430"></testcase>
+ <testcase classname="server" name="TestSettingsSetAndShow" time="0.360"></testcase>
+ <testcase classname="server" name="TestStatusAPITransactions" time="8.880"></testcase>
+ </testsuite>
+</testsuites>

pkg/cmd/bazci/testdata/expected/server_1_test.xml

@@ -0,0 +1,13 @@
+<testsuite errors="0" failures="0" skipped="0" tests="11" time="16.885" name="github.com/cockroachdb/cockroach/pkg/server">
+ <testcase name="TestAdminAPITableStats" time="2.690"></testcase>
+ <testcase name="TestAdminAPIZoneDetails" time="0.690"></testcase>
+ <testcase name="TestAllVersionsAgree" time="2.090"></testcase>
+ <testcase name="TestEndpointTelemetryBasic" time="0.310"></testcase>
+ <testcase name="TestGraphite" time="0.670"></testcase>
+ <testcase name="TestInitializeFromConfig" time="5.940"></testcase>
+ <testcase name="TestMetricsRecording" time="0.630"></testcase>
+ <testcase name="TestNodeJoin" time="1.110"></testcase>
+ <testcase name="TestPlainHTTPServer" time="0.570"></testcase>
+ <testcase name="TestRemoteDebugModeSetting" time="0.870"></testcase>
+ <testcase name="TestSettingsRefresh" time="0.770"></testcase>
+</testsuite>

pkg/cmd/bazci/testdata/expected/server_2_test.xml

@@ -0,0 +1,13 @@
+<testsuite errors="0" failures="0" skipped="0" tests="11" time="26.365" name="github.com/cockroachdb/cockroach/pkg/server">
+ <testcase name="TestAdminAPIUsers" time="0.410"></testcase>
+ <testcase name="TestClusterVersionMixedVersionTooOld" time="0.740"></testcase>
+ <testcase name="TestCorruptedClusterID" time="0.130"></testcase>
+ <testcase name="TestDecommissionSelf" time="2.300"></testcase>
+ <testcase name="TestDummyInitializeNodeFromBundle" time="7.330"></testcase>
+ <testcase name="TestLivenessAPI" time="2.750"></testcase>
+ <testcase name="TestMakeIdleMonitor" time="2.270"></testcase>
+ <testcase name="TestMetricsEndpoint" time="0.610"></testcase>
+ <testcase name="TestSecureHTTPRedirect" time="0.430"></testcase>
+ <testcase name="TestSettingsSetAndShow" time="0.360"></testcase>
+ <testcase name="TestStatusAPITransactions" time="8.880"></testcase>
+</testsuite>

pkg/cmd/bazci/watch.go

@@ -17,6 +17,7 @@ import (
  "log"
  "os"
  "path"
+ "path/filepath"
  "strings"
  "time"
 )
@@ -132,43 +133,20 @@ func (w watcher) stageTestArtifacts(phase Phase) error {
  // relDir is the directory under bazel-testlogs where the test
  // output files can be found.
  relDir := strings.ReplaceAll(strings.TrimPrefix(test, "//"), ":", "/")
- // First, copy the log file verbatim.
- relLogFilePath := path.Join(relDir, "test.log")
- err := w.maybeStageArtifact(testlogsSourceDir, relLogFilePath, 0666, phase,
- copyContentTo)
- if err != nil {
- return err
- }
- // Munge and copy the xml file.
- relXMLFilePath := path.Join(relDir, "test.xml")
- err = w.maybeStageArtifact(testlogsSourceDir, relXMLFilePath, 0666, phase,
- func(srcContent []byte, outFile io.Writer) error {
- // Parse the XML into a testSuites struct.
- suites := testSuites{}
- err := xml.Unmarshal(srcContent, &suites)
- // Note that we return an error if parsing fails. This isn't
- // unexpected -- if we read the XML file before it's been
- // completely written to disk, that will happen. Returning the
- // error will cancel the write to disk, which is exactly what we
- // want.
- if err != nil {
- return err
- }
- // We only want the first test suite in the list of suites.
- munged, err := xml.MarshalIndent(&suites.Suites[0], "", "\t")
- if err != nil {
- return err
- }
- _, err = outFile.Write(munged)
- if err != nil {
- return err
- }
- // Insert a newline just to make our lives a little easier.
- _, err = outFile.Write([]byte("\n"))
+ for _, tup := range []struct {
+ relPath string
+ stagefn func(srcContent []byte, outFile io.Writer) error
+ }{
+ {path.Join(relDir, "test.log"), copyContentTo},
+ {path.Join(relDir, "*", "test.log"), copyContentTo},
+ {path.Join(relDir, "test.xml"), mungeTestXML},
+ {path.Join(relDir, "*", "test.xml"), mungeTestXML},
+ } {
+ err := w.maybeStageArtifact(testlogsSourceDir, tup.relPath, 0666, phase,
+ tup.stagefn)
+ if err != nil {
  return err
- })
- if err != nil {
- return err
+ }
  }
  }
  return nil
@@ -235,6 +213,35 @@ func copyContentTo(srcContent []byte, outFile io.Writer) error {
  return err
 }
 
+// mungeTestXML parses and slightly munges the XML in the source file and writes
+// it to the output file. Helper function meant to be used with
+// maybeStageArtifact.
+func mungeTestXML(srcContent []byte, outFile io.Writer) error {
+ // Parse the XML into a testSuites struct.
+ suites := testSuites{}
+ err := xml.Unmarshal(srcContent, &suites)
+ // Note that we return an error if parsing fails. This isn't
+ // unexpected -- if we read the XML file before it's been
+ // completely written to disk, that will happen. Returning the
+ // error will cancel the write to disk, which is exactly what we
+ // want.
+ if err != nil {
+ return err
+ }
+ // We only want the first test suite in the list of suites.
+ munged, err := xml.MarshalIndent(&suites.Suites[0], "", "\t")
+ if err != nil {
+ return err
+ }
+ _, err = outFile.Write(munged)
+ if err != nil {
+ return err
+ }
+ // Insert a newline just to make our lives a little easier.
+ _, err = outFile.Write([]byte("\n"))
+ return err
+}
+
 // cancelableWriter is an io.WriteCloser with the following properties:
 // 1. The entire contents of the file is accumulated in memory in the `buf`.
 // 2. The file is written to disk when Close() is called...
@@ -277,39 +284,41 @@ func (w *cancelableWriter) Close() error {
  return nil
 }
 
-// maybeStageArtifact stages a build or test artifact in the artifactsDir unless
-// the cache indicates the file is already up-to-date. maybeStageArtifact stages
-// the file from the given `root` (e.g. either `bazel-testlogs` or `bazel-bin`)
-// and at the given relative path. If maybeStageArtifact determines that a file
-// needs to be staged, it is created in the artifactsDir at the same `root` and
-// `relPath` with the given `perm`, and the `stagefn` is called to populate the
-// contents of the newly staged file. If the source artifact does not exist,
-// or has not been updated since the last time it was staged, maybeStageArtifact
-// does nothing. If the `stagefn` returns a non-nil error, then the artifact is
-// not staged. (Errors will not be propagated back up to the caller of
-// maybeStageArtifact unless we're in the "finalize" phase -- errors can happen
-// sporadically if we're not finalizing, especially if we read an artifact while
-// it's in the process of being written.)
+// maybeStageArtifact stages one or more build or test artifacts in the
+// artifactsDir unless the cache indicates the file to be staged is already
+// up-to-date. maybeStageArtifact stages all files in the given `root` (e.g.
+// either `bazel-testlogs` or `bazel-bin`) matching the given `pattern`.
+// `filepath.Glob()` is used to determine which files match the given pattern.
+// If maybeStageArtifact determines that a file needs to be staged, it is
+// created in the artifactsDir at the same `root` and relative path with the
+// given `perm`, and the `stagefn` is called to populate the contents of the
+// newly staged file. If the source artifact does not exist, or has not been
+// updated since the last time it was staged, maybeStageArtifact does nothing.
+// If the `stagefn` returns a non-nil error, then the artifact is not staged.
+// (Errors will not be propagated back up to the caller of maybeStageArtifact
+// unless we're in the "finalize" phase -- errors can happen sporadically if
+// we're not finalizing, especially if we read an artifact while it's in the
+// process of being written.)
 //
 // In the intialCachingPhase, NO artifacts will be staged, but
 // maybeStageArtifact will still stat the source file and cache its metadata.
 // This is important because Bazel aggressively caches build and test artifacts,
 // so just because a file exists, doesn't necessarily mean that it should be
-// staged. For example -- if we're running //pkg/server:server_test, and
-// bazel-testlogs/pkg/server/server_test/test.xml exists, it may just be because
-// that file was created by a PREVIOUS run of :server_test. Staging it right
-// away makes no sense in this case -- the XML will contain old data. Instead,
-// we note the file metadata so that we know to re-stage it when the file is
-// updated. Meanwhile, we force-stage everything that hasn't yet been staged
-// once in the finalizePhase, to cover cases where Bazel reused the old
+// staged. For example -- if we're running //pkg/settings:settings_test, and
+// bazel-testlogs/pkg/settings/settings_test/test.xml exists, it may just be
+// because that file was created by a PREVIOUS run of :settings_test. Staging
+// it right away makes no sense in this case -- the XML will contain old data.
+// Instead, we note the file metadata so that we know to re-stage it when the
+// file is updated. Meanwhile, we force-stage everything that hasn't yet been
+// staged once in the finalizePhase, to cover cases where Bazel reused the old
 // cached artifact.
 //
-// For example, one might stage a log file with a call like:
-// w.maybeStageArtifact(testlogsSourceDir, "pkg/server/server_test/test.log",
+// For example, one might stage a set of log files with a call like:
+// w.maybeStageArtifact(testlogsSourceDir, "pkg/server/server_test/*/test.log",
 // 0666, incrementalUpdatePhase, copycontentTo)
 func (w watcher) maybeStageArtifact(
  root SourceDir,
- relPath string,
+ pattern string,
  perm os.FileMode,
  phase Phase,
  stagefn func(srcContent []byte, outFile io.Writer) error,
@@ -324,11 +333,7 @@ func (w watcher) maybeStageArtifact(
  rootPath = w.info.binDir
  artifactsSubdir = "bazel-bin"
  }
- // Fully qualified source and destination paths.
- srcPath := path.Join(rootPath, relPath)
- destPath := path.Join(artifactsDir, artifactsSubdir, relPath)
-
- stage := func() error {
+ stage := func(srcPath, destPath string) error {
  contents, err := ioutil.ReadFile(srcPath)
  if err != nil {
  return err
@@ -356,8 +361,24 @@ func (w watcher) maybeStageArtifact(
  return nil
  }
 
- stat, err := os.Stat(srcPath)
- if err == nil {
+ matches, err := filepath.Glob(path.Join(rootPath, pattern))
+ if err != nil {
+ return err
+ }
+ for _, srcPath := range matches {
+ relPath, err := filepath.Rel(rootPath, srcPath)
+ if err != nil {
+ return err
+ }
+ destPath := path.Join(artifactsDir, artifactsSubdir, relPath)
+
+ stat, err := os.Stat(srcPath)
+ // stat errors can simply be ignored -- if the file doesn't
+ // exist, we skip it. (Note that this is unlikely, but due to a
+ // race could occur.)
+ if err != nil {
+ continue
+ }
  meta := fileMetadata{size: stat.Size(), modTime: stat.ModTime()}
  oldMeta, oldMetaOk := w.fileToMeta[srcPath]
  // If we don't have metadata for this file yet, or if the
@@ -369,14 +390,14 @@ func (w watcher) maybeStageArtifact(
  // metadata.
  case incrementalUpdatePhase, finalizePhase:
  _, staged := w.fileToStaged[srcPath]
- // This is not a great situation: we've been
- // asked to stage a file that was already
- // staged. Do it again, but print a warning.
- if staged {
+ // This is not a great situation: we've been asked to stage a file
+ // that was already staged. Do it again, but print a warning. Log
+ // files are OK, since TC doesn't parse them.
+ if staged && !strings.HasSuffix(destPath, ".log") {
  log.Printf("WARNING: re-staging already-staged file at %s",
  destPath)
  }
- err := stage()
+ err := stage(srcPath, destPath)
  if err != nil {
  return err
  }
@@ -387,13 +408,11 @@ func (w watcher) maybeStageArtifact(
  // During the finalize phase, stage EVERYTHING that hasn't been
  // staged yet. This is our last chance!
  if !staged && phase == finalizePhase {
- err := stage()
+ err := stage(srcPath, destPath)
  if err != nil {
  return err
  }
  }
  }
- // stat errors can simply be ignored -- if the file doesn't exist, we
- // skip it.
  return nil
 }

pkg/cmd/bazci/watch_test.go

@@ -27,6 +27,14 @@ func assertFileCopiedVerbatim(t *testing.T, relPath string) {
  assert.Equal(t, actual, expected)
 }
 
+func assertFilesIdentical(t *testing.T, actualPath, expectedPath string) {
+ actual, err := ioutil.ReadFile(actualPath)
+ assert.Nil(t, err)
+ expected, err := ioutil.ReadFile(expectedPath)
+ assert.Nil(t, err)
+ assert.Equal(t, actual, expected)
+}
+
 func TestWatch(t *testing.T) {
  dir, cleanup := testutils.TempDir(t)
  defer cleanup()
@@ -36,7 +44,7 @@ func TestWatch(t *testing.T) {
  binDir: path.Join(testdata, "bazel-bin"),
  testlogsDir: path.Join(testdata, "bazel-testlogs"),
  goBinaries: []string{"//pkg/cmd/fake_bin:fake_bin"},
- tests: []string{"//pkg/rpc:rpc_test"},
+ tests: []string{"//pkg/rpc:rpc_test", "//pkg/server:server_test"},
  }
  completion := make(chan error, 1)
  completion <- nil
@@ -45,12 +53,14 @@ func TestWatch(t *testing.T) {
 
  assert.Nil(t, err)
  assertFileCopiedVerbatim(t, "bazel-testlogs/pkg/rpc/rpc_test/test.log")
+ assertFileCopiedVerbatim(t, "bazel-testlogs/pkg/server/server_test/shard_1_of_16/test.log")
+ assertFileCopiedVerbatim(t, "bazel-testlogs/pkg/server/server_test/shard_2_of_16/test.log")
  assertFileCopiedVerbatim(t, "bazel-bin/pkg/cmd/fake_bin/fake_bin_/fake_bin")
  // check the xml file was munged correctly.
- actual, err := ioutil.ReadFile(
- path.Join(artifactsDir, "bazel-testlogs/pkg/rpc/rpc_test/test.xml"))
- assert.Nil(t, err)
- expected, err := ioutil.ReadFile(path.Join(testdata, "expected/test.xml"))
- assert.Nil(t, err)
- assert.Equal(t, actual, expected)
+ assertFilesIdentical(t, path.Join(artifactsDir, "bazel-testlogs/pkg/rpc/rpc_test/test.xml"),
+ path.Join(testdata, "expected/rpc_test.xml"))
+ assertFilesIdentical(t, path.Join(artifactsDir, "bazel-testlogs/pkg/server/server_test/shard_1_of_16/test.xml"),
+  path.Join(testdata, "expected/server_1_test.xml"))
+ assertFilesIdentical(t, path.Join(artifactsDir, "bazel-testlogs/pkg/server/server_test/shard_2_of_16/test.xml"),
+  path.Join(testdata, "expected/server_2_test.xml"))
 }