Fix bug in skyframe-based test-suite expansion

It was tracking filtered tests and then applying the filter at the next higher
level.

I also added a bunch of comments - we actually have four implementations of
test suite expansion, and they are not consistent. Sorry about that.

PiperOrigin-RevId: 199629485

1 files changed, 13 insertions, 69 deletions

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) {