Cache flaky tests (remove the cachable flag from test result data)

The cachable flag was only set to false for flaky tests.

Before this CL, we did not cache flaky tests on subsequent runs, instead
rerunning them, even though this is both very expensive and inconsistent with
our normal handling, which is to cache passes but not errors.

If cache_test_results is enabled, we now also cached timed out test results.

If cache_test_results=auto (the default), we now also cache flaky tests.
We still do not cache failed tests; the TestRunnerAction checks if the previous
test result was marked as passed (which also applies to flaky tests, but not
to failed or timed-out tests).

Also add some unit tests.

PiperOrigin-RevId: 161187950

1 files changed, 26 insertions, 20 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/TestRunnerAction.java b/src/main/java/com/google/devtools/build/lib/rules/test/TestRunnerAction.java
index 7726361a49..30333f504e 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/test/TestRunnerAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/test/TestRunnerAction.java
@@ -14,6 +14,7 @@
 
 package com.google.devtools.build.lib.rules.test;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
@@ -271,41 +272,46 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa
   }
 
   /**
-   * Returns the cache from disk, or null if there is an error.
+   * Returns the cache from disk, or null if the file doesn't exist or if there is an error.
    */
   @Nullable
   private TestResultData readCacheStatus() {
     try (InputStream in = cacheStatus.getPath().getInputStream()) {
       return TestResultData.parseFrom(in);
     } catch (IOException expected) {
-
+      return null;
     }
-    return null;
   }
 
   private boolean computeExecuteUnconditionallyFromTestStatus() {
-    if (configuration.cacheTestResults() == TriState.NO || testProperties.isExternal()
-        || (configuration.cacheTestResults() == TriState.AUTO
-            && configuration.getRunsPerTestForLabel(getOwner().getLabel()) > 1)) {
-      return true;
-    }
+    return !canBeCached(
+        configuration.cacheTestResults(),
+        readCacheStatus(),
+        testProperties.isExternal(),
+        configuration.getRunsPerTestForLabel(getOwner().getLabel()));
+  }
 
+  @VisibleForTesting
+  static boolean canBeCached(
+      TriState cacheTestResults, TestResultData prevStatus, boolean isExternal, int runsPerTest) {
+    if (cacheTestResults == TriState.NO) {
+      return false;
+    }
+    if (isExternal) {
+      return false;
+    }
+    if (cacheTestResults == TriState.AUTO && (runsPerTest > 1)) {
+      return false;
+    }
     // Test will not be executed unconditionally - check whether test result exists and is
     // valid. If it is, method will return false and we will rely on the dependency checker
     // to make a decision about test execution.
-    TestResultData status = readCacheStatus();
-    if (status != null) {
-      if (!status.getCachable()) {
-        return true;
-      }
-
-      return (configuration.cacheTestResults() == TriState.AUTO
-          && !status.getTestPassed());
+    if (cacheTestResults == TriState.AUTO && prevStatus != null && !prevStatus.getTestPassed()) {
+      return false;
     }
-
-    // CacheStatus is an artifact, so if it does not exist, the dependency checker will rebuild
-    // it. We can't return "true" here, as it also signals to not accept cached remote results.
-    return false;
+    // Rely on the dependency checker to determine if the test can be cached. Note that the status
+    // is a declared output, so its non-existence also triggers a re-run.
+    return true;
   }
 
   /**