With interleaving now enabled, clean up our tests.

Remove many calls from tests to SkyframeLabelVisitor.sync, which is generally
no longer necessary. In particular, BuildViewTestCase.ensureTarget(s)Visited
was calling it, so removing that, as well as simulateLoadingPhase.

I moved some tests for TransitiveTargetFunction to the corresponding test
class, where they belong. I also made sure that we have test coverage in the
BuildViewTest for some cases that were previously only tested with TTF. I
dropped some tests which no longer seem applicable / for which we have coverage
elsewhere.

Also, as a drive-by change, I fixed some warnings in BuildViewTestCase.

Note that TTF is only used for genquery and in an experimental code path, but
generally superseded by ConfiguredTargetFunction, which has taken over the
responsibility for loading in all other cases (and interleaves loading and
analysis).

Some of the tests are not externally visible yet.

--
PiperOrigin-RevId: 144813237
MOS_MIGRATED_REVID=144813237

6 files changed, 95 insertions, 177 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 63e7484e6d..d7be1ffaf4 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
@@ -167,6 +167,30 @@ public class BuildViewTest extends BuildViewTestBase {
   }
 
   @Test
+  public void testSyntaxErrorInDepPackage() throws Exception {
+    // Check that a loading error in a dependency is properly reported.
+    scratch.file("a/BUILD",
+        "genrule(name='x',",
+        "        srcs = ['file.txt'],",
+        "        outs = ['foo'],",
+        "        cmd = 'echo')",
+        "@");  // syntax error
+
+    scratch.file("b/BUILD",
+        "genrule(name= 'cc',",
+        "        tools = ['//a:x'],",
+        "        outs = ['bar'],",
+        "        cmd = 'echo')");
+
+    reporter.removeHandler(failFastHandler);
+    EventBus eventBus = new EventBus();
+    AnalysisResult result = update(eventBus, defaultFlags().with(Flag.KEEP_GOING), "//b:cc");
+
+    assertContainsEvent("invalid character: '@'");
+    assertThat(result.hasError()).isTrue();
+  }
+
+  @Test
   public void testReportsAnalysisRootCauses() throws Exception {
     scratch.file("private/BUILD",
         "genrule(",
@@ -1284,6 +1308,21 @@ public class BuildViewTest extends BuildViewTestBase {
     assertThat(owners).containsExactly("//x:b", "//x:b");
   }
 
+  @Test
+  public void testErrorMessageForMissingPackageGroup() throws Exception {
+    scratch.file(
+        "apple/BUILD",
+        "py_library(name='apple', visibility=['//non:existent'])");
+    reporter.removeHandler(failFastHandler);
+    try {
+      update("//apple");
+      fail();
+    } catch (ViewCreationFailedException e) {
+      // Expected.
+    }
+    assertDoesNotContainEvent("implicitly depends upon");
+  }
+
   /** Runs the same test with the reduced loading phase. */
   @TestSpec(size = Suite.SMALL_TESTS)
   @RunWith(JUnit4.class)
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 f4c57c2cfd..476f90dd67 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
@@ -304,7 +304,6 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
     optionsPolicyEnforcer.enforce(optionsParser);
 
     BuildOptions buildOptions = ruleClassProvider.createBuildOptions(optionsParser);
-    ensureTargetsVisited(buildOptions.getAllLabels().values());
     skyframeExecutor.invalidateConfigurationCollection();
     return skyframeExecutor.createConfigurations(reporter, configurationFactory, buildOptions,
         ImmutableSet.<String>of(), false);
@@ -441,7 +440,6 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
 
     view.setArtifactRoots(
         ImmutableMap.of(PackageIdentifier.createInMainRepo(""), rootDirectory));
-    simulateLoadingPhase();
   }
 
   protected CachingAnalysisEnvironment getTestAnalysisEnvironment() {
@@ -646,27 +644,10 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
         Iterables.find(getFilesToBuild(target), artifactNamed(outputName)));
   }
 
