BEP: Provide more useful data in TestResult events

For individual test actions, more useful data can be provided
than just the success status. Do so.

Also, to keep each log reported only once, send a new
TestAttempt event for the final test attempt, but don't
have the TestResult (which contains to union of all test
files) an instance of the BuildEvent.

As we now report test results in a separate object, drop
the test data and directly pass in the files created. In
this way, we also get all the moved files reported correctly
in the individual attempts.

--
Change-Id: Ic04b7bad4f92a381bd4d1b4ec91f743b89f81f84
Reviewed-on: https://cr.bazel.build/8694
PiperOrigin-RevId: 147149025
MOS_MIGRATED_REVID=147149025

4 files changed, 72 insertions, 64 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto b/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto
index 6131112e81..60ee9e83a3 100644
--- a/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto
+++ b/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto
@@ -208,6 +208,10 @@ message TargetComplete {
 // Payload on events reporting about individual test action.
 message TestResult {
   bool success = 1;
+
+  // Files (logs, test.xml, undeclared outputs, etc) generated by that test
+  // action.
+  repeated File test_action_output = 2;
 }
 
 // Payload of the event summarizing a test.
diff --git a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
index 5e33a9ba77..c0739034e7 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
@@ -14,6 +14,7 @@
 
 package com.google.devtools.build.lib.exec;
 
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
@@ -38,6 +39,7 @@ import com.google.devtools.build.lib.rules.test.TestAttempt;
 import com.google.devtools.build.lib.rules.test.TestResult;
 import com.google.devtools.build.lib.rules.test.TestRunnerAction;
 import com.google.devtools.build.lib.rules.test.TestRunnerAction.ResolvedPaths;
+import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.util.io.FileOutErr;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
@@ -155,7 +157,19 @@ public class StandaloneTestStrategy extends TestStrategy {
                 workingDirectory);
       }
       processLastTestAttempt(attempt, dataBuilder, data);
