Restore the order of the $implicit_tests test_suite attribute value.
It used to be ordered by target name, but a while ago I changed it to be ordered by rule instantiation (https://github.com/bazelbuild/bazel/commit/59c16f6). Restore the previous order for the sake of consistency with other features of package loading.
My change a while ago was trying to be an optimization: back then we unconditionally sorted the list of Labels of all test targets; now, we only do so where there's at least one test_suite target that needs its $implicit_tests attribute set.
RELNOTES:
PiperOrigin-RevId: 285446710
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java
index c993959..3cd613e 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Package.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Package.java
@@ -1461,11 +1461,8 @@
// current instance here.
buildFile = (InputFile) Preconditions.checkNotNull(targets.get(buildFileLabel.getName()));
- // The Iterable returned by getTargets is sorted, so when we build up the list of tests by
- // processing it in order below, that list will be sorted too.
-
- List<Label> sortedTests = new ArrayList<>();
- List<Rule> implicitTestSuites = new ArrayList<>();
+ List<Label> labelsOfTestTargets = new ArrayList<>();
+ List<Rule> implicitTestSuiteRuleInstances = new ArrayList<>();
Map<Label, InputFile> newInputFiles = new HashMap<>();
for (final Rule rule : getTargets(Rule.class)) {
if (discoverAssumedInputFiles) {
@@ -1485,13 +1482,13 @@
// since clearly this information isn't available at Rule construction
// time, as forward references are permitted.
if (TargetUtils.isTestRule(rule) && !TargetUtils.hasManualTag(rule)) {
- sortedTests.add(rule.getLabel());
+ labelsOfTestTargets.add(rule.getLabel());
}
AttributeMap attributes = NonconfigurableAttributeMapper.of(rule);
if (rule.getRuleClass().equals("test_suite")
&& attributes.get("tests", BuildType.LABEL_LIST).isEmpty()) {
- implicitTestSuites.add(rule);
+ implicitTestSuiteRuleInstances.add(rule);
}
}
@@ -1499,8 +1496,11 @@
addInputFile(inputFile);
}
- for (Rule rule : implicitTestSuites) {
- rule.setAttributeValueByName("$implicit_tests", sortedTests);
+ if (!implicitTestSuiteRuleInstances.isEmpty()) {
+ Collections.sort(labelsOfTestTargets);
+ for (Rule rule : implicitTestSuiteRuleInstances) {
+ rule.setAttributeValueByName("$implicit_tests", labelsOfTestTargets);
+ }
}
return this;
}
diff --git a/src/test/java/com/google/devtools/build/lib/packages/PackageLoadingOptimizationsTest.java b/src/test/java/com/google/devtools/build/lib/packages/PackageLoadingOptimizationsTest.java
index 299fd54..0334665 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/PackageLoadingOptimizationsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/PackageLoadingOptimizationsTest.java
@@ -20,6 +20,7 @@
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.events.NullEventHandler;
import com.google.devtools.build.lib.packages.util.PackageLoadingTestCase;
+import java.util.Collection;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
@@ -126,4 +127,36 @@
assertThat(allLists.get(i).get(0)).isSameInstanceAs(firstList.get(0));
}
}
+
+ @Test
+ public void testSuiteImplicitTestsAttributeValueIsSortedByTargetName() throws Exception {
+ // When we have a BUILD file that instantiates some test targets
+ scratch.file(
+ "foo/BUILD",
+ // (in an order that is not target-name-order),
+ "sh_test(name = 'bTest', srcs = ['test.sh'])",
+ "sh_test(name = 'cTest', srcs = ['test.sh'])",
+ "sh_test(name = 'aTest', srcs = ['test.sh'])",
+ // And also a `test_suite` target, without setting the `test_suite.tests` attribute,
+ "test_suite(name = 'suite')");
+
+ // Then when we load the package,
+ PackageIdentifier fooPkgId = PackageIdentifier.createInMainRepo("foo");
+ Package fooPkg = getPackageManager().getPackage(NullEventHandler.INSTANCE, fooPkgId);
+
+ // And we get the Rule instance for the `test_suite` target,
+ Rule testSuiteRuleInstance = (Rule) fooPkg.getTarget("suite");
+ assertThat(testSuiteRuleInstance.getTargetKind()).isEqualTo("test_suite rule");
+ @SuppressWarnings("unchecked")
+ Collection<Label> implicitTestsAttributeValue =
+ (Collection<Label>)
+ testSuiteRuleInstance.getAttributeContainer().getAttr("$implicit_tests");
+ // The $implicit_tests attribute's value is ordered by target-name.
+ assertThat(implicitTestsAttributeValue)
+ .containsExactly(
+ Label.create(fooPkgId, "aTest"),
+ Label.create(fooPkgId, "bTest"),
+ Label.create(fooPkgId, "cTest"))
+ .inOrder();
+ }
}