-  protected void simulateLoadingPhase() {
-    try {
-      ensureTargetsVisited(targetConfig.getAllLabels().values());
-    } catch (Exception e) {
-      throw new RuntimeException(e);
-    }
-  }
-
   protected ActionsTestUtil actionsTestUtil() {
     return new ActionsTestUtil(getActionGraph());
   }
 
-  private Set<Target> getTargets(Iterable<Label> labels) throws InterruptedException,
-      NoSuchTargetException, NoSuchPackageException{
-    Set<Target> targets = Sets.newHashSet();
-    for (Label label : labels) {
-      targets.add(skyframeExecutor.getPackageManager().getTarget(reporter, label));
-    }
-    return targets;
-  }
-
   // Get a MutableActionGraph for testing purposes.
   protected MutableActionGraph getMutableActionGraph() {
     return mutableActionGraph;
@@ -678,50 +659,11 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
   }
 
   /**
-   * Construct the containing package of the specified labels, and all of its transitive
-   * dependencies.  This must be done prior to configuration, as the latter is intolerant of
-   * NoSuchTargetExceptions.
-   */
-  protected boolean ensureTargetsVisited(TransitivePackageLoader visitor,
-      Collection<Label> targets, Collection<Label> labels, boolean keepGoing)
-          throws InterruptedException, NoSuchTargetException, NoSuchPackageException {
-    boolean success = visitor.sync(reporter,
-        getTargets(BlazeTestUtils.convertLabels(targets)),
-        ImmutableSet.copyOf(BlazeTestUtils.convertLabels(labels)),
-        keepGoing,
-        /*parallelThreads=*/4,
-        /*maxDepth=*/Integer.MAX_VALUE);
-    return success;
-  }
-
-  protected boolean ensureTargetsVisited(Collection<Label> labels)
-      throws InterruptedException, NoSuchTargetException, NoSuchPackageException {
-    return ensureTargetsVisited(makeVisitor(), ImmutableSet.<Label>of(), labels,
-        /*keepGoing=*/false);
-  }
-
-  protected boolean ensureTargetsVisited(Label label)
-      throws InterruptedException, NoSuchTargetException, NoSuchPackageException {
-    return ensureTargetsVisited(ImmutableList.of(label));
-  }
-
-  protected boolean ensureTargetsVisited(String... labels)
-      throws InterruptedException, NoSuchTargetException, NoSuchPackageException,
-          LabelSyntaxException {
-    List<Label> actualLabels = new ArrayList<>();
-    for (String label : labels) {
-      actualLabels.add(Label.parseAbsolute(label));
-    }
-    return ensureTargetsVisited(actualLabels);
-  }
-
-  /**
    * Returns the ConfiguredTarget for the specified label, configured for the "build" (aka "target")
    * configuration.
    */
   public ConfiguredTarget getConfiguredTarget(String label)
