Cleanups for nested test_suite or java_test under trim_test_configuration
For test_suite, no longer make TestConfiguration a hard requirement (similar to https://github.com/bazelbuild/bazel/commit/215a0f70c81a7574d5d9fe5f771446aaf622f21e). Also, split out the older 'temporary hack' of TestTags in TestProvider (used in test_suite filtering) to its own provider, TestTagsProvider, that is now always provided by tests.
For java_test, change [Bazel]JavaSemantics to more gracefully handle a missing TestConfiguration. For persistent test runner related functions, returns false when missing the TestConfiguration (as it is assumed to be irrelevant). For legacy java_test matters, assumes test_arg is empty (as again assumed to be irrelevant).
RELNOTES: NONE
PiperOrigin-RevId: 334547057
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD
index 8e99574..e36237b 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD
@@ -270,6 +270,7 @@
"test/TestResult.java",
"test/TestRunnerAction.java",
"test/TestStrategy.java",
+ "test/TestTagsProvider.java",
"test/TestTargetExecutionSettings.java",
"test/TestTargetProperties.java",
],
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
index 59595f3..e8ec586 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
@@ -42,6 +42,7 @@
import com.google.devtools.build.lib.analysis.test.TestEnvironmentInfo;
import com.google.devtools.build.lib.analysis.test.TestProvider;
import com.google.devtools.build.lib.analysis.test.TestProvider.TestParams;
+import com.google.devtools.build.lib.analysis.test.TestTagsProvider;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
@@ -161,13 +162,16 @@
// Create test action and artifacts if target was successfully initialized
// and is a test. Also, as an extreme hack, only bother doing this if the TestConfiguration
// is actually present.
- if (TargetUtils.isTestRule(ruleContext.getTarget())
- && ruleContext.getConfiguration().hasFragment(TestConfiguration.class)) {
- if (runfilesSupport != null) {
- add(TestProvider.class, initializeTestProvider(filesToRunProvider));
- } else {
- if (!allowAnalysisFailures) {
- throw new IllegalStateException("Test rules must have runfiles");
+ if (TargetUtils.isTestRule(ruleContext.getTarget())) {
+ ImmutableList<String> testTags = ImmutableList.copyOf(ruleContext.getRule().getRuleTags());
+ add(TestTagsProvider.class, new TestTagsProvider(testTags));
+ if (ruleContext.getConfiguration().hasFragment(TestConfiguration.class)) {
+ if (runfilesSupport != null) {
+ add(TestProvider.class, initializeTestProvider(filesToRunProvider));
+ } else {
+ if (!allowAnalysisFailures) {
+ throw new IllegalStateException("Test rules must have runfiles");
+ }
}
}
}
@@ -407,8 +411,7 @@
(ExecutionInfo) providersBuilder.getProvider(ExecutionInfo.PROVIDER.getKey()))
.setShardCount(explicitShardCount)
.build();
- ImmutableList<String> testTags = ImmutableList.copyOf(ruleContext.getRule().getRuleTags());
- return new TestProvider(testParams, testTags);
+ return new TestProvider(testParams);
}
/**
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestProvider.java
index 344633d..d14c16e 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestProvider.java
@@ -22,17 +22,14 @@
import com.google.devtools.build.lib.analysis.TransitiveInfoProvider;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.packages.TestTimeout;
-import java.util.List;
/** A {@link TransitiveInfoProvider} for configured targets that implement test rules. */
@Immutable
public final class TestProvider implements TransitiveInfoProvider {
private final TestParams testParams;
- private final ImmutableList<String> testTags;
- public TestProvider(TestParams testParams, ImmutableList<String> testTags) {
+ public TestProvider(TestParams testParams) {
this.testParams = testParams;
- this.testTags = testTags;
}
/**
@@ -44,14 +41,6 @@
}
/**
- * Temporary hack to allow dependencies on test_suite targets to continue to work for the time
- * being.
- */
- public List<String> getTestTags() {
- return testTags;
- }
-
- /**
* Returns the test status artifacts for a specified configured target
*
* @param target the configured target. Should belong to a test rule.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestTagsProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestTagsProvider.java
new file mode 100644
index 0000000..04afe46
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestTagsProvider.java
@@ -0,0 +1,38 @@
+// Copyright 2014 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.
+
+package com.google.devtools.build.lib.analysis.test;
+
+import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.analysis.TransitiveInfoProvider;
+import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
+
+/**
+ * A {@link TransitiveInfoProvider} for configured targets to remember tags for test rules.
+ *
+ * <p>Temporary hack to allow dependencies on test_suite targets to continue to work for the time
+ * being.
+ */
+@Immutable
+public final class TestTagsProvider implements TransitiveInfoProvider {
+ private final ImmutableList<String> testTags;
+
+ public TestTagsProvider(ImmutableList<String> testTags) {
+ this.testTags = testTags;
+ }
+
+ public ImmutableList<String> getTestTags() {
+ return testTags;
+ }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaSemantics.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaSemantics.java
index b506e3f..2717140 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaSemantics.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaSemantics.java
@@ -711,7 +711,7 @@
if (JavaSemantics.useLegacyJavaTest(ruleContext)) {
TestConfiguration testConfiguration =
ruleContext.getConfiguration().getFragment(TestConfiguration.class);
- if (testConfiguration.getTestArguments().isEmpty()
+ if ((testConfiguration == null || testConfiguration.getTestArguments().isEmpty())
&& !ruleContext.attributes().isAttributeValueExplicitlySpecified("args")) {
ImmutableList.Builder<String> builder = ImmutableList.builder();
for (Artifact artifact : sources) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaSemantics.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaSemantics.java
index c9b0a1d..b677745 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaSemantics.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaSemantics.java
@@ -320,9 +320,19 @@
*/
boolean isJavaExecutableSubstitution();
+ /**
+ * Returns true if target is a test target, has TestConfiguration, and persistent test runner set.
+ *
+ * <p>Note that no TestConfiguration implies the TestConfiguration was pruned in some parent of
+ * the rule. Therefore, TestTarget not currently being analyzed as part of top-level and thus
+ * persistent test runner is not especially relevant.
+ */
static boolean isTestTargetAndPersistentTestRunner(RuleContext ruleContext) {
- return ruleContext.isTestTarget()
- && ruleContext.getFragment(TestConfiguration.class).isPersistentTestRunner();
+ if (!ruleContext.isTestTarget()) {
+ return false;
+ }
+ TestConfiguration testConfiguration = ruleContext.getFragment(TestConfiguration.class);
+ return testConfiguration != null && testConfiguration.isPersistentTestRunner();
}
static Runfiles getTestSupportRunfiles(RuleContext ruleContext) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/TestSuite.java b/src/main/java/com/google/devtools/build/lib/rules/test/TestSuite.java
index 323f0e0..55e9036 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/test/TestSuite.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/test/TestSuite.java
@@ -23,7 +23,7 @@
import com.google.devtools.build.lib.analysis.Runfiles;
import com.google.devtools.build.lib.analysis.RunfilesProvider;
import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
-import com.google.devtools.build.lib.analysis.test.TestProvider;
+import com.google.devtools.build.lib.analysis.test.TestTagsProvider;
import com.google.devtools.build.lib.packages.BuildType;
import com.google.devtools.build.lib.packages.TestTargetUtils;
import com.google.devtools.build.lib.packages.Type;
@@ -65,9 +65,9 @@
Iterables.concat(
getPrerequisites(ruleContext, "tests"),
getPrerequisites(ruleContext, "$implicit_tests"))) {
- if (dep.getProvider(TestProvider.class) != null) {
+ if (dep.getProvider(TestTagsProvider.class) != null) {
// getTestTags maps to Rule.getRuleTags.
- List<String> tags = dep.getProvider(TestProvider.class).getTestTags();
+ List<String> tags = dep.getProvider(TestTagsProvider.class).getTestTags();
if (!TestTargetUtils.testMatchesFilters(
tags, requiredExcluded.first, requiredExcluded.second)) {
// This test does not match our filter. Ignore it.
@@ -105,8 +105,8 @@
for (TransitiveInfoCollection dep : ruleContext.getPrerequisites(attributeName)) {
// TODO(bazel-team): Maybe convert the TransitiveTestsProvider into an inner interface.
TransitiveTestsProvider provider = dep.getProvider(TransitiveTestsProvider.class);
- TestProvider testProvider = dep.getProvider(TestProvider.class);
- if (provider == null && testProvider == null) {
+ TestTagsProvider tagsProvider = dep.getProvider(TestTagsProvider.class);
+ if (provider == null && tagsProvider == null) {
ruleContext.attributeError(attributeName,
"expecting a test or a test_suite rule but '" + dep.getLabel() + "' is not one");
}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/TestSuiteRule.java b/src/main/java/com/google/devtools/build/lib/rules/test/TestSuiteRule.java
index adb99f9..5eafdbb 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/test/TestSuiteRule.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/test/TestSuiteRule.java
@@ -21,6 +21,7 @@
import com.google.devtools.build.lib.analysis.RuleDefinition;
import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment;
import com.google.devtools.build.lib.analysis.test.TestConfiguration;
+import com.google.devtools.build.lib.packages.ConfigurationFragmentPolicy.MissingFragmentPolicy;
import com.google.devtools.build.lib.packages.RuleClass;
/** Rule object implementing "test_suite". */
@@ -29,8 +30,10 @@
public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) {
return builder
// Technically, test_suite does not use TestConfiguration. But the tests it depends on
- // will always depend on TestConfiguration, so requiring it here simply acknowledges that.
+ // will always depend on TestConfiguration, so requiring it here simply acknowledges that
+ // and prevents pruning by --trim_test_configuration.
.requiresConfigurationFragments(TestConfiguration.class)
+ .setMissingFragmentPolicy(TestConfiguration.class, MissingFragmentPolicy.IGNORE)
.override(
attr("testonly", BOOLEAN)
.value(true)
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/test/TrimTestConfigurationTest.java b/src/test/java/com/google/devtools/build/lib/analysis/test/TrimTestConfigurationTest.java
index d6cc9b5..905e7fe 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/test/TrimTestConfigurationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/test/TrimTestConfigurationTest.java
@@ -751,6 +751,51 @@
}
@Test
+ public void flagOnNonTestTargetWithTestSuiteDependencies_IsPermitted() throws Exception {
+ // reporter.removeHandler(failFastHandler);
+ scratch.file(
+ "test/BUILD",
+ "load(':test.bzl', 'starlark_test')",
+ "load(':lib.bzl', 'starlark_lib')",
+ "starlark_lib(",
+ " name = 'starlark_dep',",
+ " deps = [':a_test_suite'],",
+ " testonly = 1,",
+ ")",
+ "starlark_test(",
+ " name = 'starlark_test',",
+ ")",
+ "test_suite(",
+ " name = 'a_test_suite',",
+ " tests = [':starlark_test'],",
+ ")");
+ useConfiguration("--trim_test_configuration", "--noexpand_test_suites", "--test_arg=TypeA");
+ update("//test:starlark_dep");
+ assertThat(getAnalysisResult().getTargetsToBuild()).isNotEmpty();
+ }
+
+ @Test
+ public void flagOnNonTestTargetWithJavaTestDependencies_IsPermitted() throws Exception {
+ // reporter.removeHandler(failFastHandler);
+ scratch.file(
+ "test/BUILD",
+ "load(':lib.bzl', 'starlark_lib')",
+ "starlark_lib(",
+ " name = 'starlark_dep',",
+ " deps = [':JavaTest'],",
+ " testonly = 1,",
+ ")",
+ "java_test(",
+ " name = 'JavaTest',",
+ " srcs = ['JavaTest.java'],",
+ " test_class = 'test.JavaTest',",
+ ")");
+ useConfiguration("--trim_test_configuration", "--noexpand_test_suites", "--test_arg=TypeA");
+ update("//test:starlark_dep");
+ assertThat(getAnalysisResult().getTargetsToBuild()).isNotEmpty();
+ }
+
+ @Test
public void flagOnTestSuiteWithTestDependencies_CanBeAnalyzed() throws Exception {
scratch.file(
"test/BUILD",
diff --git a/src/test/java/com/google/devtools/build/lib/runtime/TestResultAggregatorTest.java b/src/test/java/com/google/devtools/build/lib/runtime/TestResultAggregatorTest.java
index 59627b3..68719d3 100644
--- a/src/test/java/com/google/devtools/build/lib/runtime/TestResultAggregatorTest.java
+++ b/src/test/java/com/google/devtools/build/lib/runtime/TestResultAggregatorTest.java
@@ -51,7 +51,7 @@
when(mockParams.getTimeout()).thenReturn(TestTimeout.LONG);
when(mockParams.getTestStatusArtifacts())
.thenReturn(ImmutableList.<Artifact.DerivedArtifact>of());
- TestProvider testProvider = new TestProvider(mockParams, ImmutableList.<String>of());
+ TestProvider testProvider = new TestProvider(mockParams);
ConfiguredTarget mockTarget = mock(ConfiguredTarget.class);
when(mockTarget.getProvider(TestProvider.class)).thenReturn(testProvider);
diff --git a/src/test/java/com/google/devtools/build/lib/runtime/TestSummaryTest.java b/src/test/java/com/google/devtools/build/lib/runtime/TestSummaryTest.java
index 28a38fc..d7cce6c 100644
--- a/src/test/java/com/google/devtools/build/lib/runtime/TestSummaryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/runtime/TestSummaryTest.java
@@ -253,7 +253,7 @@
when(testParams.getRuns()).thenReturn(2);
when(testParams.getShards()).thenReturn(3);
- TestProvider testProvider = new TestProvider(testParams, ImmutableList.of());
+ TestProvider testProvider = new TestProvider(testParams);
when(stubTarget.getProvider(eq(TestProvider.class))).thenReturn(testProvider);
PathConverter pathConverter = mock(PathConverter.class);
