fix accumulating logic around cached tests

there was an error in the accounting of how many tests were actually
executed vs those that were cached. this was a result of not considering
remotely cached tests when accumulating test results. fix the bug and
add some unit tests to prove out functionality, as well as some
javadoc clarifying functionality.

R=janakr

--
MOS_MIGRATED_REVID=86112421

3 files changed, 26 insertions, 6 deletions

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 b0de6cd6e6..2c8bd79409 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
@@ -41,7 +41,7 @@ public class TestResult {
    *
    * @param testAction The test that was run.
    * @param data test result protobuffer.
-   * @param cached true if this is a cached test result.
+   * @param cached true if this is a locally cached test result.
    */
   public TestResult(TestRunnerAction testAction, TestResultData data, boolean cached) {
     this.testAction = Preconditions.checkNotNull(testAction);
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/TestResultAnalyzer.java b/src/main/java/com/google/devtools/build/lib/runtime/TestResultAnalyzer.java
index 78d66dbc22..211192ea31 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/TestResultAnalyzer.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/TestResultAnalyzer.java
@@ -189,14 +189,13 @@ public class TestResultAnalyzer {
     int numCached = existingSummary.numCached();
     int numLocalActionCached = existingSummary.numLocalActionCached();
 
-    if (!existingSummary.actionRan() && !result.isCached()) {
-      // At least one run of the test actually ran uncached.
+    // If a test was neither cached locally nor remotely we say action was taken.
+    if (!(result.isCached() || result.getData().getRemotelyCached())) {
       summaryBuilder.setActionRan(true);
-    }
-
-    if (result.isCached() || result.getData().getRemotelyCached()) {
+    } else {
       numCached++;
     }
+    
     if (result.isCached()) {
       numLocalActionCached++;
     }
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
index 171f15085f..75c7fe2e75 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/TestSummary.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/TestSummary.java
@@ -211,6 +211,12 @@ public class TestSummary implements Comparable<TestSummary> {
       return this;
     }
 
+    /**
+     * Set the number of results cached, locally or remotely.
+     * 
+     * @param numCached number of results cached locally or remotely
+     * @return this Builder
+     */
     public Builder setNumCached(int numCached) {
       checkMutation();
       summary.numCached = numCached;
@@ -329,6 +335,12 @@ public class TestSummary implements Comparable<TestSummary> {
     return status;
   }
 
+  /**
+   * 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() {
     return numCached > 0;
   }
@@ -341,6 +353,9 @@ public class TestSummary implements Comparable<TestSummary> {
     return numLocalActionCached;
   }
 
+  /**
+   * @return number of results that were cached locally or remotely
+   */
   public int numCached() {
     return numCached;
   }
@@ -349,6 +364,12 @@ public class TestSummary implements Comparable<TestSummary> {
     return totalRuns() - numCached;
   }
 
+  /**
+   * 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() {
     return actionRan;
   }