Persistent tests: fix NPE for most test rules

Only the java_test rule currently supports the persistent test runner, and
all other test rules throw NPE. Instead, automatically fall back to the
non-persistent test runner for rules that don't support it, avoiding the NPE.

This requires updating the infrastructure to *not* just check the test config,
but also whether an individual test *action* has the persistent runner enabled.

This allows enabling this feature by default, but having it only apply to rules
that support it.

Note that I am not sure if the persistent test runner works in Bazel since
there are no public tests for it.

Fixes #11519.

Change-Id: I321e7497267e77e668209a541600908b38a32464

Closes #11527.

Change-Id: I321e7497267e77e668209a541600908b38a32464
PiperOrigin-RevId: 319499062

src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java

@@ -158,6 +158,14 @@ public TestActionBuilder setShardCount(int explicitShardCount) {
     return this;
   }
 
+  private boolean isPersistentTestRunner() {
+    return ruleContext
+            .getConfiguration()
+            .getFragment(TestConfiguration.class)
+            .isPersistentTestRunner()
+        && persistentTestRunnerRunfiles != null;
+  }
+
   /**
    * Creates a test action and artifacts for the given rule. The test action will
    * use the specified executable and runfiles.
@@ -188,8 +196,8 @@ private TestParams createTestAction(int shards) {
           PrerequisiteArtifacts.nestedSet(ruleContext, "$test_runtime", TransitionMode.HOST);
       inputsBuilder.addTransitive(testRuntime);
     }
-    TestTargetProperties testProperties = new TestTargetProperties(
-        ruleContext, executionRequirements);
+    TestTargetProperties testProperties =
+        new TestTargetProperties(ruleContext, executionRequirements, isPersistentTestRunner());
 
     // If the test rule does not provide InstrumentedFilesProvider, there's not much that we can do.
     final boolean collectCodeCoverage = config.isCodeCoverageEnabled()
@@ -301,7 +309,7 @@ private TestParams createTestAction(int shards) {
     } else {
       Artifact flagFile = null;
       // The worker spawn runner expects a flag file containg the worker's flags.
-      if (testConfiguration.isPersistentTestRunner()) {
+      if (isPersistentTestRunner()) {
         flagFile = ruleContext.getBinArtifact(ruleContext.getLabel().getName() + "_flag_file.txt");
         inputsBuilder.add(flagFile);
       }
@@ -366,7 +374,7 @@ private TestParams createTestAction(int shards) {
             testConfiguration.runsPerTestDetectsFlakes()
                 && testConfiguration.cancelConcurrentTests();
         RunfilesSupplier testRunfilesSupplier;
-        if (testConfiguration.isPersistentTestRunner()) {
+        if (isPersistentTestRunner()) {
           // Create a RunfilesSupplier from the persistent test runner's runfiles. Pass only the
           // test runner's runfiles to avoid using a different worker for every test run.
           testRunfilesSupplier =
@@ -380,7 +388,7 @@ private TestParams createTestAction(int shards) {
         }
 
         ImmutableList.Builder<Artifact> tools = new ImmutableList.Builder<>();
-        if (testConfiguration.isPersistentTestRunner()) {
+        if (isPersistentTestRunner()) {
           tools.add(testActionExecutable);
           tools.add(executionSettings.getExecutable());
           tools.addAll(additionalTools.build());

src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java

@@ -327,6 +327,12 @@ public TestShardingStrategy testShardingStrategy() {
     return options.testShardingStrategy;
   }
 
+  /**
+   * Whether the persistent test runner is enabled. Note that not all test rules support this
+   * feature, in which case Bazel should fall back to the normal test runner. Therefore, this method
+   * must only be called by test rules, and never for test actions. For actions, use {@code
+   * TestTargetProperties.isPersistentTestRunner} instead.
+   */
   public boolean isPersistentTestRunner() {
     return options.persistentTestRunner;
   }

src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java

@@ -584,7 +584,7 @@ public void setupEnvVariables(Map<String, String> env, Duration timeout) {
       env.put("RUNFILES_MANIFEST_ONLY", "1");
     }
 