-      throws NoSuchPackageException, NoSuchTargetException, LabelSyntaxException,
-          InterruptedException {
+      throws LabelSyntaxException {
     return getConfiguredTarget(label, targetConfig);
   }
 
@@ -730,8 +672,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
    * given build configuration.
    */
   protected ConfiguredTarget getConfiguredTarget(String label, BuildConfiguration config)
-      throws NoSuchPackageException, NoSuchTargetException,
-      LabelSyntaxException, InterruptedException {
+      throws LabelSyntaxException {
     return getConfiguredTarget(Label.parseAbsolute(label), config);
   }
 
@@ -745,8 +686,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
    * {@link MemoizingEvaluator#getExistingValueForTesting} call in
    * {@link SkyframeExecutor#getConfiguredTargetForTesting}.  See also b/26382502.
    */
-  protected ConfiguredTarget getConfiguredTarget(Label label, BuildConfiguration config)
-      throws NoSuchPackageException, NoSuchTargetException, InterruptedException {
+  protected ConfiguredTarget getConfiguredTarget(Label label, BuildConfiguration config) {
     return view.getConfiguredTargetForTesting(
         reporter, BlazeTestUtils.convertLabel(label), config);
   }
@@ -756,8 +696,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
    * the "build" (aka "target") configuration.
    */
   protected FileConfiguredTarget getFileConfiguredTarget(String label)
-      throws NoSuchPackageException, NoSuchTargetException,
-      LabelSyntaxException, InterruptedException {
+      throws LabelSyntaxException {
     return (FileConfiguredTarget) getConfiguredTarget(label, targetConfig);
   }
 
@@ -766,8 +705,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
    * the "host" configuration.
    */
   protected ConfiguredTarget getHostConfiguredTarget(String label)
-      throws NoSuchPackageException, NoSuchTargetException,
-      LabelSyntaxException, InterruptedException {
+      throws LabelSyntaxException {
     return getConfiguredTarget(label, getHostConfiguration());
   }
 
@@ -776,8 +714,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
    * the "host" configuration.
    */
   protected FileConfiguredTarget getHostFileConfiguredTarget(String label)
-      throws NoSuchPackageException, NoSuchTargetException,
-      LabelSyntaxException, InterruptedException {
+      throws LabelSyntaxException {
     return (FileConfiguredTarget) getHostConfiguredTarget(label);
   }
 
@@ -839,11 +776,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
                                                      String... lines)
       throws IOException, Exception {
     Target rule = scratchRule(packageName, ruleName, lines);
-    if (ensureTargetsVisited(rule.getLabel())) {
-      return view.getConfiguredTargetForTesting(reporter, rule.getLabel(), config);
-    } else {
-      return null;
-    }
+    return view.getConfiguredTargetForTesting(reporter, rule.getLabel(), config);
   }
 
   /**
@@ -1000,7 +933,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
   }
 
   /**
-   * Gets a derived Artifact for testing in the {@link BuildConfiguration#getBinDirectory()}. This
+   * Gets a derived Artifact for testing in the {@link BuildConfiguration#getBinDirectory}. This
    * method should only be used for tests that do no analysis, and so there is no ConfiguredTarget
    * to own this artifact. If the test runs the analysis phase, {@link
    * #getBinArtifact(String, ArtifactOwner)} or its convenience methods should be
@@ -1014,7 +947,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
 
   /**
    * Gets a derived Artifact for testing in the subdirectory of the {@link
-   * BuildConfiguration#getBinDirectory()} corresponding to the package of {@code owner}. So
+   * BuildConfiguration#getBinDirectory} corresponding to the package of {@code owner}. So
    * to specify a file foo/foo.o owned by target //foo:foo, {@code packageRelativePath} should just
    * be "foo.o".
    */
@@ -1024,7 +957,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
 
   /**
    * Gets a derived Artifact for testing in the subdirectory of the {@link
-   * BuildConfiguration#getBinDirectory()} corresponding to the package of {@code owner}. So
+   * BuildConfiguration#getBinDirectory} corresponding to the package of {@code owner}. So
    * to specify a file foo/foo.o owned by target //foo:foo, {@code packageRelativePath} should just
    * be "foo.o".
    */
@@ -1036,7 +969,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
 
   /**
    * Gets a derived Artifact for testing in the subdirectory of the {@link
-   * BuildConfiguration#getBinDirectory()} corresponding to the package of {@code owner}, where the
+   * BuildConfiguration#getBinDirectory} corresponding to the package of {@code owner}, where the
    * given artifact belongs to the given ConfiguredTarget together with the given Aspect. So to
    * specify a file foo/foo.o owned by target //foo:foo with an aspect from FooAspect, {@code
    * packageRelativePath} should just be "foo.o", and aspectOfOwner should be FooAspect.class. This
@@ -1051,7 +984,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
 
   /**
    * Gets a derived Artifact for testing in the subdirectory of the {@link
-   * BuildConfiguration#getBinDirectory()} corresponding to the package of {@code owner}, where the
+   * BuildConfiguration#getBinDirectory} corresponding to the package of {@code owner}, where the
    * given artifact belongs to the given ConfiguredTarget together with the given Aspect. So to
    * specify a file foo/foo.o owned by target //foo:foo with an aspect from FooAspect, {@code
    * packageRelativePath} should just be "foo.o", and aspectOfOwner should be FooAspect.class. This
@@ -1076,7 +1009,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
 
   /**
    * Gets a derived Artifact for testing in the subdirectory of the {@link
-   * BuildConfiguration#getBinDirectory()} corresponding to the package of {@code owner}. So
+   * BuildConfiguration#getBinDirectory} corresponding to the package of {@code owner}. So
    * to specify a file foo/foo.o owned by target //foo:foo, {@code packageRelativePath} should just
    * be "foo.o".
    */
@@ -1087,7 +1020,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
   }
 
   /**
-   * Gets a derived Artifact for testing in the {@link BuildConfiguration#getGenfilesDirectory()}.
+   * Gets a derived Artifact for testing in the {@link BuildConfiguration#getGenfilesDirectory}.
    * This method should only be used for tests that do no analysis, and so there is no
    * ConfiguredTarget to own this artifact. If the test runs the analysis phase, {@link
    * #getGenfilesArtifact(String, ArtifactOwner)} or its convenience methods should be used instead.
@@ -1100,7 +1033,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
 
   /**
    * Gets a derived Artifact for testing in the subdirectory of the {@link
-   * BuildConfiguration#getGenfilesDirectory()} corresponding to the package of {@code owner}.
+   * BuildConfiguration#getGenfilesDirectory} corresponding to the package of {@code owner}.
    * So to specify a file foo/foo.o owned by target //foo:foo, {@code packageRelativePath} should
    * just be "foo.o".
    */
@@ -1110,7 +1043,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
 
   /**
    * Gets a derived Artifact for testing in the subdirectory of the {@link
-   * BuildConfiguration#getGenfilesDirectory()} corresponding to the package of {@code owner}.
+   * BuildConfiguration#getGenfilesDirectory} corresponding to the package of {@code owner}.
    * So to specify a file foo/foo.o owned by target //foo:foo, {@code packageRelativePath} should
    * just be "foo.o".
    */
@@ -1120,7 +1053,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
 
   /**
    * Gets a derived Artifact for testing in the subdirectory of the {@link
-   * BuildConfiguration#getGenfilesDirectory()} corresponding to the package of {@code owner},
+   * BuildConfiguration#getGenfilesDirectory} corresponding to the package of {@code owner},
    * where the given artifact belongs to the given ConfiguredTarget together with the given Aspect.
    * So to specify a file foo/foo.o owned by target //foo:foo with an apsect from FooAspect,
    * {@code packageRelativePath} should just be "foo.o", and aspectOfOwner should be
@@ -1164,7 +1097,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
 
   /**
    * Gets a derived Artifact for testing in the subdirectory of the {@link
-   * BuildConfiguration#getGenfilesDirectory()} corresponding to the package of {@code owner}.
+   * BuildConfiguration#getGenfilesDirectory} corresponding to the package of {@code owner}.
    * So to specify a file foo/foo.o owned by target //foo:foo, {@code packageRelativePath} should
    * just be "foo.o".
    */
@@ -1176,7 +1109,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
 
   /**
    * Gets a derived Artifact for testing in the subdirectory of the {@link
-   * BuildConfiguration#getIncludeDirectory()} corresponding to the package of {@code owner}.
+   * BuildConfiguration#getIncludeDirectory} corresponding to the package of {@code owner}.
    * So to specify a file foo/foo.o owned by target //foo:foo, {@code packageRelativePath} should
    * just be "foo.h".
    */
@@ -1186,7 +1119,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
 
   /**
    * Gets a derived Artifact for testing in the subdirectory of the {@link
-   * BuildConfiguration#getIncludeDirectory()} corresponding to the package of {@code owner}.
+   * BuildConfiguration#getIncludeDirectory} corresponding to the package of {@code owner}.
    * So to specify a file foo/foo.o owned by target //foo:foo, {@code packageRelativePath} should
    * just be "foo.h".
    */
diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/IOExceptionsTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/IOExceptionsTest.java
index 901e0bfabf..0f546ae21c 100644
--- a/src/test/java/com/google/devtools/build/lib/pkgcache/IOExceptionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/pkgcache/IOExceptionsTest.java
@@ -13,16 +13,14 @@
 // limitations under the License.
 package com.google.devtools.build.lib.pkgcache;
 
-import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
 import com.google.common.base.Function;
-import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.packages.ConstantRuleVisibility;
-import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.packages.util.PackageLoadingTestCase;
+import com.google.devtools.build.lib.skyframe.TransitiveTargetValue;
 import com.google.devtools.build.lib.util.BlazeClock;
 import com.google.devtools.build.lib.vfs.FileStatus;
 import com.google.devtools.build.lib.vfs.FileSystem;
@@ -30,16 +28,16 @@ import com.google.devtools.build.lib.vfs.ModifiedFileSet;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
-
+import com.google.devtools.build.skyframe.EvaluationResult;
+import com.google.devtools.build.skyframe.SkyKey;
+import com.google.devtools.build.skyframe.SkyValue;
+import java.io.IOException;
+import javax.annotation.Nullable;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 
-import java.io.IOException;
-
-import javax.annotation.Nullable;
-
 /**
  * Tests for recovering from IOExceptions thrown by the filesystem when reading BUILD files. Needs
  * its own test class because it uses a custom filesystem.
@@ -67,6 +65,16 @@ public class IOExceptionsTest extends PackageLoadingTestCase {
     this.visitor = skyframeExecutor.pkgLoader();
   }
 
+  private boolean visitTransitively(Label label) throws InterruptedException {
+    SkyKey key = TransitiveTargetValue.key(label);
+    EvaluationResult<SkyValue> result =
+        skyframeExecutor.prepareAndGet(key, /*numThreads=*/5, reporter);
+    TransitiveTargetValue value = (TransitiveTargetValue) result.get(key);
+    System.out.println(value);
+    boolean hasTransitiveError = (value == null) || value.getTransitiveRootCauses() != null;
+    return !result.hasError() && !hasTransitiveError;
+  }
+
   protected void syncPackages() throws Exception {
     skyframeExecutor.invalidateFilesUnderPathForTesting(
         reporter, ModifiedFileSet.EVERYTHING_MODIFIED, rootDirectory);
@@ -100,11 +108,7 @@ public class IOExceptionsTest extends PackageLoadingTestCase {
         return null;
       }
     };
-    assertFalse(visitor.sync(reporter, ImmutableSet.<Target>of(),
-        ImmutableSet.of(Label.parseAbsolute("//pkg:x")), /*keepGoing=*/false,
-        /*parallelThreads=*/5, Integer.MAX_VALUE));
-    assertContainsEvent("no such package 'pkg'");
-    assertEquals(1, eventCollector.count());
+    assertFalse(visitTransitively(Label.parseAbsolute("//pkg:x")));
     scratch.overwriteFile("pkg/BUILD",
         "# another comment to force reload",
         "sh_library(name = 'x')");
@@ -112,9 +116,7 @@ public class IOExceptionsTest extends PackageLoadingTestCase {
     syncPackages();
     eventCollector.clear();
     reporter.addHandler(failFastHandler);
-    assertTrue(visitor.sync(reporter, ImmutableSet.<Target>of(),
-        ImmutableSet.of(Label.parseAbsolute("//pkg:x")), /*keepGoing=*/false,
-        /*parallelThreads=*/5, Integer.MAX_VALUE));
+    assertTrue(visitTransitively(Label.parseAbsolute("//pkg:x")));
     assertNoEvents();
   }
 
@@ -135,9 +137,7 @@ public class IOExceptionsTest extends PackageLoadingTestCase {
         return null;
       }
     };
-    assertFalse(visitor.sync(reporter, ImmutableSet.<Target>of(),
-        ImmutableSet.of(Label.parseAbsolute("//top:top")), /*keepGoing=*/false,
-        /*parallelThreads=*/5, Integer.MAX_VALUE));
+    assertFalse(visitTransitively(Label.parseAbsolute("//top:top")));
     assertContainsEvent("no such package 'pkg'");
     // The traditional label visitor does not propagate the original IOException message.
     // assertContainsEvent("custom crash");
@@ -151,9 +151,7 @@ public class IOExceptionsTest extends PackageLoadingTestCase {
     syncPackages();
     eventCollector.clear();
     reporter.addHandler(failFastHandler);
-    assertTrue(visitor.sync(reporter, ImmutableSet.<Target>of(),
-        ImmutableSet.of(Label.parseAbsolute("//top:top")), /*keepGoing=*/false,
-        /*parallelThreads=*/5, Integer.MAX_VALUE));
+    assertTrue(visitTransitively(Label.parseAbsolute("//top:top")));
     assertNoEvents();
   }
 
@@ -172,10 +170,6 @@ public class IOExceptionsTest extends PackageLoadingTestCase {
         return null;
       }
     };
-    assertFalse(visitor.sync(reporter, ImmutableSet.<Target>of(),
-        ImmutableSet.of(Label.parseAbsolute("//top/pkg:x")), /*keepGoing=*/false,
-        /*parallelThreads=*/5, Integer.MAX_VALUE));
-    assertContainsEvent("no such package 'top/pkg'");
-    assertEquals(1, eventCollector.count());
+    assertFalse(visitTransitively(Label.parseAbsolute("//top/pkg:x")));
   }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/PackageCacheTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/PackageCacheTest.java
index fa22c66ea7..a7a5918c8c 100644
--- a/src/test/java/com/google/devtools/build/lib/pkgcache/PackageCacheTest.java
+++ b/src/test/java/com/google/devtools/build/lib/pkgcache/PackageCacheTest.java
@@ -40,7 +40,6 @@ import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.packages.Preprocessor;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.Target;
-import com.google.devtools.build.lib.packages.util.MockToolsConfig;
 import com.google.devtools.build.lib.skyframe.DiffAwareness;
 import com.google.devtools.build.lib.skyframe.PackageLookupFunction.CrossRepositoryLabelViolationStrategy;
 import com.google.devtools.build.lib.skyframe.PackageLookupValue.BuildFileName;
@@ -59,9 +58,7 @@ import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.common.options.OptionsParser;
 import com.google.devtools.common.options.OptionsParsingException;
 import java.io.IOException;
-import java.util.HashSet;
 import java.util.List;
-import java.util.Set;
 import java.util.UUID;
 import org.junit.Before;
 import org.junit.Test;
@@ -582,41 +579,6 @@ public class PackageCacheTest extends FoundationTestCase {
     assertThat(cc.getFeatures()).hasSize(1);
   }
 
-  /** Visit label and its dependencies and load all of them. */
-  private void visitLabel(String label) throws Exception {
-    TransitivePackageLoader visitor = getPackageManager().newTransitiveLoader();
-    Set<Target> targets = new HashSet<>();
-    targets.add(getPackageManager().getTarget(reporter, Label.parseAbsolute(label)));
-    visitor.sync(reporter, targets, ImmutableSet.<Label>of(),
-        false, 1, Integer.MAX_VALUE);
-  }
-
-  @Test
-  public void testSyntaxErrorInDepPackage() throws Exception {
-    reporter.removeHandler(failFastHandler);
-    AnalysisMock.get().setupMockClient(new MockToolsConfig(rootDirectory));
-
-    scratch.file("a/BUILD",
-        "genrule(name='x',",
-        "        srcs = ['file.txt'],",
-        "        outs = ['foo'],",
-        "        cmd = 'echo')",
-        "@");  // syntax error
-
-    scratch.file("b/BUILD",
-        "genrule(name= 'cc',",
-        "        tools = ['//a:x'],",
-        "        outs = ['bar'],",
-        "        cmd = 'echo')");
-
-    Package pkgB = getPackage("b");
-
-    // We should get error message from package a, but package is properly loaded.
-    visitLabel("//b:cc");
-    assertContainsEvent("invalid character: '@'");
-    assertFalse(pkgB.containsErrors());
-  }
-
   @Test
   public void testBrokenPackageOnMultiplePackagePathEntries() throws Exception {
     reporter.removeHandler(failFastHandler);
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java
index 9d0c8fb6c9..ad36b3c92a 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java
@@ -45,7 +45,6 @@ import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.flags.InvocationPolicyEnforcer;
 import com.google.devtools.build.lib.packages.RuleClass;
-import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.rules.ToolchainLookup;
 import com.google.devtools.build.lib.testutil.MoreAsserts;
 import com.google.devtools.build.lib.util.FileType;
@@ -558,12 +557,9 @@ public class CcCommonTest extends BuildViewTestCase {
         "cc_library(name = 'lib',",
         "           srcs = ['foo.cc'],",
         "           includes = ['./'])");
-    Target target = getTarget("@pkg//bar:lib");
-    ensureTargetsVisited(target.getLabel());
-    assertThat(
-            view.hasErrors(
-                view.getConfiguredTargetForTesting(reporter, target.getLabel(), targetConfig)))
-        .isFalse();
+    Label label = Label.parseAbsolute("@pkg//bar:lib");
+    ConfiguredTarget target = view.getConfiguredTargetForTesting(reporter, label, targetConfig);
+    assertThat(view.hasErrors(target)).isFalse();
     assertNoEvents();
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
index 47905d58a6..fb86314aaa 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
@@ -1245,7 +1245,7 @@ public class SkylarkIntegrationTest extends BuildViewTestCase {
       ((PackageFunction) skyFunctions.get(SkyFunctions.PACKAGE))
           .setSkylarkImportLookupFunctionForInliningForTesting(skylarkImportLookupFunction);
     }
-    
+
     @Override
     @Test
     public void testRecursiveImport() throws Exception {
@@ -1260,19 +1260,15 @@ public class SkylarkIntegrationTest extends BuildViewTestCase {
 
       reporter.removeHandler(failFastHandler);
       try {
-        // ensureTargetsVisited() produces a different event than getTarget, and it doesn't fail
-        // even though there is an error in the rule. What's going on here?
-        ensureTargetsVisited("//test/skylark:rule");
         getTarget("//test/skylark:rule");
         fail();
       } catch (BuildFileContainsErrorsException e) {
-        // This is expected
+        // The reason that this is an exception and not reported to the event handler is that the
+        // error is reported by the parent sky function, which we don't have here.
+        assertThat(e.getMessage()).contains("Skylark import cycle");
+        assertThat(e.getMessage()).contains("test/skylark:ext1.bzl");
+        assertThat(e.getMessage()).contains("test/skylark:ext2.bzl");
       }
-      assertContainsEvent("test/skylark:ext1.bzl");
-      assertContainsEvent("test/skylark:ext2.bzl");
-      assertContainsEvent("Skylark import cycle");
-      assertContainsEvent("Loading of target '//test/skylark:rule' failed; build aborted");
-      assertThat(eventCollector).hasSize(1);
     }
 
     @Override
@@ -1290,18 +1286,16 @@ public class SkylarkIntegrationTest extends BuildViewTestCase {
 
       reporter.removeHandler(failFastHandler);
       try {
-        ensureTargetsVisited("//test/skylark:rule");
         getTarget("//test/skylark:rule");
         fail();
       } catch (BuildFileContainsErrorsException e) {
-        // This is expected
+        // The reason that this is an exception and not reported to the event handler is that the
+        // error is reported by the parent sky function, which we don't have here.
+        assertThat(e.getMessage()).contains("Skylark import cycle");
+        assertThat(e.getMessage()).contains("//test/skylark:ext2.bzl");
+        assertThat(e.getMessage()).contains("//test/skylark:ext3.bzl");
+        assertThat(e.getMessage()).contains("//test/skylark:ext4.bzl");
       }
-      assertContainsEvent("//test/skylark:ext2.bzl");
-      assertContainsEvent("//test/skylark:ext3.bzl");
-      assertContainsEvent("//test/skylark:ext4.bzl");
-      assertContainsEvent("Skylark import cycle");
-      assertContainsEvent("Loading of target '//test/skylark:rule' failed; build aborted");
-      assertThat(eventCollector).hasSize(1);
     }
 
   }