Rewrite SkylarkAspectsTest to inherit AnalysisTestCase.

While BuildViewTestCase now has an update() function, it also has a lot of
legacy cruft that doesn't match how the code is used in production. This also
makes the test code a bit simpler.

Also add a couple missing fail() statements.

This is in preparation for adding a couple more tests; we don't have good
test coverage for interaction between Skylark aspects and native rules and
vice versa, especially for error scenarios.

I may reuse some of the AspectTest code, but that inherits AnalysisTestCase.
Inheriting AnalysisTestCase here also makes that easier.

--
MOS_MIGRATED_REVID=112460142

3 files changed, 36 insertions, 74 deletions

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 d46920894f..5e01fd3af3 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
@@ -226,7 +226,9 @@ public abstract class AnalysisTestCase extends FoundationTestCase {
   /**
    * Update the BuildView: syncs the package cache; loads and analyzes the given labels.
    */
-  protected void update(EventBus eventBus, FlagBuilder config, String... labels) throws Exception {
+  protected AnalysisResult update(
+      EventBus eventBus, FlagBuilder config, ImmutableList<String> aspects, String... labels)
+          throws Exception {
     Set<Flag> flags = config.flags;
 
     LoadingOptions loadingOptions = Options.getDefaults(LoadingOptions.class);
@@ -259,23 +261,34 @@ public abstract class AnalysisTestCase extends FoundationTestCase {
         buildView.update(
             loadingResult,
             masterConfig,
-            ImmutableList.<String>of(),
+            aspects,
             viewOptions,
             AnalysisTestUtil.TOP_LEVEL_ARTIFACT_CONTEXT,
             reporter,
             eventBus,
             isLoadingEnabled());
+    return analysisResult;
+  }
+
+  protected AnalysisResult update(EventBus eventBus, FlagBuilder config, String... labels)
+      throws Exception {
+    return update(eventBus, config, /*aspects=*/ImmutableList.<String>of(), labels);
   }
 
-  protected void update(FlagBuilder config, String... labels) throws Exception {
-    update(new EventBus(), config, labels);
+  protected AnalysisResult update(FlagBuilder config, String... labels) throws Exception {
+    return update(new EventBus(), config, /*aspects=*/ImmutableList.<String>of(), labels);
   }
 
   /**
    * Update the BuildView: syncs the package cache; loads and analyzes the given labels.
    */
-  protected void update(String... labels) throws Exception {
-    update(new EventBus(), defaultFlags(), labels);
+  protected AnalysisResult update(String... labels) throws Exception {
+    return update(new EventBus(), defaultFlags(), /*aspects=*/ImmutableList.<String>of(), labels);
+  }
+
+  protected AnalysisResult update(ImmutableList<String> aspects, String... labels)
+      throws Exception {
+    return update(new EventBus(), defaultFlags(), aspects, labels);
   }
 
   protected Target getTarget(String label) {
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/TestAspects.java b/src/test/java/com/google/devtools/build/lib/analysis/util/TestAspects.java
index 015e94141c..24929f1793 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/TestAspects.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/TestAspects.java
@@ -249,6 +249,7 @@ public class TestAspects {
       return builder.build();
     }
 
+    @Override
     public ConfiguredAspect create(
         ConfiguredTarget base, RuleContext ruleContext, AspectParameters parameters) {
       StringBuilder information = new StringBuilder("aspect " + ruleContext.getLabel());
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java
index b629094f40..2b97b11fc9 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java
@@ -15,18 +15,18 @@ package com.google.devtools.build.lib.skylark;
 
 import static com.google.common.collect.Iterables.transform;
 import static com.google.common.truth.Truth.assertThat;
+import static org.junit.Assert.fail;
 
 import com.google.common.base.Function;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
-import com.google.common.eventbus.EventBus;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.BuildView.AnalysisResult;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.OutputGroupProvider;
 import com.google.devtools.build.lib.analysis.SkylarkProviders;
 import com.google.devtools.build.lib.analysis.ViewCreationFailedException;
-import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
+import com.google.devtools.build.lib.analysis.util.AnalysisTestCase;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.packages.AspectDefinition;
@@ -47,7 +47,7 @@ import javax.annotation.Nullable;
  * Tests for Skylark aspects
  */
 @RunWith(JUnit4.class)
-public class SkylarkAspectsTest extends BuildViewTestCase {
+public class SkylarkAspectsTest extends AnalysisTestCase {
   @Test
   public void testAspect() throws Exception {
     scratch.file(
@@ -59,13 +59,7 @@ public class SkylarkAspectsTest extends BuildViewTestCase {
     scratch.file("test/BUILD", "java_library(name = 'xxx',)");
 
     AnalysisResult analysisResult =
-        update(
-            ImmutableList.of("//test:xxx"),
-            ImmutableList.of("test/aspect.bzl%MyAspect"),
-            false,
-            LOADING_PHASE_THREADS,
-            true,
-            new EventBus());
+        update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:xxx");
     assertThat(
             transform(
                 analysisResult.getTargetsToBuild(),
@@ -104,13 +98,7 @@ public class SkylarkAspectsTest extends BuildViewTestCase {
     scratch.file("test/BUILD", "java_library(name = 'xxx',)");
 
     AnalysisResult analysisResult =
-        update(
-            ImmutableList.of("//test:xxx"),
-            ImmutableList.of("test/aspect.bzl%MyAspect"),
-            false,
-            LOADING_PHASE_THREADS,
-            true,
-            new EventBus());
+        update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:xxx");
     AspectValue aspectValue = Iterables.getOnlyElement(analysisResult.getAspects());
     AspectKey aspectKey = aspectValue.getKey();
     AspectDefinition aspectDefinition = aspectKey.getAspect().getDefinition();
@@ -160,13 +148,7 @@ public class SkylarkAspectsTest extends BuildViewTestCase {
         ")");
 
     AnalysisResult analysisResult =
-        update(
-            ImmutableList.of("//test:xxx"),
-            ImmutableList.of("test/aspect.bzl%MyAspect"),
-            false,
-            LOADING_PHASE_THREADS,
-            true,
-            new EventBus());
+        update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:xxx");
     assertThat(
             transform(
                 analysisResult.getTargetsToBuild(),
@@ -222,13 +204,7 @@ public class SkylarkAspectsTest extends BuildViewTestCase {
         ")");
 
     AnalysisResult analysisResult =
-        update(
-            ImmutableList.of("//test:xxx"),
-            ImmutableList.of("test/aspect.bzl%MyAspect"),
-            false,
-            LOADING_PHASE_THREADS,
-            true,
-            new EventBus());
+        update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:xxx");
     assertThat(
         transform(
             analysisResult.getTargetsToBuild(),
@@ -252,7 +228,6 @@ public class SkylarkAspectsTest extends BuildViewTestCase {
     assertThat(names).containsExactlyElementsIn(expectedSet);
   }
 
-
   @Test
   public void testAspectsFromSkylarkRules() throws Exception {
     scratch.file(
@@ -290,14 +265,7 @@ public class SkylarkAspectsTest extends BuildViewTestCase {
         "     attr = [':yyy'],",
         ")");
 
-    AnalysisResult analysisResult =
-        update(
-            ImmutableList.of("//test:xxx"),
-            ImmutableList.<String>of(),
-            false,
-            LOADING_PHASE_THREADS,
-            true,
-            new EventBus());
+    AnalysisResult analysisResult = update("//test:xxx");
     assertThat(
         transform(
             analysisResult.getTargetsToBuild(),
@@ -340,13 +308,8 @@ public class SkylarkAspectsTest extends BuildViewTestCase {
 
     reporter.removeHandler(failFastHandler);
     try {
-      update(
-          ImmutableList.of("//test:xxx"),
-          ImmutableList.of("test/aspect.bzl%MyAspect"),
-          false,
-          LOADING_PHASE_THREADS,
-          true,
-          new EventBus());
+      update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:xxx");
+      fail();
     } catch (ViewCreationFailedException e) {
       // expect to fail.
     }
@@ -372,13 +335,8 @@ public class SkylarkAspectsTest extends BuildViewTestCase {
 
     reporter.removeHandler(failFastHandler);
     try {
-      update(
-          ImmutableList.of("//test:xxx"),
-          ImmutableList.of("test/aspect.bzl%MyAspect"),
-          false,
-          LOADING_PHASE_THREADS,
-          true,
-          new EventBus());
+      update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:xxx");
+      fail();
     } catch (ViewCreationFailedException e) {
       // expect to fail.
     }
@@ -399,13 +357,8 @@ public class SkylarkAspectsTest extends BuildViewTestCase {
 
     reporter.removeHandler(failFastHandler);
     try {
-      update(
-          ImmutableList.of("//test:xxx"),
-          ImmutableList.of("test/aspect.bzl%MyAspect"),
-          false,
-          LOADING_PHASE_THREADS,
-          true,
-          new EventBus());
+      update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:xxx");
+      fail();
     } catch (ViewCreationFailedException e) {
       // expect to fail.
     }
@@ -429,13 +382,8 @@ public class SkylarkAspectsTest extends BuildViewTestCase {
 
     reporter.removeHandler(failFastHandler);
     try {
-      update(
-          ImmutableList.of("//test:xxx"),
-          ImmutableList.of("test/aspect.bzl%MyAspect"),
-          false,
-          LOADING_PHASE_THREADS,
-          true,
-          new EventBus());
+      update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:xxx");
+      fail();
     } catch (ViewCreationFailedException e) {
       // expect to fail.
     }