[buck] restore test caching (#25)

* Revert "Remove test caching."

This reverts commit 0fc321f.

* post-revert fixes

README.md

@@ -47,6 +47,17 @@ Buck tries to move fast with respect to its internals. However, for user facing
 - For larger features, a change eventually is put in place that sets the default to the new behavior. e.g. when Skylark becomes the default build file parser.
 - When the removal date is reached, a change is submitted to remove the feature. At this point, the configuration value will still parse, but will not be used by Buck internally.
 
+Updating the Buck executable in AMP
+-----------------------------------
+
+To update `buck.pex` from AMP, run the following in this repo:
+
+    ant
+    ./bin/buck build --config java.target_level=11 --config java.source_level=11  --config python.interpreter=python2 --config python.pex_flags='' buck --show-output
+    cp ./buck-out/gen/<hash>/programs/buck.pex <location-of-AMP-directory>
+
+Note: `<hash>` is an 8 letter SHA prefix that will depend on which buck commit you're on.
+
 License
 -------
 Apache License 2.0

src/com/facebook/buck/android/AndroidInstrumentationTest.java

@@ -309,6 +309,13 @@ private String getPathForResourceJar(PackagedResource packagedResource) {
     return filesystem.resolve(packagedResource.get()).toString();
   }
 
+  @Override
+  public boolean hasTestResultFiles(SourcePathResolverAdapter pathResolver) {
+    Path testResultPath = getProjectFilesystem().resolve(
+        getPathToTestOutputDirectory().resolve(TEST_RESULT_FILE));
+    return testResultPath.toFile().exists();
+  }
+
   @Override
   public Path getPathToTestOutputDirectory() {
     return BuildTargetPaths.getGenPath(

src/com/facebook/buck/apple/AppleTest.java

@@ -262,6 +262,11 @@ public ImmutableSet<String> getContacts() {
     return contacts;
   }
 
+  @Override
+  public boolean hasTestResultFiles(SourcePathResolverAdapter pathResolver) {
+    return getProjectFilesystem().exists(testOutputPath);
+  }
+
   public Pair<ImmutableList<Step>, ExternalTestRunnerTestSpec> getTestCommand(
       ExecutionContext context,
       TestRunningOptions options,

src/com/facebook/buck/apple/AppleTestX.java

@@ -150,6 +150,11 @@ public ImmutableSet<String> getContacts() {
     return contacts;
   }
 
+  @Override
+  public boolean hasTestResultFiles(SourcePathResolverAdapter pathResolver) {
+    return false;
+  }
+
   @Override
   public CoercedTestRunnerSpec getSpecs() {
     return specs;

src/com/facebook/buck/cli/TestCommand.java

@@ -98,6 +98,7 @@
 import java.util.Optional;
 import java.util.ResourceBundle;
 import java.util.Set;
+import java.util.concurrent.ExecutionException;
 import java.util.stream.Collectors;
 import java.util.stream.StreamSupport;
 import javax.annotation.Nullable;
@@ -112,6 +113,9 @@
 
 public class TestCommand extends BuildCommand {
 
+  public static final String USE_RESULTS_CACHE = "use_results_cache";
+  public static final String RERUN_ONLY_FAILING = "rerun_only_failing";
+
   private static final Logger LOG = Logger.get(TestCommand.class);
 
   @Option(name = "--all", usage = "Whether all of the tests should be run. ")
@@ -145,6 +149,14 @@ public class TestCommand extends BuildCommand {
   @Nullable
   private String pathToJavaAgent = null;
 
+  @Option(name = "--no-results-cache", usage = "Whether to use cached test results.")
+  @Nullable
+  private Boolean isResultsCacheDisabled = null;
+
+  @Option(name = "--only-failing", usage = "Don't re-run tests that are cached as passed.")
+  @Nullable
+  private Boolean isOnlyFailing = null;
+
   @Option(name = "--build-filtered", usage = "Whether to build filtered out tests.")
   @Nullable
   private Boolean isBuildFiltered = null;
@@ -209,6 +221,29 @@ public boolean isCodeCoverageEnabled() {
     return isCodeCoverageEnabled;
   }
 
+  public TestRunningOptions.TestResultCacheMode getResultsCacheMode(BuckConfig buckConfig) {
+    // The option is negative (--no-X) but we prefer to reason about positives, in the code.
+    if (isResultsCacheDisabled == null) {
+      boolean isUseResultsCache = buckConfig.getBooleanValue("test", USE_RESULTS_CACHE, false);
+      isResultsCacheDisabled = !isUseResultsCache;
+    }
+    if (isOnlyFailing == null) {
+      isOnlyFailing = buckConfig.getBooleanValue("test", RERUN_ONLY_FAILING, false);
+    }
+
+    if (isCodeCoverageEnabled()) {
+      return TestRunningOptions.TestResultCacheMode.DISABLED;
+    }
+
+    if (isResultsCacheDisabled) {
+      return TestRunningOptions.TestResultCacheMode.DISABLED;
+    }
+    if (isOnlyFailing) {
+      return TestRunningOptions.TestResultCacheMode.ENABLED_IF_PASSED;
+    }
+    return TestRunningOptions.TestResultCacheMode.ENABLED;
+  }
+
   @Override
   public boolean isDebugEnabled() {
     return isDebugEnabled;
@@ -270,6 +305,7 @@ private TestRunningOptions getTestRunningOptions(CommandRunnerParams params) {
             .setRunAllTests(isRunAllTests())
             .setTestSelectorList(testSelectorOptions.getTestSelectorList())
             .setShouldExplainTestSelectorList(testSelectorOptions.shouldExplain())
+            .setTestResultCacheMode(getResultsCacheMode(params.getBuckConfig()))
             .setShufflingTests(isShufflingTests)
             .setPathToXmlTestOutput(Optional.ofNullable(pathToXmlTestOutput))
             .setPathToJavaAgent(Optional.ofNullable(pathToJavaAgent))
@@ -305,7 +341,7 @@ private ExitCode runTestsInternal(
       Build build,
       BuildContext buildContext,
       Iterable<TestRule> testRules)
-      throws InterruptedException, IOException {
+      throws InterruptedException, IOException, ExecutionException {
 
     if (!withDashArguments.isEmpty()) {
       throw new CommandLineException(

src/com/facebook/buck/cli/TestRuleKeyFileHelper.java

@@ -0,0 +1,82 @@
+/*
+ * Copyright 2013-present Facebook, Inc.
+ *
+ * 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.facebook.buck.cli;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+
+import com.facebook.buck.core.build.engine.BuildEngine;
+import com.facebook.buck.core.rulekey.RuleKey;
+import com.facebook.buck.io.filesystem.ProjectFilesystem;
+import com.facebook.buck.core.test.rule.TestRule;
+import com.facebook.buck.step.Step;
+import com.facebook.buck.step.fs.WriteFileStep;
+
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+
+/**
+ * Helper class to annotate and validate an output directory of a test with a rule key.
+ */
+public class TestRuleKeyFileHelper {
+
+  public static final String RULE_KEY_FILE = ".rulekey";
+
+  private final BuildEngine buildEngine;
+
+  public TestRuleKeyFileHelper(BuildEngine buildEngine) {
+    this.buildEngine = buildEngine;
+  }
+
+  /**
+   * Creates a file in the test's output directory and writes the rule key file in it.
+   * @return A {@link Step} that writes the rule key for the test to it's output directory
+   */
+  public Step createRuleKeyInDirStep(TestRule testRule) {
+    RuleKey ruleKey = buildEngine.getRuleKey(testRule.getBuildTarget());
+    return new WriteFileStep(
+        testRule.getProjectFilesystem(),
+        ruleKey.toString(),
+        getRuleKeyFilePath(testRule),
+        /* executable */ false);
+  }
+
+  /**
+   * Checks if a matching rule key file for a test is present in its directoryReturns
+   * @return true if a rule key is written in the specified directory.
+   */
+  public boolean isRuleKeyInDir(TestRule testRule) throws IOException {
+    ProjectFilesystem filesystem = testRule.getProjectFilesystem();
+    Path ruleKeyPath = filesystem.resolve(getRuleKeyFilePath(testRule));
+    if (!Files.isRegularFile(ruleKeyPath)) {
+      return false;
+    }
+
+    RuleKey ruleKey = buildEngine.getRuleKey(testRule.getBuildTarget());
+    try (BufferedReader reader = Files.newBufferedReader(ruleKeyPath, UTF_8)) {
+      return reader.readLine().equals(ruleKey.toString());
+    }
+  }
+
+  /**
+   * Get the path file where the rule key is written, given the path to the output directory.
+   */
+  private Path getRuleKeyFilePath(TestRule testRule) {
+    return testRule.getPathToTestOutputDirectory().resolve(RULE_KEY_FILE);
+  }
+}

src/com/facebook/buck/cli/TestRunning.java

@@ -22,6 +22,8 @@
 import com.facebook.buck.android.HasInstallableApk;
 import com.facebook.buck.core.build.context.BuildContext;
 import com.facebook.buck.core.build.engine.BuildEngine;
+import com.facebook.buck.core.build.engine.BuildResult;
+import com.facebook.buck.core.build.engine.BuildRuleSuccessType;
 import com.facebook.buck.core.build.execution.context.ExecutionContext;
 import com.facebook.buck.core.exceptions.HumanReadableException;
 import com.facebook.buck.core.model.BuildTarget;
@@ -104,6 +106,7 @@
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicReference;
+import java.util.stream.Collectors;
 import javax.annotation.Nullable;
 import javax.xml.parsers.DocumentBuilder;
 import javax.xml.parsers.DocumentBuilderFactory;
@@ -135,7 +138,7 @@ public static int runTests(
       BuildEngine buildEngine,
       BuildContext buildContext,
       SourcePathRuleFinder ruleFinder)
-      throws IOException, InterruptedException {
+    throws IOException, InterruptedException, ExecutionException {
 
     ImmutableSet<JavaLibrary> rulesUnderTestForCoverage;
     // If needed, we first run instrumentation on the class files.
@@ -188,6 +191,7 @@ public static int runTests(
     // ListenableFuture.
     List<ListenableFuture<TestResults>> results = new ArrayList<>();
 
+    TestRuleKeyFileHelper testRuleKeyFileHelper = new TestRuleKeyFileHelper(buildEngine);
     AtomicInteger lastReportedTestSequenceNumber = new AtomicInteger();
     List<TestRun> separateTestRuns = new ArrayList<>();
     List<TestRun> parallelTestRuns = new ArrayList<>();
@@ -200,6 +204,17 @@ public static int runTests(
                   buildContext.getSourcePathResolver(),
                   /*isUsingTestSelectors*/ !options.getTestSelectorList().isEmpty()));
 
+      boolean isTestRunRequired;
+      isTestRunRequired = isTestRunRequiredForTest(
+        test,
+        buildEngine,
+        buildContext,
+        executionContext,
+        testRuleKeyFileHelper,
+        options.getTestResultCacheMode(),
+        resultsInterpreter,
+        !options.getTestSelectorList().isEmpty(),
+        !options.getEnvironmentOverrides().isEmpty());
       Map<String, UUID> testUUIDMap = new HashMap<>();
       AtomicReference<TestStatusMessageEvent.Started> currentTestStatusMessageEvent =
           new AtomicReference<>();
@@ -278,17 +293,24 @@ public void testsDidEnd(List<TestCaseSummary> testCaseSummaries) {
           };
 
       List<Step> steps;
-      params.getBuckEventBus().post(IndividualTestEvent.started(testTargets));
-      ImmutableList.Builder<Step> stepsBuilder = ImmutableList.builder();
-      Preconditions.checkState(buildEngine.isRuleBuilt(test.getBuildTarget()));
-      List<Step> testSteps =
+      if (isTestRunRequired) {
+        params.getBuckEventBus().post(IndividualTestEvent.started(testTargets));
+        ImmutableList.Builder<Step> stepsBuilder = ImmutableList.builder();
+        Preconditions.checkState(buildEngine.isRuleBuilt(test.getBuildTarget()));
+        List<Step> testSteps =
           test.runTests(executionContext, options, buildContext, testReportingCallback);
-      if (!testSteps.isEmpty()) {
-        stepsBuilder.addAll(testSteps);
+        if (!testSteps.isEmpty()) {
+          stepsBuilder.addAll(testSteps);
+          stepsBuilder.add(testRuleKeyFileHelper.createRuleKeyInDirStep(test));
+        }
+        steps = stepsBuilder.build();
+      } else {
+        steps = ImmutableList.of();
       }
-      steps = stepsBuilder.build();
 
-      TestRun testRun = ImmutableTestRun.of(test, steps, resultsInterpreter, testReportingCallback);
+      TestRun testRun = ImmutableTestRun.of(test, steps, getStatusTransformingCallable(
+        isTestRunRequired,
+        resultsInterpreter), testReportingCallback);
 
       // Always run the commands, even if the list of commands as empty. There may be zero
       // commands because the rule is cached, but its results must still be processed.
@@ -572,7 +594,81 @@ public synchronized TestResults call() throws Exception {
     };
   }
 
-  /** Generates the set of Java library rules under test. */
+  private static Callable<TestResults> getStatusTransformingCallable(
+      boolean isTestRunRequired,
+      final Callable<TestResults> originalCallable) {
+    if (isTestRunRequired) {
+      return originalCallable;
+    }
+    return () -> {
+      TestResults originalTestResults = originalCallable.call();
+      List<TestCaseSummary> cachedTestResults = originalTestResults.getTestCases().stream()
+          .map(TestCaseSummary.TO_CACHED_TRANSFORMATION::apply)
+          .collect(Collectors.toList());
+      return TestResults.of(
+          originalTestResults.getBuildTarget(),
+          ImmutableList.copyOf(cachedTestResults),
+          originalTestResults.getContacts(),
+          originalTestResults.getLabels());
+    };
+  }
+
+  @VisibleForTesting
+  static boolean isTestRunRequiredForTest(
+      TestRule test,
+      BuildEngine cachingBuildEngine,
+      BuildContext buildContext,
+      ExecutionContext executionContext,
+      TestRuleKeyFileHelper testRuleKeyFileHelper,
+      TestRunningOptions.TestResultCacheMode resultCacheMode,
+      Callable<TestResults> testResultInterpreter,
+      boolean isRunningWithTestSelectors,
+      boolean hasEnvironmentOverrides)
+      throws IOException, ExecutionException, InterruptedException {
+    boolean isTestRunRequired;
+    BuildResult result;
+    if (executionContext.isDebugEnabled()) {
+      // If debug is enabled, then we should always run the tests as the user is expecting to
+      // hook up a debugger.
+      isTestRunRequired = true;
+    } else if (isRunningWithTestSelectors) {
+      // As a feature to aid developers, we'll assume that when we are using test selectors,
+      // we should always run each test (and never look at the cache.)
+      // TODO(edward) When #3090004 and #3436849 are closed we can respect the cache again.
+      isTestRunRequired = true;
+    } else if (hasEnvironmentOverrides) {
+      // This is rather obtuse, ideally the environment overrides can be hashed and compared...
+      isTestRunRequired = true;
+    } else if (((result = cachingBuildEngine.getBuildRuleResult(
+        test.getBuildTarget())) != null) &&
+        result.getSuccess() == BuildRuleSuccessType.MATCHING_RULE_KEY &&
+        test.hasTestResultFiles(buildContext.getSourcePathResolver()) &&
+        testRuleKeyFileHelper.isRuleKeyInDir(test) &&
+        (resultCacheMode == TestRunningOptions.TestResultCacheMode.ENABLED ||
+            (resultCacheMode == TestRunningOptions.TestResultCacheMode.ENABLED_IF_PASSED &&
+                areTestsSuccessful(testResultInterpreter)))) {
+      // If this build rule's artifacts (which includes the rule's output and its test result
+      // files) are up to date, then no commands are necessary to run the tests. The test result
+      // files will be read from the XML files in interpretTestResults().
+      isTestRunRequired = false;
+    } else {
+      isTestRunRequired = true;
+    }
+    return isTestRunRequired;
+  }
+
+  private static boolean areTestsSuccessful(Callable<TestResults> callable) {
+    try {
+      return callable.call().isSuccess();
+    } catch (Exception ex) {
+      LOG.error(ex);
+      return false;
+    }
+  }
+
+  /**
+   * Generates the set of Java library rules under test.
+   */
   static ImmutableSet<JavaLibrary> getRulesUnderTest(Iterable<TestRule> tests) {
     ImmutableSet.Builder<JavaLibrary> rulesUnderTest = ImmutableSet.builder();