Make --experimental_interleave_loading_and_analysis a no-op.

This is now enabled by default, and this change removes the code path where
it's disabled.

Remove a few tests that were testing the removed code, and rewrite some others
that still seem useful. We still drop configured targets on configuration
changes, so we use that to check that things are dropped from Skyframe.

--
MOS_MIGRATED_REVID=132226208

9 files changed, 75 insertions, 168 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java b/src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java
index 857a01f23e..3beebb6c94 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java
@@ -28,14 +28,12 @@ import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.rules.java.JavaSourceJarsProvider;
 import com.google.devtools.build.lib.testutil.Suite;
 import com.google.devtools.build.lib.testutil.TestSpec;
-
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
-
 import java.util.Set;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
 
 /**
  * Analysis caching tests.
@@ -519,20 +517,4 @@ public class AnalysisCachingTest extends AnalysisCachingTestBase {
     assertEquals(1, countObjectsPartiallyMatchingRegex(newAnalyzedTargets, "//java/a:B.java"));
     assertEquals(1, countObjectsPartiallyMatchingRegex(newAnalyzedTargets, "//java/a:y"));
   }
-
-  /**
-   * {link AnalysisCachingTest} without loading phase.
-   */
-  @RunWith(JUnit4.class)
-  public static class AnalysisCachingTestWithoutLoading extends AnalysisCachingTest {
-    @Override
-    protected boolean isLoadingEnabled() {
-      return false;
-    }
-
-    // Error processing without loading phase is not working properly yet.
-    @Override
-    @Test
-    public void testBuildFileInCycleChanged() {}
-  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java b/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java
index 14c4b1fb2f..d29e3324a5 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java
@@ -41,7 +41,6 @@ import com.google.devtools.build.lib.packages.AspectParameters;
 import com.google.devtools.build.lib.packages.NativeAspectClass;
 import com.google.devtools.build.lib.packages.RuleClass;
 import com.google.devtools.build.lib.vfs.ModifiedFileSet;
-
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -385,13 +384,4 @@ public class AspectTest extends AnalysisTestCase {
     assertThat(getOnlyElement(extraActionArtifacts).getLabel()).isEqualTo(Label.create("@//a", "b"));
 
   }
-
-  @RunWith(JUnit4.class)
-  public static class AspectTestWithoutLoading extends AspectTest {
-
-    @Override
-    protected boolean isLoadingEnabled() {
-      return false;
-    }
-  }
 }
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 888f9d4c0f..c5960dd05f 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
@@ -46,7 +46,6 @@ import com.google.devtools.build.lib.analysis.util.ExpectedDynamicConfigurationE
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.Rule;
-import com.google.devtools.build.lib.pkgcache.LoadingFailedException;
 import com.google.devtools.build.lib.skyframe.SkyFunctions;
 import com.google.devtools.build.lib.skyframe.TargetPatternValue.TargetPatternKey;
 import com.google.devtools.build.lib.skyframe.util.SkyframeExecutorTestUtils;