-    if (testConfiguration.isPersistentTestRunner()) {
+    if (testProperties.isPersistentTestRunner()) {
       // Let the test runner know it runs persistently within a worker.
       env.put("PERSISTENT_TEST_RUNNER", "true");
     }

src/main/java/com/google/devtools/build/lib/analysis/test/TestTargetProperties.java

@@ -70,13 +70,16 @@ private static ResourceSet getResourceSetFromSize(TestSize size) {
   private final boolean isExternal;
   private final String language;
   private final ImmutableMap<String, String> executionInfo;
+  private final boolean isPersistentTestRunner;
 
   /**
-   * Creates test target properties instance. Constructor expects that it
-   * will be called only for test configured targets.
+   * Creates test target properties instance. Constructor expects that it will be called only for
+   * test configured targets.
    */
-  TestTargetProperties(RuleContext ruleContext,
-      ExecutionInfo executionRequirements) {
+  TestTargetProperties(
+      RuleContext ruleContext,
+      ExecutionInfo executionRequirements,
+      boolean isPersistentTestRunner) {
     Rule rule = ruleContext.getRule();
 
     Preconditions.checkState(TargetUtils.isTestRule(rule));
@@ -87,6 +90,7 @@ private static ResourceSet getResourceSetFromSize(TestSize size) {
     // We need to use method on ruleConfiguredTarget to perform validation.
     isFlaky = ruleContext.attributes().get("flaky", Type.BOOLEAN);
     isExternal = TargetUtils.isExternalTestRule(rule);
+    this.isPersistentTestRunner = isPersistentTestRunner;
 
     Map<String, String> executionInfo = Maps.newLinkedHashMap();
     executionInfo.putAll(TargetUtils.getExecutionInfo(rule));
@@ -133,6 +137,10 @@ public boolean isExternal() {
     return isExternal;
   }
 
+  public boolean isPersistentTestRunner() {
+    return isPersistentTestRunner;
+  }
+
   public ResourceSet getLocalResourceUsage(Label label, boolean usingLocalTestJobs)
       throws UserExecException {
     if (usingLocalTestJobs) {

src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java

@@ -35,7 +35,6 @@
 import com.google.devtools.build.lib.actions.TestExecException;
 import com.google.devtools.build.lib.analysis.actions.SpawnAction;
 import com.google.devtools.build.lib.analysis.test.TestAttempt;
-import com.google.devtools.build.lib.analysis.test.TestConfiguration;
 import com.google.devtools.build.lib.analysis.test.TestResult;
 import com.google.devtools.build.lib.analysis.test.TestRunnerAction;
 import com.google.devtools.build.lib.analysis.test.TestRunnerAction.ResolvedPaths;
@@ -117,7 +116,7 @@ public TestRunnerSpawn createTestRunnerSpawn(
       executionInfo.put(ExecutionRequirements.NO_CACHE, "");
     }
     executionInfo.put(ExecutionRequirements.TIMEOUT, "" + getTimeout(action).getSeconds());
-    if (action.getConfiguration().getFragment(TestConfiguration.class).isPersistentTestRunner()) {
+    if (action.getTestProperties().isPersistentTestRunner()) {
       executionInfo.put(ExecutionRequirements.SUPPORTS_WORKERS, "1");
     }
 
@@ -136,7 +135,7 @@ public TestRunnerSpawn createTestRunnerSpawn(
             action.getRunfilesSupplier(),
             ImmutableMap.of(),
             /*inputs=*/ action.getInputs(),
-            action.getConfiguration().getFragment(TestConfiguration.class).isPersistentTestRunner()
+            action.getTestProperties().isPersistentTestRunner()
                 ? action.getTools()
                 : NestedSetBuilder.emptySet(Order.STABLE_ORDER),
             ImmutableSet.copyOf(action.getSpawnOutputs()),

src/test/shell/bazel/BUILD

@@ -910,6 +910,18 @@ sh_test(
     ],
 )
 
+sh_test(
+    name = "persistent_test_runner_test",
+    size = "large",
+    srcs = ["persistent_test_runner_test.sh"],
+    data = [
+        ":test-deps",
+    ],
+    tags = [
+        "no_windows",
+    ],
+)
+
 sh_test(
     name = "bazel_workspace_status_test",
     size = "large",

src/test/shell/bazel/persistent_test_runner_test.sh

@@ -0,0 +1,45 @@
+#!/bin/bash
+#
+# Copyright 2020 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.
+#
+# Tests the persistent test runner
+#
+
+# Load test environment
+# Load the test setup defined in the parent directory
+CURRENT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+source "${CURRENT_DIR}/../integration_test_setup.sh" \
+  || { echo "integration_test_setup.sh not found!" >&2; exit 1; }
+
+cat >>$TEST_TMPDIR/bazelrc <<'EOF'
+build --strategy=TestRunner=worker,local
+build --experimental_persistent_test_runner
+EOF
+
+function test_simple_sh_test() {
+  mkdir -p examples/tests/
+  cat << 'EOF' > examples/tests/BUILD
+sh_test(
+  name = "shell",
+  srcs = [ "shell.sh" ],
+)
+EOF
+  cat << 'EOF' > examples/tests/shell.sh
+EOF
+  chmod +x examples/tests/shell.sh
+  bazel test examples/tests:shell &> $TEST_log || fail "Shell test failed"
+}
+
+run_suite "persistent_test_runner"