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

2 files changed, 21 insertions, 65 deletions

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);