Add cycle reporting for test_suite expansion.

Test suite cycles now produce nice cycle errors like this:

ERROR /workspace/BUILD:1:41: in test_suite rule //:b: cycle in dependency graph:
.-> //:b
|   //:a
`-- //:b

Before, test_suite cycles resulted in a crash.

Fixes #5851.

Closes #6066.

PiperOrigin-RevId: 215373465

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

@@ -40,14 +40,16 @@ abstract class AbstractLabelCycleReporter implements CyclesReporter.SingleCycleR
     this.packageProvider = packageProvider;
   }
 
-  /** Returns the String representation of the {@code SkyKey}. */
-  protected abstract String prettyPrint(SkyKey key);
-
   /** Returns the associated Label of the SkyKey. */
   protected abstract Label getLabel(SkyKey key);
 
   protected abstract boolean canReportCycle(SkyKey topLevelKey, CycleInfo cycleInfo);
 
+  /** Returns the String representation of the {@code SkyKey}. */
+  protected String prettyPrint(SkyKey key) {
+    return getLabel(key).toString();
+  }
+
   /**
    * Can be used to skip individual keys on the path to cycle.
    *
@@ -81,11 +83,18 @@ public boolean maybeReportCycle(
     }
 
     if (alreadyReported) {
-      Label label = getLabel(topLevelKey);
-      Target target = getTargetForLabel(eventHandler, label);
-      eventHandler.handle(Event.error(target.getLocation(),
-          "in " + target.getTargetKind() + " " + label +
-              ": cycle in dependency graph: target depends on an already-reported cycle"));
+      if (!shouldSkip(topLevelKey)) {
+        Label label = getLabel(topLevelKey);
+        Target target = getTargetForLabel(eventHandler, label);
+        eventHandler.handle(
+            Event.error(
+                target.getLocation(),
+                "in "
+                    + target.getTargetKind()
+                    + " "
+                    + label
+                    + ": cycle in dependency graph: target depends on an already-reported cycle"));
+      }
     } else {
       StringBuilder cycleMessage = new StringBuilder("cycle in dependency graph:");
       ImmutableList<SkyKey> pathToCycle = cycleInfo.getPathToCycle();

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

@@ -2268,9 +2268,10 @@ private CyclesReporter createCyclesReporter() {
     return new CyclesReporter(
         new TransitiveTargetCycleReporter(packageManager),
         new ActionArtifactCycleReporter(packageManager),
+        new ConfiguredTargetCycleReporter(packageManager),
+        new TestSuiteCycleReporter(packageManager),
         // TODO(ulfjack): The SkylarkModuleCycleReporter swallows previously reported cycles
         // unconditionally! Is that intentional?
-        new ConfiguredTargetCycleReporter(packageManager),
         new SkylarkModuleCycleReporter());
   }
 

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

@@ -0,0 +1,43 @@
+// Copyright 2018 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.skyframe;
+
+import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.pkgcache.PackageProvider;
+import com.google.devtools.build.lib.skyframe.TestsInSuiteValue.TestsInSuiteKey;
+import com.google.devtools.build.skyframe.CycleInfo;
+import com.google.devtools.build.skyframe.SkyKey;
+
+/** Reports cycles occurring in during the expansion of <code>test_suite</code> rules. */
+class TestSuiteCycleReporter extends AbstractLabelCycleReporter {
+
+  public TestSuiteCycleReporter(PackageProvider packageProvider) {
+    super(packageProvider);
+  }
+
+  @Override
+  protected boolean canReportCycle(SkyKey topLevelKey, CycleInfo cycleInfo) {
+    return cycleInfo.getCycle().stream().allMatch(TestsInSuiteKey.class::isInstance);
+  }
+
+  @Override
+  protected boolean shouldSkip(SkyKey key) {
+    return !(key instanceof TestsInSuiteKey);
+  }
+
+  @Override
+  protected Label getLabel(SkyKey key) {
+    return ((TestsInSuiteKey) key).getTestSuiteLabel();
+  }
+}

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

@@ -48,11 +48,6 @@ protected boolean canReportCycle(SkyKey topLevelKey, CycleInfo cycleInfo) {
         IS_SUPPORTED_SKY_KEY);
   }
 
-  @Override
-  public String prettyPrint(SkyKey key) {
-    return getLabel(key).toString();
-  }
-
   @Override
   protected Label getLabel(SkyKey key) {
     return ((TransitiveTargetKey) key).getLabel();

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

@@ -14,6 +14,7 @@
 package com.google.devtools.build.lib.pkgcache;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows;
 import static org.junit.Assert.fail;
 
 import com.google.common.base.Functions;
@@ -821,6 +822,20 @@ public void mapsOriginalPatternsToLabels() throws Exception {
             "test/...", Label.parseAbsoluteUnchecked("//test/b:b_lib"));
   }
 
+  @Test
+  public void testSuiteCycle() throws Exception {
+    tester.addFile(
+        "BUILD", "test_suite(name = 'a', tests = [':b']); test_suite(name = 'b', tests = [':a'])");
+    assertThat(
+            assertThrows(TargetParsingException.class, () -> tester.loadKeepGoing("//:a", "//:b")))
+        .hasMessageThat()
+        .contains("cycles detected");
+    assertThat(tester.assertContainsError("cycle in dependency graph").toString())
+        .containsMatch("in test_suite rule //:.: cycle in dependency graph");
+    PatternExpandingError err = tester.findPostOnce(PatternExpandingError.class);
+    assertThat(err.getPattern()).containsExactly("//:a", "//:b");
+  }
+
   @Test
   public void mapsOriginalPatternsToLabels_omitsExcludedTargets() throws Exception {
     tester.addFile("test/a/BUILD", "cc_library(name = 'a_lib', srcs = ['a.cc'])");