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
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java
index 9c4d5b1..7b35bb8 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java
@@ -158,6 +158,14 @@
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 @@
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 @@
} 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 @@
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 @@
}
ImmutableList.Builder<Artifact> tools = new ImmutableList.Builder<>();
- if (testConfiguration.isPersistentTestRunner()) {
+ if (isPersistentTestRunner()) {
tools.add(testActionExecutable);
tools.add(executionSettings.getExecutable());
tools.addAll(additionalTools.build());
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java
index bf59452..f476e82 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java
@@ -327,6 +327,12 @@
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;
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java
index e79d8d3..2fb04db 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java
@@ -584,7 +584,7 @@
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");
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestTargetProperties.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestTargetProperties.java
index 15a422f..0493dc6 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestTargetProperties.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestTargetProperties.java
@@ -70,13 +70,16 @@
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 @@
// 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 @@
return isExternal;
}
+ public boolean isPersistentTestRunner() {
+ return isPersistentTestRunner;
+ }
+
public ResourceSet getLocalResourceUsage(Label label, boolean usingLocalTestJobs)
throws UserExecException {
if (usingLocalTestJobs) {
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
index e0d24d1..f8c7d1d 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
+++ b/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 @@
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 @@
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()),
diff --git a/src/test/shell/bazel/BUILD b/src/test/shell/bazel/BUILD
index f8d0843..ca9ceb9 100644
--- a/src/test/shell/bazel/BUILD
+++ b/src/test/shell/bazel/BUILD
@@ -911,6 +911,18 @@
)
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",
srcs = ["bazel_workspace_status_test.sh"],
diff --git a/src/test/shell/bazel/persistent_test_runner_test.sh b/src/test/shell/bazel/persistent_test_runner_test.sh
new file mode 100755
index 0000000..67f7028
--- /dev/null
+++ b/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"
