Add more assertions about test suite expansion behavior

Characterizing this correctly this time: Target patterns are evaluated with two different sets of semantics. For determining which targets to build, only test suites in exclusion target patterns are expanded. For determining which tests to run (or determining which targets to build if --build_tests_only is set), test suites in all target patterns are expanded. In each case, test suites are expanded for each target pattern individually, not for the whole set of targets after the list of target patterns is processed.

Also update a related code comment.

The newer implementation for this logic already has equivalent tests for this behavior, in particular testFilterNegativeTestFromTestSuite and testNegativeTestSuiteExpanded in LoadingPhaseRunnerTest.

RELNOTES: None
PiperOrigin-RevId: 174756583

src/main/java/com/google/devtools/build/lib/skyframe/TestTargetPatternsResultBuilder.java

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TestTargetPatternsResultBuilder.java b/src/main/java/com/google/devtools/build/lib/skyframe/TestTargetPatternsResultBuilder.java
index 823d107..0a53f75 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TestTargetPatternsResultBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TestTargetPatternsResultBuilder.java
@@ -24,8 +24,12 @@
 import java.util.ArrayList;
 
 /**
- * Evaluates set of test targets based on list of target patterns. In contradistinction to 
- * {@code BuildTargetPatternEvaluatorUtil} this class will expand all test suites. 
+ * Evaluates the set of test targets based on list of target patterns. Test suite expansion is
+ * performed on inclusion and exclusion target patterns before adding or subtracting from the set.
+ * This differs from the behavior of {@code BuildTargetPatternsResultBuilder}, which only applies
+ * test suite expansion to inclusion target patterns. (The result of this is that excluding a test
+ * suite excludes all of the referenced tests from the set of tests to be run, but does not exclude
+ * those from the set of targets to be built unless --build_tests_only is set.)
  */
 class TestTargetPatternsResultBuilder extends TargetPatternsResultBuilder {
   private final ArrayList<ResolvedTargetsOfPattern> labelsOfPatterns = new ArrayList<>(); 

src/test/java/com/google/devtools/build/lib/pkgcache/TargetPatternEvaluatorTest.java

diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/TargetPatternEvaluatorTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/TargetPatternEvaluatorTest.java
index bf8a2cb..b62e5a8 100644
--- a/src/test/java/com/google/devtools/build/lib/pkgcache/TargetPatternEvaluatorTest.java
+++ b/src/test/java/com/google/devtools/build/lib/pkgcache/TargetPatternEvaluatorTest.java
@@ -807,15 +807,24 @@
 
   @Test
   public void testTestSuiteExclusion() throws Exception {
-    // This step doesn't do test_suite expansion of the individual target patterns. Doing so
-    // would cause an exclusion of the targets in a package to exclude all tests referred to in test
-    // suites in that package.
+    // Test suites are expanded differently depending on if FILTER_TESTS is used. Those semantics
+    // are used for determining what tests are run (and for determining what targets are built
+    // only if --build_only_tests is set). Test suites are expanded for each target pattern
+    // in sequence, not the whole set of target patterns after all the inclusions and exclusions
+    // are processed. Test suites are expanded for inclusion test targets in all cases, but for
+    // exclusion test targets only for determining what tests should be run.
     scratch.file(
-        "parent/excluded/BUILD",
-        "test_suite(name = 'test_suite', tests = ['//parent/other:specific_test'])");
-    scratch.file("parent/other/BUILD", "cc_test(name = 'specific_test')");
-    assertThat(parseList("parent/...", "-parent/excluded/...")).containsExactlyElementsIn(
-        labels("//parent/other:specific_test"));
+        "parent/test_suite/BUILD",
+        "test_suite(name = 'test_suite', tests = ['//parent/test:specific_test'])");
+    scratch.file("parent/test/BUILD", "cc_test(name = 'specific_test')");
+    assertThat(parseList("parent/...", "-parent/test_suite/..."))
+        .containsExactlyElementsIn(labels("//parent/test:specific_test"));
+    assertThat(parseList(FILTER_TESTS, "parent/...", "-parent/test_suite/...")).isEmpty();
+    assertThat(parseList("parent/test_suite:test_suite", "-parent/test:specific_test"))
+        .containsExactlyElementsIn(labels("//parent/test_suite:test_suite"));
+    assertThat(
+            parseList(FILTER_TESTS, "parent/test_suite:test_suite", "-parent/test:specific_test"))
+        .isEmpty();
   }
 
   /** Regression test for bug: "blaze test "no targets found" warning now fatal" */