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

2 files changed, 71 insertions, 80 deletions

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;
-    }
   }
 }