BEP: Use TestStatus enum to report the status of a TestResult

-- Change-Id: I07c15f7d232a3e9363ebedfb9b5523999630c401 Reviewed-on: https://cr.bazel.build/9450 PiperOrigin-RevId: 150887776 MOS_MIGRATED_REVID=150887776
sigaev · Mar 23, 2017 · 7a2597d · 7a2597d
1 parent 3e6bebf
commit 7a2597d
Show file tree

Hide file tree

Showing 7 changed files with 108 additions and 72 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/BUILD b/src/main/java/com/google/devtools/build/lib/BUILD
@@ -535,6 +535,7 @@ java_library(
         ],
     ) + [
         "runtime/BlazeServerStartupOptions.java",
+        "runtime/BuildEventStreamerUtils.java",
     ],
     exports = [
         ":transitive-info-provider",

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
@@ -261,9 +261,24 @@ message TargetComplete {
   repeated string tag = 3;
 }
 
+enum TestStatus {
+  NO_STATUS = 0;
+  PASSED = 1;
+  FLAKY = 2;
+  TIMEOUT = 3;
+  FAILED = 4;
+  INCOMPLETE = 5;
+  REMOTE_FAILURE = 6;
+  FAILED_TO_BUILD = 7;
+  BLAZE_HALTED_BEFORE_TESTING = 8;
+};
+
 // Payload on events reporting about individual test action.
 message TestResult {
-  bool success = 1;
+  reserved 1;
+
+  // The status of this test.
+  TestStatus status = 5;
 
   // True, if the reported attempt is taken from the tool's local cache.
   bool cached_locally = 4;
@@ -281,18 +296,6 @@ message TestResult {
 // TODO(aehlig): extend event with additional information as soon as we known
 // which additional information we need for test summaries.
 message TestSummary {
-  enum TestStatus {
-    NO_STATUS = 0;
-    PASSED = 1;
-    FLAKY = 2;
-    TIMEOUT = 3;
-    FAILED = 4;
-    INCOMPLETE = 5;
-    REMOTE_FAILURE = 6;
-    FAILED_TO_BUILD = 7;
-    BLAZE_HALTED_BEFORE_TESTING = 8;
-  };
-
   // Wrapper around BlazeTestStatus to support importing that enum to proto3.
   // Overall status of test, accumulated over all runs, shards, and attempts.
   TestStatus overall_status = 5;

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
@@ -170,8 +170,12 @@ public void exec(TestRunnerAction action, ActionExecutionContext actionExecution
           .getEventBus()
           .post(
               new TestAttempt(
-                  action, attempt, data.getTestPassed(), data.getRunDurationMillis(),
-                  testOutputsBuilder.build(), true));
+                  action,
+                  attempt,
+                  data.getStatus(),
+                  data.getRunDurationMillis(),
+                  testOutputsBuilder.build(),
+                  true));
       finalizeTest(actionExecutionContext, action, dataBuilder.build());
     } catch (IOException e) {
       executor.getEventHandler().handle(Event.error("Caught I/O exception: " + e));
@@ -214,8 +218,12 @@ private void processFailedTestAttempt(
         .getEventBus()
         .post(
             new TestAttempt(
-                action, attempt, data.getTestPassed(), data.getRunDurationMillis(),
-                testOutputsBuilder.build(), false));
+                action,
+                attempt,
+                data.getStatus(),
+                data.getRunDurationMillis(),
+                testOutputsBuilder.build(),
+                false));
     processTestOutput(executor, outErr, new TestResult(action, data, false), testLog);
   }
 

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
@@ -20,16 +20,18 @@
 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.runtime.BuildEventStreamerUtils;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.view.test.TestStatus.BlazeTestStatus;
 import com.google.devtools.build.lib.view.test.TestStatus.TestResultData;
 import java.util.Collection;
 
-/** This event is raised whenever a an individual test attempt is completed. */
+/** This event is raised whenever an individual test attempt is completed. */
 public class TestAttempt implements BuildEvent {
 
   private final TestRunnerAction testAction;
-  private final boolean success;
+  private final BlazeTestStatus status;
   private final boolean cachedLocally;
   private final int attempt;
   private final boolean lastAttempt;
@@ -47,13 +49,13 @@ public TestAttempt(
       boolean cachedLocally,
       TestRunnerAction testAction,
       Integer attempt,
-      boolean success,
+      BlazeTestStatus status,
       long durationMillis,
       Collection<Pair<String, Path>> files,
       boolean lastAttempt) {
     this.testAction = testAction;
     this.attempt = attempt;
-    this.success = success;
+    this.status = status;
     this.cachedLocally = cachedLocally;
     this.durationMillis = durationMillis;
     this.files = files;
@@ -63,28 +65,28 @@ public TestAttempt(
   public TestAttempt(
       TestRunnerAction testAction,
       Integer attempt,
-      boolean success,
+      BlazeTestStatus status,
       long durationMillis,
       Collection<Pair<String, Path>> files,
       boolean lastAttempt) {
-    this(false, testAction, attempt, success, durationMillis, files, lastAttempt);
+    this(false, testAction, attempt, status, durationMillis, files, lastAttempt);
   }
 
   public TestAttempt(
       TestRunnerAction testAction,
       Integer attempt,
-      boolean success,
+      BlazeTestStatus status,
       Collection<Pair<String, Path>> files,
       boolean lastAttempt) {
-    this(testAction, attempt, success, 0, files, lastAttempt);
+    this(testAction, attempt, status, 0, files, lastAttempt);
   }
 
   public TestAttempt(
       TestRunnerAction testAction,
       Integer attempt,
-      boolean success,
+      BlazeTestStatus status,
       Collection<Pair<String, Path>> files) {
-    this(testAction, attempt, success, files, false);
+    this(testAction, attempt, status, files, false);
   }
 
   public static TestAttempt fromCachedTestResult(TestResult result) {
@@ -93,7 +95,7 @@ public static TestAttempt fromCachedTestResult(TestResult result) {
         true,
         result.getTestAction(),
         1,
-        data.getTestPassed(),
+        data.getStatus(),
         data.getRunDurationMillis(),
         result.getFiles(),
         true);
@@ -126,7 +128,7 @@ public Collection<BuildEventId> getChildrenEvents() {
   public BuildEventStreamProtos.BuildEvent asStreamProto(PathConverter pathConverter) {
     BuildEventStreamProtos.TestResult.Builder builder =
         BuildEventStreamProtos.TestResult.newBuilder();
-    builder.setSuccess(success);
+    builder.setStatus(BuildEventStreamerUtils.bepStatus(status));
     builder.setCachedLocally(cachedLocally);
     builder.setTestAttemptDurationMillis(durationMillis);
     for (Pair<String, Path> file : files) {

diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BuildEventStreamerUtils.java b/src/main/java/com/google/devtools/build/lib/runtime/BuildEventStreamerUtils.java
@@ -0,0 +1,55 @@
+// Copyright 2017 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.runtime;
+
+import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos;
+import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.TestStatus;
+import com.google.devtools.build.lib.view.test.TestStatus.BlazeTestStatus;
+
+/**
+ * Utility methods for the build event stream.
+ *
+ * <p>TODO(aehlig): remove once proto to proto dependencies are available and pass through the
+ * BlazeTestStatus.
+ */
+public final class BuildEventStreamerUtils {
+
+  /** Map BlazeTestStatus to TestStatus. */
+  public static TestStatus bepStatus(BlazeTestStatus status) {
+    switch (status) {
+      case NO_STATUS:
+        return BuildEventStreamProtos.TestStatus.NO_STATUS;
+      case PASSED:
+        return BuildEventStreamProtos.TestStatus.PASSED;
+      case FLAKY:
+        return BuildEventStreamProtos.TestStatus.FLAKY;
+      case FAILED:
+        return BuildEventStreamProtos.TestStatus.FAILED;
+      case TIMEOUT:
+        return BuildEventStreamProtos.TestStatus.TIMEOUT;
+      case INCOMPLETE:
+        return BuildEventStreamProtos.TestStatus.INCOMPLETE;
+      case REMOTE_FAILURE:
+        return BuildEventStreamProtos.TestStatus.REMOTE_FAILURE;
+      case BLAZE_HALTED_BEFORE_TESTING:
+        return BuildEventStreamProtos.TestStatus.BLAZE_HALTED_BEFORE_TESTING;
+      default:
+        // Not used as the above is a complete case distinction; however, by the open
+        // nature of protobuf enums, we need the clause to convice java, that we always
+        // have a return statement.
+        return BuildEventStreamProtos.TestStatus.NO_STATUS;
+    }
+  }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/TestSummary.java b/src/main/java/com/google/devtools/build/lib/runtime/TestSummary.java
@@ -219,7 +219,7 @@ public Builder setActionRan(boolean actionRan) {
 
     /**
      * Set the number of results cached, locally or remotely.
-     * 
+     *
      * @param numCached number of results cached locally or remotely
      * @return this Builder
      */
@@ -342,9 +342,8 @@ public BlazeTestStatus getStatus() {
   }
 
   /**
-   * Whether or not any results associated with this test were cached locally
-   * or remotely.
-   * 
+   * Whether or not any results associated with this test were cached locally or remotely.
+   *
    * @return true if any results were cached, false if not
    */
   public boolean isCached() {
@@ -371,9 +370,9 @@ private int numUncached() {
   }
 
   /**
-   * Whether or not any action was taken for this test, that is there was some
-   * result that was <em>not cached</em>.
-   * 
+   * Whether or not any action was taken for this test, that is there was some result that was
+   * <em>not cached</em>.
+   *
    * @return true if some action was taken for this test, false if not
    */
   public boolean actionRan() {
@@ -463,43 +462,11 @@ public Collection<BuildEventId> getChildrenEvents() {
     return ImmutableList.of();
   }
 
-  /**
-   * Map BlazeTestStatus to TestSummary.TestStatus.
-   *
-   * <p>TODO(aehlig): remove once proto to proto dependencies are available and pass through the
-   * BlazeTestStatus.
-   */
-  private BuildEventStreamProtos.TestSummary.TestStatus bepStatus() {
-    switch (status) {
-      case NO_STATUS:
-        return BuildEventStreamProtos.TestSummary.TestStatus.NO_STATUS;
-      case PASSED:
-        return BuildEventStreamProtos.TestSummary.TestStatus.PASSED;
-      case FLAKY:
-        return BuildEventStreamProtos.TestSummary.TestStatus.FLAKY;
-      case FAILED:
-        return BuildEventStreamProtos.TestSummary.TestStatus.FAILED;
-      case TIMEOUT:
-        return BuildEventStreamProtos.TestSummary.TestStatus.TIMEOUT;
-      case INCOMPLETE:
-        return BuildEventStreamProtos.TestSummary.TestStatus.INCOMPLETE;
-      case REMOTE_FAILURE:
-        return BuildEventStreamProtos.TestSummary.TestStatus.REMOTE_FAILURE;
-      case BLAZE_HALTED_BEFORE_TESTING:
-        return BuildEventStreamProtos.TestSummary.TestStatus.BLAZE_HALTED_BEFORE_TESTING;
-      default:
-        // Not used as the above is a complete case distinction; however, by the open
-        // nature of protobuf enums, we need the clause to convice java, that we always
-        // have a return statement.
-        return BuildEventStreamProtos.TestSummary.TestStatus.NO_STATUS;
-    }
-  }
-
   @Override
   public BuildEventStreamProtos.BuildEvent asStreamProto(PathConverter pathConverter) {
     BuildEventStreamProtos.TestSummary.Builder summaryBuilder =
         BuildEventStreamProtos.TestSummary.newBuilder()
-            .setOverallStatus(bepStatus())
+            .setOverallStatus(BuildEventStreamerUtils.bepStatus(status))
             .setTotalRunCount(totalRuns());
     for (Path path : getFailedLogs()) {
       summaryBuilder.addFailed(

diff --git a/src/test/shell/integration/build_event_stream_test.sh b/src/test/shell/integration/build_event_stream_test.sh
@@ -123,7 +123,7 @@ function test_test_summary() {
   expect_log_once '^test_summary '
   expect_log_once '^progress '
   expect_not_log 'aborted'
-  expect_log_once 'status.*PASSED'
+  expect_log 'status.*PASSED'
   expect_not_log 'status.*FAILED'
   expect_not_log 'status.*FLAKY'
 }
@@ -137,7 +137,7 @@ function test_test_inidivual_results() {
     || fail "bazel test failed"
   expect_log '^test_result'
   expect_log 'run.*1'
-  expect_log 'success.*true'
+  expect_log 'status.*PASSED'
   expect_log_once '^test_summary '
   expect_log_once '^progress '
   expect_not_log 'aborted'
@@ -155,7 +155,7 @@ function test_test_attempts() {
   expect_log 'attempt.*2$'
   expect_log 'attempt.*3$'
   expect_log_once '^test_summary '
-  expect_log_once 'status.*FAILED'
+  expect_log 'status.*FAILED'
   expect_not_log 'status.*PASSED'
   expect_not_log 'status.*FLAKY'
   expect_log_once '^progress '