diff options
Diffstat (limited to 'src/main/java/com/google/devtools')
7 files changed, 94 insertions, 155 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/ResolvedTargets.java b/src/main/java/com/google/devtools/build/lib/cmdline/ResolvedTargets.java index e06f9d7aea..5f4c818682 100644 --- a/src/main/java/com/google/devtools/build/lib/cmdline/ResolvedTargets.java +++ b/src/main/java/com/google/devtools/build/lib/cmdline/ResolvedTargets.java @@ -16,10 +16,9 @@ package com.google.devtools.build.lib.cmdline; import com.google.common.base.Predicate; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Sets; - import java.util.Collection; +import java.util.LinkedHashSet; import java.util.Set; - import javax.annotation.concurrent.Immutable; /** @@ -102,7 +101,7 @@ public final class ResolvedTargets<T> { private volatile boolean hasError = false; private Builder() { - this(Sets.<T>newLinkedHashSet(), Sets.<T>newLinkedHashSet()); + this(new LinkedHashSet<>(), new LinkedHashSet<>()); } private Builder(Set<T> targets, Set<T> filteredTargets) { diff --git a/src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java b/src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java index 133cb71e9c..b1dd3da648 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java +++ b/src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java @@ -297,9 +297,10 @@ public final class TargetUtils { if (!(input instanceof Rule)) { return requiredTags.isEmpty(); } - // Note that test_tags are those originating from the XX_test rule, - // whereas the requiredTags and excludedTags originate from the command - // line or test_suite rule. + // Note that test_tags are those originating from the XX_test rule, whereas the requiredTags + // and excludedTags originate from the command line or test_suite rule. + // TODO(ulfjack): getRuleTags is inconsistent with TestFunction and other places that use + // tags + size, but consistent with TestSuite. return TestTargetUtils.testMatchesFilters( ((Rule) input).getRuleTags(), requiredTags, excludedTags, false); }; diff --git a/src/main/java/com/google/devtools/build/lib/packages/TestTargetUtils.java b/src/main/java/com/google/devtools/build/lib/packages/TestTargetUtils.java index 4dddba9c63..ec68b8194f 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/TestTargetUtils.java +++ b/src/main/java/com/google/devtools/build/lib/packages/TestTargetUtils.java @@ -14,7 +14,6 @@ package com.google.devtools.build.lib.packages; import com.google.common.collect.ImmutableList; -import com.google.common.collect.Sets; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.ResolvedTargets; import com.google.devtools.build.lib.cmdline.TargetParsingException; @@ -25,9 +24,9 @@ import com.google.devtools.build.lib.syntax.Type; import com.google.devtools.build.lib.util.Pair; import java.util.ArrayList; import java.util.Collection; +import java.util.Collections; import java.util.HashMap; import java.util.HashSet; -import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Set; @@ -42,10 +41,11 @@ public final class TestTargetUtils { * Returns whether a test with the specified tags matches a filter (as specified by the set * of its positive and its negative filters). */ - public static boolean testMatchesFilters(Collection<String> testTags, - Collection<String> requiredTags, Collection<String> excludedTags, + public static boolean testMatchesFilters( + Collection<String> testTags, + Collection<String> requiredTags, + Collection<String> excludedTags, boolean mustMatchAllPositive) { - for (String tag : excludedTags) { if (testTags.contains(tag)) { return false; @@ -53,28 +53,70 @@ public final class TestTargetUtils { } // Check required tags, if there are any. - if (!requiredTags.isEmpty()) { - if (mustMatchAllPositive) { - // Require all tags to be present. - for (String tag : requiredTags) { - if (!testTags.contains(tag)) { - return false; - } - } - return true; - } else { - // Require at least one positive tag. - for (String tag : requiredTags) { - if (testTags.contains(tag)) { - return true; - } + if (requiredTags.isEmpty()) { + return true; + } else if (mustMatchAllPositive) { + // Require all tags to be present. + for (String tag : requiredTags) { + if (!testTags.contains(tag)) { + return false; } } - - return false; // No positive tag found. + return true; + } else { + // Require at least one positive tag. If the two collections are not disjoint, then they have + // at least one element in common. + return !Collections.disjoint(requiredTags, testTags); } + } - return true; // No tags are required. + /** + * Decides whether to include a test in a test_suite or not. + * @param testTags Collection of all tags exhibited by a given test. + * @param requiredTags Tags declared by the suite. A Test must match ALL of these. + * @param excludedTags Tags declared by the suite. A Test must match NONE of these. + * @return false is the test is to be removed. + */ + public static boolean testMatchesFilters( + Collection<String> testTags, + Collection<String> requiredTags, + Collection<String> excludedTags) { + return testMatchesFilters( + testTags, requiredTags, excludedTags, /* mustMatchAllPositive= */ true); + } + + /** + * Decides whether to include a test in a test_suite or not. + * @param testTarget A given test target. + * @param requiredTags Tags declared by the suite. A Test must match ALL of these. + * @param excludedTags Tags declared by the suite. A Test must match NONE of these. + * @return false is the test is to be removed. + */ + private static boolean testMatchesFilters( + Rule testTarget, + Collection<String> requiredTags, + Collection<String> excludedTags) { + AttributeMap nonConfigurableAttrs = NonconfigurableAttributeMapper.of(testTarget); + Set<String> testTags = new HashSet<>(nonConfigurableAttrs.get("tags", Type.STRING_LIST)); + testTags.add(nonConfigurableAttrs.get("size", Type.STRING)); + return testMatchesFilters(testTags, requiredTags, excludedTags); + } + + /** + * Filters 'tests' (by mutation) according to the 'tags' attribute, specifically those that + * match ALL of the tags in tagsAttribute. + * + * @precondition {@code env.getAccessor().isTestSuite(testSuite)} + * @precondition {@code env.getAccessor().isTestRule(test)} for all test in tests + */ + public static void filterTests(Rule testSuite, Set<Target> tests) { + List<String> tagsAttribute = + NonconfigurableAttributeMapper.of(testSuite).get("tags", Type.STRING_LIST); + // Split the tags list into positive and negative tags + Pair<Collection<String>, Collection<String>> tagLists = sortTagsBySense(tagsAttribute); + Collection<String> positiveTags = tagLists.first; + Collection<String> negativeTags = tagLists.second; + tests.removeIf((Target t) -> !testMatchesFilters((Rule) t, positiveTags, negativeTags)); } /** @@ -178,7 +220,7 @@ public final class TestTargetUtils { private Set<Target> getTestsInSuite(Rule testSuite) throws TargetParsingException { Set<Target> tests = testsInSuite.get(testSuite); if (tests == null) { - tests = Sets.newHashSet(); + tests = new HashSet<>(); testsInSuite.put(testSuite, tests); // break cycles by inserting empty set early. computeTestsInSuite(testSuite, tests); } @@ -189,7 +231,7 @@ public final class TestTargetUtils { * Populates 'result' with all the tests associated with the specified * 'testSuite'. Throws an exception if any target is missing. * - * CAUTION! Keep this logic consistent with {@code TestsSuiteConfiguredTarget}! + * CAUTION! Keep this logic consistent with {@code TestSuite} and {@code TestsInSuiteFunction}! */ private void computeTestsInSuite(Rule testSuite, Set<Target> result) throws TargetParsingException { @@ -263,57 +305,5 @@ public final class TestTargetUtils { throw new TargetParsingException("interrupted", e); } } - - /** - * Filters 'tests' (by mutation) according to the 'tags' attribute, specifically those that - * match ALL of the tags in tagsAttribute. - * - * @precondition {@code env.getAccessor().isTestSuite(testSuite)} - * @precondition {@code env.getAccessor().isTestRule(test)} for all test in tests - */ - private void filterTests(Rule testSuite, Set<Target> tests) { - List<String> tagsAttribute = - NonconfigurableAttributeMapper.of(testSuite).get("tags", Type.STRING_LIST); - // Split the tags list into positive and negative tags - Pair<Collection<String>, Collection<String>> tagLists = sortTagsBySense(tagsAttribute); - Collection<String> positiveTags = tagLists.first; - Collection<String> negativeTags = tagLists.second; - - Iterator<Target> it = tests.iterator(); - while (it.hasNext()) { - Rule test = (Rule) it.next(); - AttributeMap nonConfigurableAttributes = NonconfigurableAttributeMapper.of(test); - List<String> testTags = - new ArrayList<>(nonConfigurableAttributes.get("tags", Type.STRING_LIST)); - testTags.add(nonConfigurableAttributes.get("size", Type.STRING)); - if (!includeTest(testTags, positiveTags, negativeTags)) { - it.remove(); - } - } - } - - /** - * Decides whether to include a test in a test_suite or not. - * @param testTags Collection of all tags exhibited by a given test. - * @param positiveTags Tags declared by the suite. A Test must match ALL of these. - * @param negativeTags Tags declared by the suite. A Test must match NONE of these. - * @return false is the test is to be removed. - */ - private static boolean includeTest(Collection<String> testTags, - Collection<String> positiveTags, Collection<String> negativeTags) { - // Add this test if it matches ALL of the positive tags and NONE of the - // negative tags in the tags attribute. - for (String tag : negativeTags) { - if (testTags.contains(tag)) { - return false; - } - } - for (String tag : positiveTags) { - if (!testTags.contains(tag)) { - return false; - } - } - return true; - } } } diff --git a/src/main/java/com/google/devtools/build/lib/query2/engine/TestsFunction.java b/src/main/java/com/google/devtools/build/lib/query2/engine/TestsFunction.java index 0331d480c5..7119cb47ad 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/engine/TestsFunction.java +++ b/src/main/java/com/google/devtools/build/lib/query2/engine/TestsFunction.java @@ -85,6 +85,8 @@ public class TestsFunction implements QueryFunction { }); } + // TODO(ulfjack): This must match the code in TestTargetUtils. However, we don't currently want + // to depend on the packages library. Extract to a neutral place? /** * Decides whether to include a test in a test_suite or not. * @param testTags Collection of all tags exhibited by a given test. @@ -254,6 +256,7 @@ public class TestsFunction implements QueryFunction { Iterator<T> it = tests.iterator(); while (it.hasNext()) { T test = it.next(); + // TODO(ulfjack): This does not match the code used for TestSuite. List<String> testTags = new ArrayList<>(env.getAccessor().getStringListAttr(test, "tags")); testTags.add(env.getAccessor().getStringAttr(test, "size")); if (!includeTest(testTags, requiredTags, excludedTags)) { 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 5767d83f7c..26a8a9f71c 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 @@ -52,6 +52,7 @@ public class TestSuite implements RuleConfiguredTargetFactory { List<String> tagsAttribute = new ArrayList<>( ruleContext.attributes().get("tags", Type.STRING_LIST)); + // TODO(ulfjack): This is inconsistent with the other places that do test_suite expansion. tagsAttribute.remove("manual"); Pair<Collection<String>, Collection<String>> requiredExcluded = TestTargetUtils.sortTagsBySense(tagsAttribute); @@ -66,9 +67,10 @@ public class TestSuite implements RuleConfiguredTargetFactory { getPrerequisites(ruleContext, "tests"), getPrerequisites(ruleContext, "$implicit_tests"))) { if (dep.getProvider(TestProvider.class) != null) { + // getTestTags maps to Rule.getRuleTags. List<String> tags = dep.getProvider(TestProvider.class).getTestTags(); if (!TestTargetUtils.testMatchesFilters( - tags, requiredExcluded.first, requiredExcluded.second, true)) { + tags, requiredExcluded.first, requiredExcluded.second)) { // This test does not match our filter. Ignore it. continue; } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java index 42265a46f1..4c8cf80a27 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java @@ -163,8 +163,8 @@ final class SkyframeTargetPatternEvaluator implements TargetPatternEvaluator { private TargetPatternsResultBuilder createTargetPatternEvaluatorUtil( FilteringPolicy policy, ExtendedEventHandler eventHandler, boolean keepGoing) { return policy == FilteringPolicies.FILTER_TESTS - ? new TestTargetPatternsResultBuilder(skyframeExecutor.getPackageManager(), eventHandler, - keepGoing) + ? new TestTargetPatternsResultBuilder( + skyframeExecutor.getPackageManager(), eventHandler, keepGoing) : new BuildTargetPatternsResultBuilder(); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TestsInSuiteFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TestsInSuiteFunction.java index 099ef643f2..67574159c0 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TestsInSuiteFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TestsInSuiteFunction.java @@ -13,12 +13,11 @@ // limitations under the License. package com.google.devtools.build.lib.skyframe; -import com.google.common.base.Predicate; +import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.cmdline.ResolvedTargets; import com.google.devtools.build.lib.events.Event; -import com.google.devtools.build.lib.packages.AttributeMap; import com.google.devtools.build.lib.packages.BuildFileNotFoundException; import com.google.devtools.build.lib.packages.BuildType; import com.google.devtools.build.lib.packages.NoSuchTargetException; @@ -29,15 +28,13 @@ import com.google.devtools.build.lib.packages.Target; import com.google.devtools.build.lib.packages.TargetUtils; import com.google.devtools.build.lib.packages.TestTargetUtils; import com.google.devtools.build.lib.skyframe.TestsInSuiteValue.TestsInSuiteKey; -import com.google.devtools.build.lib.syntax.Type; -import com.google.devtools.build.lib.util.Pair; import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; import com.google.devtools.build.skyframe.ValueOrException; import java.util.ArrayList; -import java.util.Collection; import java.util.HashMap; +import java.util.HashSet; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; @@ -79,16 +76,18 @@ final class TestsInSuiteFunction implements SkyFunction { */ private static ResolvedTargets<Label> computeTestsInSuite( Environment env, Rule testSuite, boolean strict) throws InterruptedException { - ResolvedTargets.Builder<Target> targetsBuilder = ResolvedTargets.builder(); + Set<Target> result = new HashSet<>(); + boolean hasError = false; + List<Target> testsAndSuites = new ArrayList<>(); // Note that testsAndSuites can contain input file targets; the test_suite rule does not // restrict the set of targets that can appear in tests or suites. - targetsBuilder.mergeError(getPrerequisites(env, testSuite, "tests", testsAndSuites)); + hasError |= getPrerequisites(env, testSuite, "tests", testsAndSuites); // 1. Add all tests for (Target test : testsAndSuites) { if (TargetUtils.isTestRule(test)) { - targetsBuilder.add(test); + result.add(test); } else if (strict && !TargetUtils.isTestSuiteRule(test)) { // If strict mode is enabled, then give an error for any non-test, non-test-suite targets. // TODO(ulfjack): We need to throw to end the process if we happen to be in --nokeep_going, @@ -97,32 +96,28 @@ final class TestsInSuiteFunction implements SkyFunction { "in test_suite rule '" + testSuite.getLabel() + "': expecting a test or a test_suite rule but '" + test.getLabel() + "' is not one.")); - targetsBuilder.setError(); + hasError = true; } } // 2. Add implicit dependencies on tests in same package, if any. List<Target> implicitTests = new ArrayList<>(); - targetsBuilder.mergeError(getPrerequisites(env, testSuite, "$implicit_tests", implicitTests)); + hasError |= getPrerequisites(env, testSuite, "$implicit_tests", implicitTests); for (Target target : implicitTests) { // The Package construction of $implicit_tests ensures that this check never fails, but we // add it here anyway for compatibility with future code. if (TargetUtils.isTestRule(target)) { - targetsBuilder.add(target); + result.add(target); } } // 3. Filter based on tags, size, env. - filterTests(testSuite, targetsBuilder); + TestTargetUtils.filterTests(testSuite, result); // 4. Expand all suites recursively, collecting labels. - ResolvedTargets<Target> targets = targetsBuilder.build(); ResolvedTargets.Builder<Label> labelsBuilder = ResolvedTargets.builder(); - labelsBuilder.merge( - new ResolvedTargets<>( - toLabels(targets.getTargets()), - toLabels(targets.getFilteredTargets()), - targets.hasError())); + // Don't set filtered targets; they would be removed from the containing test suite. + labelsBuilder.merge(new ResolvedTargets<>(toLabels(result), ImmutableSet.of(), hasError)); for (Target suite : testsAndSuites) { if (TargetUtils.isTestSuiteRule(suite)) { @@ -187,57 +182,6 @@ final class TestsInSuiteFunction implements SkyFunction { return hasError; } - /** - * Filters 'tests' (by mutation) according to the 'tags' attribute, specifically those that - * match ALL of the tags in tagsAttribute. - */ - private static void filterTests(Rule testSuite, ResolvedTargets.Builder<Target> tests) { - List<String> tagsAttribute = - NonconfigurableAttributeMapper.of(testSuite).get("tags", Type.STRING_LIST); - // Split the tags list into positive and negative tags. - Pair<Collection<String>, Collection<String>> tagLists = - TestTargetUtils.sortTagsBySense(tagsAttribute); - final Collection<String> positiveTags = tagLists.first; - final Collection<String> negativeTags = tagLists.second; - - tests.filter(new Predicate<Target>() { - @Override - public boolean apply(@Nullable Target input) { - Rule test = (Rule) input; - AttributeMap nonConfigurableAttributes = NonconfigurableAttributeMapper.of(test); - List<String> testTags = - new ArrayList<>(nonConfigurableAttributes.get("tags", Type.STRING_LIST)); - testTags.add(nonConfigurableAttributes.get("size", Type.STRING)); - return includeTest(testTags, positiveTags, negativeTags); - } - }); - } - - /** - * Decides whether to include a test in a test_suite or not. - * - * @param testTags Collection of all tags exhibited by a given test. - * @param positiveTags Tags declared by the suite. A Test must match ALL of these. - * @param negativeTags Tags declared by the suite. A Test must match NONE of these. - * @return false if the test is to be removed. - */ - private static boolean includeTest(Collection<String> testTags, - Collection<String> positiveTags, Collection<String> negativeTags) { - // Add this test if it matches ALL of the positive tags and NONE of the - // negative tags in the tags attribute. - for (String tag : negativeTags) { - if (testTags.contains(tag)) { - return false; - } - } - for (String tag : positiveTags) { - if (!testTags.contains(tag)) { - return false; - } - } - return true; - } - @Nullable @Override public String extractTag(SkyKey skyKey) { |