Implement proper error handling for interleaved loading and analysis.

Add test coverage by re-running BuildViewTest with the new Skyframe loading
phase runner.

--
MOS_MIGRATED_REVID=113517509

3 files changed, 46 insertions, 10 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
index 69573f7534..6f3d01ce85 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
@@ -75,7 +75,7 @@ import java.util.regex.Pattern;
  */
 @TestSpec(size = Suite.SMALL_TESTS)
 @RunWith(JUnit4.class)
-public final class BuildViewTest extends BuildViewTestBase {
+public class BuildViewTest extends BuildViewTestBase {
 
   @Test
   public void testRuleConfiguredTarget() throws Exception {
@@ -438,7 +438,7 @@ public final class BuildViewTest extends BuildViewTestBase {
     try {
       update("//foo:top");
       fail();
-    } catch (LoadingFailedException e) {
+    } catch (LoadingFailedException | ViewCreationFailedException e) {
       // Expected.
     }
     assertContainsEvent("no such target '//badbuild:isweird': target 'isweird' not declared in "
@@ -837,13 +837,13 @@ public final class BuildViewTest extends BuildViewTestBase {
         "java_binary(name = 'java', srcs = ['DoesntMatter.java'])",
         "cc_binary(name = 'cpp', data = [':java'])");
     // Everything is fine - the dependency graph is acyclic.
-    update(defaultFlags(), "//foo:java", "//foo:cpp");
+    update("//foo:java", "//foo:cpp");
     // Now there will be an analysis-phase cycle because the java_binary now has an implicit dep on
     // the cc_binary launcher.
     useConfiguration("--java_launcher=//foo:cpp");
     reporter.removeHandler(failFastHandler);
     try {
-      update(defaultFlags(), "//foo:java", "//foo:cpp");
+      update("//foo:java", "//foo:cpp");
       fail();
     } catch (ViewCreationFailedException expected) {
       Truth.assertThat(expected.getMessage())
@@ -852,4 +852,33 @@ public final class BuildViewTest extends BuildViewTestBase {
     assertContainsEvent("cycle in dependency graph");
     assertContainsEvent("This cycle occurred because of a configuration option");
   }
+
+  @Test
+  public void testDependsOnBrokenTarget() throws Exception {
+    scratch.file("foo/BUILD",
+        "sh_test(name = 'test', srcs = ['test.sh'], data = ['//bar:data'])");
+    scratch.file("bar/BUILD",
+        "BROKEN BROKEN BROKEN!!!");
+    reporter.removeHandler(failFastHandler);
+    try {
+      update("//foo:test");
+      fail();
+    } catch (LoadingFailedException expected) {
+      Truth.assertThat(expected.getMessage())
+          .matches("Loading failed; build aborted.*");
+    } catch (ViewCreationFailedException expected) {
+      Truth.assertThat(expected.getMessage())
+          .matches("Analysis of target '//foo:test' failed; build aborted.*");
+    }
+  }
+
+  /** Runs the same test with the reduced loading phase. */
+  @TestSpec(size = Suite.SMALL_TESTS)
+  @RunWith(JUnit4.class)
+  public static class WithSkyframeLoadingPhase extends BuildViewTest {
+    @Override
+    protected FlagBuilder defaultFlags() {
+      return super.defaultFlags().with(Flag.SKYFRAME_LOADING_PHASE);
+    }
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/DependencyResolverTest.java b/src/test/java/com/google/devtools/build/lib/analysis/DependencyResolverTest.java
index e75ca935dd..2179908a9e 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/DependencyResolverTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/DependencyResolverTest.java
@@ -23,9 +23,12 @@ import com.google.devtools.build.lib.analysis.util.AnalysisTestCase;
 import com.google.devtools.build.lib.analysis.util.TestAspects;
 import com.google.devtools.build.lib.analysis.util.TestAspects.AspectRequiringRule;
 import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.packages.Aspect;
 import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.NativeAspectClass;
+import com.google.devtools.build.lib.packages.NoSuchPackageException;
+import com.google.devtools.build.lib.packages.NoSuchTargetException;
 import com.google.devtools.build.lib.packages.NoSuchThingException;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
@@ -71,10 +74,10 @@ public class DependencyResolverTest extends AnalysisTestCase {
 
       @Nullable
       @Override
-      protected Target getTarget(Label label) throws NoSuchThingException {
+      protected Target getTarget(Target from, Label label, NestedSetBuilder<Label> rootCauses) {
         try {
           return packageManager.getTarget(reporter, label);
-        } catch (InterruptedException e) {
+        } catch (NoSuchPackageException | NoSuchTargetException | InterruptedException e) {
           throw new IllegalStateException(e);
         }
       }
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
index 5e01fd3af3..a1381affac 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
@@ -40,7 +40,6 @@ import com.google.devtools.build.lib.packages.PackageFactory;
 import com.google.devtools.build.lib.packages.Preprocessor;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.packages.util.MockToolsConfig;
-import com.google.devtools.build.lib.pkgcache.LegacyLoadingPhaseRunner;
 import com.google.devtools.build.lib.pkgcache.LoadingOptions;
 import com.google.devtools.build.lib.pkgcache.LoadingPhaseRunner;
 import com.google.devtools.build.lib.pkgcache.LoadingResult;
@@ -89,7 +88,8 @@ public abstract class AnalysisTestCase extends FoundationTestCase {
 
   /** All the flags that can be passed to {@link BuildView#update}. */
   public enum Flag {
-    KEEP_GOING
+    KEEP_GOING,
+    SKYFRAME_LOADING_PHASE,
   }
 
   /** Helper class to make it easy to enable and disable flags. */
@@ -105,6 +105,10 @@ public abstract class AnalysisTestCase extends FoundationTestCase {
       flags.remove(flag);
       return this;
     }
+
+    boolean contains(Flag flag) {
+      return flags.contains(flag);
+    }
   }
 
   protected BlazeDirectories directories;
@@ -169,8 +173,8 @@ public abstract class AnalysisTestCase extends FoundationTestCase {
         Options.getDefaults(PackageCacheOptions.class).defaultVisibility, true,
         3, ruleClassProvider.getDefaultsPackageContent(), UUID.randomUUID());
     packageManager = skyframeExecutor.getPackageManager();
-    loadingPhaseRunner =
-        new LegacyLoadingPhaseRunner(packageManager, pkgFactory.getRuleClassNames());
+    loadingPhaseRunner = skyframeExecutor.getLoadingPhaseRunner(
+        pkgFactory.getRuleClassNames(), defaultFlags().contains(Flag.SKYFRAME_LOADING_PHASE));
     buildView = new BuildView(directories, ruleClassProvider, skyframeExecutor, null);
     useConfiguration();
   }