@@ -456,7 +455,7 @@ public class BuildViewTest extends BuildViewTestBase {
     try {
       update("//foo:top");
       fail();
-    } catch (LoadingFailedException | ViewCreationFailedException e) {
+    } catch (ViewCreationFailedException e) {
       // Expected.
     }
     assertContainsEvent("no such target '//badbuild:isweird': target 'isweird' not declared in "
@@ -912,9 +911,6 @@ public class BuildViewTest extends BuildViewTestBase {
     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.*");
@@ -934,17 +930,10 @@ public class BuildViewTest extends BuildViewTestBase {
     try {
       update("//cycle:foo");
       fail();
-    } catch (LoadingFailedException | ViewCreationFailedException expected) {
+    } catch (ViewCreationFailedException expected) {
       assertContainsEvent("in cc_library rule //cycle:foo: cycle in dependency graph:");
-      // In the legacy case, SkyframeLabelVisitor prints the loading error for //cycle:foo. In the
-      // interleaved case, the SkyframeBuildView only throws the exception, but doesn't print the
-      // error - the error is printed by the BuildTool, which isn't used in this test.
-      if (defaultFlags().contains(Flag.SKYFRAME_LOADING_PHASE)) {
-        assertThat(expected.getMessage())
-            .contains("Analysis of target '//cycle:foo' failed; build aborted");
-      } else {
-        assertContainsEvent("Loading of target '//cycle:foo' failed; build aborted");
-      }
+      assertThat(expected.getMessage())
+          .contains("Analysis of target '//cycle:foo' failed; build aborted");
     }
   }
 
@@ -971,31 +960,17 @@ public class BuildViewTest extends BuildViewTestBase {
         "//cycle:foo", "//cycle:bat", "//cycle:baz");
     assertContainsEvent("in cc_library rule //cycle:foo: cycle in dependency graph:");
     assertContainsEvent("in cc_library rule //cycle:bas: cycle in dependency graph:");
-    if (defaultFlags().contains(Flag.SKYFRAME_LOADING_PHASE)) {
-      assertContainsEvent(
-          "errors encountered while analyzing target '//cycle:foo': it will not be built");
-      assertContainsEvent(
-          "errors encountered while analyzing target '//cycle:bat': it will not be built");
-      // With interleaved loading and analysis, we can no longer distinguish loading-phase cycles
-      // and analysis-phase cycles. This was previously reported as a loading-phase cycle, as it
-      // happens with any configuration (cycle is hard-coded in the BUILD files). Also see the
-      // test below.
-      assertThat(Iterables.transform(analysisFailureRecorder.events, ANALYSIS_EVENT_TO_STRING_PAIR))
-          .containsExactly(
-              Pair.of("//cycle:foo", "//cycle:foo"), Pair.of("//cycle:bat", "//cycle:bas"));
-    } else {
-      assertContainsEvent("errors encountered while loading target '//cycle:foo'");
-      assertContainsEvent("errors encountered while loading target '//cycle:bat'");
-
-      assertThat(Iterables.transform(loadingFailureRecorder.events,
-          new Function<Pair<Label, Label>, Pair<String, String>>() {
-            @Override
-            public Pair<String, String> apply(Pair<Label, Label> labelPair) {
-              return Pair.of(labelPair.getFirst().toString(), labelPair.getSecond().toString());
-            }
-          })).containsExactly(
-              Pair.of("//cycle:foo", "//cycle:foo"), Pair.of("//cycle:bat", "//cycle:bas"));
-    }
+    assertContainsEvent(
+        "errors encountered while analyzing target '//cycle:foo': it will not be built");
+    assertContainsEvent(
+        "errors encountered while analyzing target '//cycle:bat': it will not be built");
+    // With interleaved loading and analysis, we can no longer distinguish loading-phase cycles
+    // and analysis-phase cycles. This was previously reported as a loading-phase cycle, as it
+    // happens with any configuration (cycle is hard-coded in the BUILD files). Also see the
+    // test below.
+    assertThat(Iterables.transform(analysisFailureRecorder.events, ANALYSIS_EVENT_TO_STRING_PAIR))
+        .containsExactly(
+            Pair.of("//cycle:foo", "//cycle:foo"), Pair.of("//cycle:bat", "//cycle:bas"));
   }
 
   @Test
@@ -1040,7 +1015,7 @@ public class BuildViewTest extends BuildViewTestBase {
     try {
       update(defaultFlags().with(Flag.KEEP_GOING));
       fail();
-    } catch (LoadingFailedException | InvalidConfigurationException e) {
+    } catch (InvalidConfigurationException e) {
       assertThat(e.getMessage()).contains("third_party/crosstool/v2");
     }
   }
@@ -1053,7 +1028,7 @@ public class BuildViewTest extends BuildViewTestBase {
     try {
       update(defaultFlags().with(Flag.KEEP_GOING));
       fail();
-    } catch (LoadingFailedException | InvalidConfigurationException e) {
+    } catch (InvalidConfigurationException e) {
       assertContainsEvent(
           "no such package 'does/not/exist': BUILD file not found on package path");
     }
@@ -1073,7 +1048,7 @@ public class BuildViewTest extends BuildViewTestBase {
     try {
       update(defaultFlags().with(Flag.KEEP_GOING));
       fail();
-    } catch (LoadingFailedException | InvalidConfigurationException e) {
+    } catch (InvalidConfigurationException e) {
       // Expected
     }
   }
@@ -1089,7 +1064,7 @@ public class BuildViewTest extends BuildViewTestBase {
     try {
       update(defaultFlags().with(Flag.KEEP_GOING));
       fail();
-    } catch (LoadingFailedException | InvalidConfigurationException e) {
+    } catch (InvalidConfigurationException e) {
       assertThat(e.getMessage()).contains("//xcode:does_not_exist");
     }
   }
@@ -1105,7 +1080,7 @@ public class BuildViewTest extends BuildViewTestBase {
     try {
       update("//z/b:b");
       fail();
-    } catch (LoadingFailedException | ViewCreationFailedException expected) {
+    } catch (ViewCreationFailedException expected) {
       assertContainsEvent("no such package 'nonexistent'");
     }
   }
@@ -1122,7 +1097,7 @@ public class BuildViewTest extends BuildViewTestBase {
     try {
       update("//z/b:b");
       fail();
-    } catch (LoadingFailedException | ViewCreationFailedException expected) {
+    } catch (ViewCreationFailedException expected) {
       assertContainsEvent("no such package 'b'");
     }
   }
@@ -1138,7 +1113,7 @@ public class BuildViewTest extends BuildViewTestBase {
     try {
       update("//parent:a");
       fail();
-    } catch (LoadingFailedException | ViewCreationFailedException expected) {
+    } catch (ViewCreationFailedException expected) {
     }
     assertContainsEventWithFrequency("name 'undefined_symbol' is not defined", 1);
     assertContainsEventWithFrequency(
@@ -1175,7 +1150,7 @@ public class BuildViewTest extends BuildViewTestBase {
     try {
       update("//okay");
       fail();
-    } catch (LoadingFailedException | ViewCreationFailedException e) {
+    } catch (ViewCreationFailedException e) {
     }
     assertContainsEventWithFrequency("name 'undefined_symbol' is not defined", 1);
     assertContainsEventWithFrequency(
@@ -1194,11 +1169,7 @@ public class BuildViewTest extends BuildViewTestBase {
         "sh_binary(name = 'okay', srcs = ['okay.sh'])");
     useConfiguration("--experimental_action_listener=//parent:a");
     reporter.removeHandler(failFastHandler);
-    try {
-      update(defaultFlags().with(Flag.KEEP_GOING), "//okay");
-    } catch (LoadingFailedException ignored) {
-      // In the legacy case, we get a loading exception even with keep going. Why?
-    }
+    update(defaultFlags().with(Flag.KEEP_GOING), "//okay");
     assertContainsEventWithFrequency("name 'undefined_symbol' is not defined", 1);
     assertContainsEventWithFrequency(
         "Target '//child:b' contains an error and its package is in error and referenced "
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/DuplicateActionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/DuplicateActionTest.java
index 90830c43da..655b86f674 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/DuplicateActionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/DuplicateActionTest.java
@@ -17,7 +17,6 @@ package com.google.devtools.build.lib.analysis;
 import static org.junit.Assert.assertFalse;
 
 import com.google.devtools.build.lib.analysis.util.AnalysisTestCase;
-
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -41,16 +40,4 @@ public class DuplicateActionTest extends AnalysisTestCase {
     assertFalse(hasErrors(getConfiguredTarget("//a:a")));
     assertFalse(hasErrors(getConfiguredTarget("//a:b")));
   }
-
-  /**
-   *  Same test with loading phase disabled.
-   */
-  @RunWith(JUnit4.class)
-  public static class DuplicateActionTestWithoutLoading extends DuplicateActionTest {
-
-    @Override
-    protected boolean isLoadingEnabled() {
-      return false;
-    }
-  }
 }
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 030fee3edd..e7d0655360 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
@@ -194,11 +194,6 @@ public abstract class AnalysisTestCase extends FoundationTestCase {
     return AnalysisMock.get();
   }
 
-  /** To be overriden by sub classes if they want to disable loading. */
-  protected boolean isLoadingEnabled() {
-    return true;
-  }
-
   protected ImmutableList<PrecomputedValue.Injected> getPrecomputedValues() {
     return ImmutableList.of();
   }
@@ -294,10 +289,17 @@ public abstract class AnalysisTestCase extends FoundationTestCase {
     skyframeExecutor.invalidateFilesUnderPathForTesting(reporter,
         ModifiedFileSet.EVERYTHING_MODIFIED, rootDirectory);
 
-    LoadingResult loadingResult = loadingPhaseRunner
-        .execute(reporter, eventBus, ImmutableList.copyOf(labels), PathFragment.EMPTY_FRAGMENT,
-            loadingOptions, buildOptions.getAllLabels(), viewOptions.keepGoing, isLoadingEnabled(),
-            /*determineTests=*/false, /*callback=*/null);
+    LoadingResult loadingResult =
+        loadingPhaseRunner.execute(
+            reporter,
+            eventBus,
+            ImmutableList.copyOf(labels),
+            PathFragment.EMPTY_FRAGMENT,
+            loadingOptions,
+            buildOptions.getAllLabels(),
+            viewOptions.keepGoing,
+            /*determineTests=*/false,
+            /*callback=*/null);
 
     BuildRequestOptions requestOptions = optionsParser.getOptions(BuildRequestOptions.class);
     ImmutableSortedSet<String> multiCpu = ImmutableSortedSet.copyOf(requestOptions.multiCpus);
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
index 9f4514cd94..f73ed390aa 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
@@ -239,11 +239,6 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
     return null;
   }
 
-  /** To be overriden by sub classes if they want to disable loading. */
-  protected boolean isLoadingEnabled() {
-    return true;
-  }
-
   protected AnalysisMock getAnalysisMock() {
     return AnalysisMock.get();
   }
@@ -1512,9 +1507,17 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
 
     LoadingPhaseRunner runner = new LegacyLoadingPhaseRunner(getPackageManager(),
         Collections.unmodifiableSet(ruleClassProvider.getRuleClassMap().keySet()));
-    LoadingResult loadingResult = runner.execute(reporter, eventBus, targets,
-        PathFragment.EMPTY_FRAGMENT, loadingOptions, getTargetConfiguration().getAllLabels(),
-        viewOptions.keepGoing, isLoadingEnabled(), /*determineTests=*/false, /*callback=*/null);
+    LoadingResult loadingResult =
+        runner.execute(
+            reporter,
+            eventBus,
+            targets,
+            PathFragment.EMPTY_FRAGMENT,
+            loadingOptions,
+            getTargetConfiguration().getAllLabels(),
+            viewOptions.keepGoing,
+            /*determineTests=*/false,
+            /*callback=*/null);
     if (!doAnalysis) {
       // TODO(bazel-team): What's supposed to happen in this case?
       return null;
diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java
index 480078162e..afa781ae84 100644
--- a/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java
@@ -35,7 +35,6 @@ import com.google.devtools.build.lib.analysis.BuildView;
 import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
 import com.google.devtools.build.lib.analysis.util.AnalysisMock;
 import com.google.devtools.build.lib.cmdline.Label;
-import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.cmdline.TargetParsingException;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventKind;
@@ -63,20 +62,17 @@ import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
 import com.google.devtools.common.options.Options;
 import com.google.devtools.common.options.OptionsParser;
 import com.google.devtools.common.options.OptionsParsingException;
-
-import org.junit.Before;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
-
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
-import java.util.Set;
 import java.util.UUID;
 import java.util.logging.Level;
 import java.util.logging.Logger;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
 
 /** Tests for {@link LoadingPhaseRunner}. */
 @RunWith(JUnit4.class)
@@ -95,15 +91,15 @@ public class LoadingPhaseRunnerTest {
 
   @Before
   public final void createLoadingPhaseTester() throws Exception  {
-    tester = new LoadingPhaseTester(!runsLoadingPhase());
+    tester = new LoadingPhaseTester(useSkyframeTargetPatternEval());
   }
 
   protected List<Target> getTargets(String... targetNames) throws Exception {
     return tester.getTargets(targetNames);
   }
 
-  protected boolean runsLoadingPhase() {
-    return true;
+  protected boolean useSkyframeTargetPatternEval() {
+    return false;
   }
 
   @Test
@@ -125,10 +121,6 @@ public class LoadingPhaseRunnerTest {
       public void notifyTargets(Collection<Target> targets) throws LoadingFailedException {
         targetsNotified.addAll(targets);
       }
-
-      @Override
-      public void notifyVisitedPackages(Set<PackageIdentifier> visitedPackages) {
-      }
     });
     assertNoErrors(tester.load("//base:hello"));
     assertThat(targetsNotified).containsExactlyElementsIn(getTargets("//base:hello"));
@@ -170,22 +162,6 @@ public class LoadingPhaseRunnerTest {
   }
 
   @Test
-  public void testBadTransitiveClosure() throws Exception {
-    if (!runsLoadingPhase()) {
-      // TODO(ulfjack): Requires loading phase.
-      return;
-    }
-    tester.addFile("base/BUILD",
-        "filegroup(name = 'hello', srcs = ['//nonexistent:missing'])");
-    LoadingResult loadingResult = tester.loadKeepGoing("//base:hello");
-    assertFalse(loadingResult.hasTargetPatternError());
-    assertTrue(loadingResult.hasLoadingError());
-    assertThat(loadingResult.getTargets()).containsExactlyElementsIn(ImmutableList.<Target>of());
-    assertNull(loadingResult.getTestsToRun());
-    tester.assertContainsError("no such package 'nonexistent': BUILD file not found");
-  }
-
-  @Test
   public void testManualTarget() throws Exception {
     AnalysisMock.get().ccSupport().setup(tester.mockToolsConfig);
     tester.addFile("cc/BUILD", "cc_library(name = 'my_lib', srcs = ['lib.cc'], tags = ['manual'])");
@@ -420,9 +396,6 @@ public class LoadingPhaseRunnerTest {
         "test_suite(name = 'tests', tests = [':my_test'])");
     LoadingResult loadingResult = tester.loadTests("//cc:tests", "-//cc:my_test");
     tester.assertContainsWarning("All specified test targets were excluded by filters");
-    if (runsLoadingPhase()) {
-      assertThat(loadingResult.getTargets()).containsExactlyElementsIn(getTargets("//cc:my_test"));
-    }
     assertThat(loadingResult.getTestsToRun()).containsExactlyElementsIn(getTargets());
   }
 
@@ -527,14 +500,10 @@ public class LoadingPhaseRunnerTest {
         "sh_binary(name = 'bad', srcs = ['bad.sh'])",
         "undefined_symbol");
     LoadingResult loadingResult = tester.loadKeepGoing("//bad");
-    if (runsLoadingPhase()) {
-      // The legacy loading phase runner reports a loading error, but no target pattern error in
-      // keep_going mode, even though it's clearly an error in the referenced target itself, rather
-      // than in its transitive closure. This happens because the target pattern eval swallows such
-      // errors in keep_going mode. We could fix that, but it's a fairly invasive change, and we're
-      // planning to migrate to the Skyframe-based implementation anyway.
+    if (!useSkyframeTargetPatternEval()) {
+      // The LegacyLoadingPhaseRunner drops the error on the floor. The target can be resolved
+      // after all, even if the package is in error.
       assertThat(loadingResult.hasTargetPatternError()).isFalse();
-      assertThat(loadingResult.hasLoadingError()).isTrue();
     } else {
       assertThat(loadingResult.hasTargetPatternError()).isTrue();
     }
@@ -557,9 +526,6 @@ public class LoadingPhaseRunnerTest {
     tester.useLoadingOptions("--compile_one_dependency");
     try {
       LoadingResult loadingResult = tester.load("base/hello.cc");
-      if (runsLoadingPhase()) {
-        fail();
-      }
       assertThat(loadingResult.hasLoadingError()).isFalse();
     } catch (LoadingFailedException expected) {
       tester.assertContainsError("no such package 'bad'");
@@ -572,7 +538,7 @@ public class LoadingPhaseRunnerTest {
         "cc_library(name = 'hello', srcs = ['hello.cc', '//bad:bad.cc'])");
     tester.useLoadingOptions("--compile_one_dependency");
     LoadingResult loadingResult = tester.loadKeepGoing("base/hello.cc");
-    assertEquals(loadingResult.hasLoadingError(), runsLoadingPhase());
+    assertFalse(loadingResult.hasLoadingError());
   }
 
   private void assertCircularSymlinksDuringTargetParsing(String targetPattern) throws Exception {
@@ -695,10 +661,17 @@ public class LoadingPhaseRunnerTest {
         EventBus eventBus = new EventBus();
         FilteredTargetListener listener = new FilteredTargetListener();
         eventBus.register(listener);
-        result = loadingPhaseRunner.execute(storedErrors, eventBus,
-            ImmutableList.copyOf(patterns), PathFragment.EMPTY_FRAGMENT, options,
-            ImmutableListMultimap.<String, Label>of(), keepGoing, /*enableLoading=*/true,
-            determineTests, loadingCallback);
+        result =
+            loadingPhaseRunner.execute(
+                storedErrors,
+                eventBus,
+                ImmutableList.copyOf(patterns),
+                PathFragment.EMPTY_FRAGMENT,
+                options,
+                ImmutableListMultimap.<String, Label>of(),
+                keepGoing,
+                determineTests,
+                loadingCallback);
         this.targetParsingCompleteEvent = listener.targetParsingCompleteEvent;
         this.loadingPhaseCompleteEvent = listener.loadingPhaseCompleteEvent;
       } catch (LoadingFailedException e) {
diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/SkyframeLoadingPhaseRunnerTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/SkyframeLoadingPhaseRunnerTest.java
index 0686a37b4d..a8a79bbfa0 100644
--- a/src/test/java/com/google/devtools/build/lib/pkgcache/SkyframeLoadingPhaseRunnerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/pkgcache/SkyframeLoadingPhaseRunnerTest.java
@@ -20,7 +20,7 @@ import org.junit.runners.JUnit4;
 @RunWith(JUnit4.class)
 public final class SkyframeLoadingPhaseRunnerTest extends LoadingPhaseRunnerTest {
   @Override
-  protected boolean runsLoadingPhase() {
-    return false;
+  protected boolean useSkyframeTargetPatternEval() {
+    return true;
   }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/util/SkyframeExecutorTestUtils.java b/src/test/java/com/google/devtools/build/lib/skyframe/util/SkyframeExecutorTestUtils.java
index 34fb706b09..f0a81f15e6 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/util/SkyframeExecutorTestUtils.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/util/SkyframeExecutorTestUtils.java
@@ -28,7 +28,6 @@ import com.google.devtools.build.lib.skyframe.ConfiguredTargetValue;
 import com.google.devtools.build.lib.skyframe.PackageValue;
 import com.google.devtools.build.lib.skyframe.SkyFunctions;
 import com.google.devtools.build.lib.skyframe.SkyframeExecutor;
-import com.google.devtools.build.lib.skyframe.TargetMarkerValue;
 import com.google.devtools.build.skyframe.ErrorInfo;
 import com.google.devtools.build.skyframe.EvaluationResult;
 import com.google.devtools.build.skyframe.MemoizingEvaluator;
@@ -166,9 +165,9 @@ public class SkyframeExecutorTestUtils {
    * <p>This helper is provided so legacy tests don't need to know about details of skyframe keys.
    */
   @Nullable
-  public static ErrorInfo getExistingFailedTarget(SkyframeExecutor skyframeExecutor, Label label)
+  public static ErrorInfo getExistingFailedPackage(SkyframeExecutor skyframeExecutor, Label label)
       throws InterruptedException {
-    SkyKey key = TargetMarkerValue.key(label);
+    SkyKey key = PackageValue.key(label.getPackageIdentifier());
     return getExistingError(skyframeExecutor, key);
   }
 }