-      finalizeTest(attempt, actionExecutionContext, action, dataBuilder.build());
+      ImmutableList.Builder<Pair<String, Path>> testOutputsBuilder = new ImmutableList.Builder<>();
+      if (action.getTestLog().getPath().exists()) {
+        testOutputsBuilder.add(Pair.of("test.log", action.getTestLog().getPath()));
+      }
+      if (resolvedPaths.getXmlOutputPath().exists()) {
+        testOutputsBuilder.add(Pair.of("test.xml", resolvedPaths.getXmlOutputPath()));
+      }
+      executor
+          .getEventBus()
+          .post(
+              new TestAttempt(
+                  action, attempt, data.getTestPassed(), testOutputsBuilder.build(), true));
+      finalizeTest(actionExecutionContext, action, dataBuilder.build());
     } catch (IOException e) {
       executor.getEventHandler().handle(Event.error("Caught I/O exception: " + e));
       throw new EnvironmentalExecException("unexpected I/O exception", e);
@@ -170,6 +184,7 @@ public class StandaloneTestStrategy extends TestStrategy {
       TestResultData data,
       FileOutErr outErr)
       throws IOException {
+    ImmutableList.Builder<Pair<String, Path>> testOutputsBuilder = new ImmutableList.Builder<>();
     // Rename outputs
     String namePrefix =
         FileSystemUtils.removeExtension(action.getTestLog().getExecPath().getBaseName());
@@ -180,17 +195,21 @@ public class StandaloneTestStrategy extends TestStrategy {
     Path testLog = attemptsDir.getChild(attemptPrefix + ".log");
     if (action.getTestLog().getPath().exists()) {
       action.getTestLog().getPath().renameTo(testLog);
+      testOutputsBuilder.add(Pair.of("test.log", testLog));
     }
     ResolvedPaths resolvedPaths = action.resolve(executor.getExecRoot());
     if (resolvedPaths.getXmlOutputPath().exists()) {
       Path destinationPath = attemptsDir.getChild(attemptPrefix + ".xml");
       resolvedPaths.getXmlOutputPath().renameTo(destinationPath);
+      testOutputsBuilder.add(Pair.of("test.xml", destinationPath));
     }
     // Add the test log to the output
     dataBuilder.addFailedLogs(testLog.toString());
     dataBuilder.addTestTimes(data.getTestTimes(0));
     dataBuilder.addAllTestProcessTimes(data.getTestProcessTimesList());
-    executor.getEventBus().post(new TestAttempt(action, attempt));
+    executor
+        .getEventBus()
+        .post(new TestAttempt(action, attempt, data.getTestPassed(), testOutputsBuilder.build()));
     processTestOutput(executor, outErr, new TestResult(action, data, false), testLog);
   }
 
@@ -365,12 +384,9 @@ public class StandaloneTestStrategy extends TestStrategy {
   }
 
   private final void finalizeTest(
-      int attempt,
-      ActionExecutionContext actionExecutionContext,
-      TestRunnerAction action,
-      TestResultData data)
+      ActionExecutionContext actionExecutionContext, TestRunnerAction action, TestResultData data)
       throws IOException, ExecException {
-    TestResult result = new TestResult(action, data, false, attempt);
+    TestResult result = new TestResult(action, data, false);
     postTestResult(actionExecutionContext.getExecutor(), result);
 
     processTestOutput(
diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/TestAttempt.java b/src/main/java/com/google/devtools/build/lib/rules/test/TestAttempt.java
index dc076b7adb..0364c731ec 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/test/TestAttempt.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/test/TestAttempt.java
@@ -20,16 +20,18 @@ import com.google.devtools.build.lib.buildeventstream.BuildEventId;
 import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos;
 import com.google.devtools.build.lib.buildeventstream.GenericBuildEvent;
 import com.google.devtools.build.lib.buildeventstream.PathConverter;
+import com.google.devtools.build.lib.util.Pair;
+import com.google.devtools.build.lib.vfs.Path;
 import java.util.Collection;
 
-/**
- * This event is raised whenever a an individual test attempt is completed that is not the final
- * attempt for the given test, shard, and run.
- */
+/** This event is raised whenever a an individual test attempt is completed. */
 public class TestAttempt implements BuildEvent {
 
   private final TestRunnerAction testAction;
+  private final boolean success;
   private final int attempt;
+  private final boolean lastAttempt;
+  private final Collection<Pair<String, Path>> files;
 
   /**
    * Construct the event given the test action and attempt number.
@@ -37,9 +39,25 @@ public class TestAttempt implements BuildEvent {
    * @param testAction The test that was run.
    * @param attempt The number of the attempt for this action.
    */
-  public TestAttempt(TestRunnerAction testAction, Integer attempt) {
+  public TestAttempt(
+      TestRunnerAction testAction,
+      Integer attempt,
+      boolean success,
+      Collection<Pair<String, Path>> files,
+      boolean lastAttempt) {
     this.testAction = testAction;
     this.attempt = attempt;
+    this.success = success;
+    this.files = files;
+    this.lastAttempt = lastAttempt;
+  }
+
+  public TestAttempt(
+      TestRunnerAction testAction,
+      Integer attempt,
+      boolean success,
+      Collection<Pair<String, Path>> files) {
+    this(testAction, attempt, success, files, false);
   }
 
   @Override
@@ -53,18 +71,30 @@ public class TestAttempt implements BuildEvent {
 
   @Override
   public Collection<BuildEventId> getChildrenEvents() {
-    return ImmutableList.of(
-        BuildEventId.testResult(
-            testAction.getOwner().getLabel(),
-            testAction.getRunNumber(),
-            testAction.getShardNum(),
-            attempt + 1));
+    if (lastAttempt) {
+      return ImmutableList.of();
+    } else {
+      return ImmutableList.of(
+          BuildEventId.testResult(
+              testAction.getOwner().getLabel(),
+              testAction.getRunNumber(),
+              testAction.getShardNum(),
+              attempt + 1));
+    }
   }
 
   @Override
   public BuildEventStreamProtos.BuildEvent asStreamProto(PathConverter pathConverter) {
-    BuildEventStreamProtos.TestResult.Builder resultBuilder =
+    BuildEventStreamProtos.TestResult.Builder builder =
         BuildEventStreamProtos.TestResult.newBuilder();
-    return GenericBuildEvent.protoChaining(this).setTestResult(resultBuilder.build()).build();
+    builder.setSuccess(success);
+    for (Pair<String, Path> file : files) {
+      builder.addTestActionOutput(
+          BuildEventStreamProtos.File.newBuilder()
+              .setName(file.getFirst())
+              .setUri(pathConverter.apply(file.getSecond()))
+              .build());
+    }
+    return GenericBuildEvent.protoChaining(this).setTestResult(builder.build()).build();
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/TestResult.java b/src/main/java/com/google/devtools/build/lib/rules/test/TestResult.java
index ba3ddd2dcb..f0c2b6d484 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/test/TestResult.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/test/TestResult.java
@@ -14,13 +14,7 @@
 
 package com.google.devtools.build.lib.rules.test;
 
-import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.actions.Artifact;
-import com.google.devtools.build.lib.buildeventstream.BuildEvent;
-import com.google.devtools.build.lib.buildeventstream.BuildEventId;
-import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos;
-import com.google.devtools.build.lib.buildeventstream.GenericBuildEvent;
-import com.google.devtools.build.lib.buildeventstream.PathConverter;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
@@ -29,8 +23,6 @@ import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.view.test.TestStatus.BlazeTestStatus;
 import com.google.devtools.build.lib.view.test.TestStatus.TestResultData;
-import java.util.Collection;
-import javax.annotation.Nullable;
 
 /**
  * This is the event passed from the various test strategies to the {@code RecordingTestListener}
@@ -38,12 +30,11 @@ import javax.annotation.Nullable;
  */
 @ThreadSafe
 @Immutable
-public class TestResult implements BuildEvent {
+public class TestResult {
 
   private final TestRunnerAction testAction;
   private final TestResultData data;
   private final boolean cached;
-  private final @Nullable Integer attempt;
 
   /**
    * Construct the TestResult for the given test / status.
@@ -52,16 +43,10 @@ public class TestResult implements BuildEvent {
    * @param data test result protobuffer.
    * @param cached true if this is a locally cached test result.
    */
-  public TestResult(
-      TestRunnerAction testAction, TestResultData data, boolean cached, @Nullable Integer attempt) {
+  public TestResult(TestRunnerAction testAction, TestResultData data, boolean cached) {
     this.testAction = Preconditions.checkNotNull(testAction);
     this.data = data;
     this.cached = cached;
-    this.attempt = attempt;
-  }
-
-  public TestResult(TestRunnerAction testAction, TestResultData data, boolean cached) {
-    this(testAction, data, cached, null);
   }
 
   public static boolean isBlazeTestStatusPassed(BlazeTestStatus status) {
@@ -145,31 +130,4 @@ public class TestResult implements BuildEvent {
   public TestResultData getData() {
     return data;
   }
-
-  @Override
-  public BuildEventId getEventId() {
-    if (attempt != null) {
-      return BuildEventId.testResult(
-          testAction.getOwner().getLabel(),
-          testAction.getRunNumber(),
-          testAction.getShardNum(),
-          attempt);
-    } else {
-      return BuildEventId.testResult(
-          testAction.getOwner().getLabel(), testAction.getRunNumber(), testAction.getShardNum());
-    }
-  }
-
-  @Override
-  public Collection<BuildEventId> getChildrenEvents() {
-    return ImmutableList.of();
-  }
-
-  @Override
-  public BuildEventStreamProtos.BuildEvent asStreamProto(PathConverter pathConverter) {
-    BuildEventStreamProtos.TestResult.Builder resultBuilder =
-        BuildEventStreamProtos.TestResult.newBuilder();
-    resultBuilder.setSuccess(data.getTestPassed());
-    return GenericBuildEvent.protoChaining(this).setTestResult(resultBuilder.build()).build();
-  }
 }