Prefactoring for making "blaze test <alias target>" work. Mainly renaming classes and variables and shuffling things around so that the actual functional change is as small as possible. RELNOTES: None. PiperOrigin-RevId: 281043194

diff --git a/src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java b/src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java
index e22434d..156d08e 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java

@@ -82,6 +82,16 @@
     return target instanceof Rule && isTestSuiteRuleName(((Rule) target).getRuleClass());
   }
 
+  /** Returns true iff {@code target} is an {@code alias} rule. */
+  public static boolean isAlias(Target target) {
+    if (!(target instanceof Rule)) {
+      return false;
+    }
+
+    Rule rule = (Rule) target;
+    return !rule.getRuleClassObject().isSkylark() && rule.getRuleClass().equals("alias");
+  }
+
   /**
    * Returns true iff {@code target} is a {@code *_test} or {@code test_suite}.
    */

diff --git a/src/main/java/com/google/devtools/build/lib/packages/TestTargetUtils.java b/src/main/java/com/google/devtools/build/lib/packages/TestTargetUtils.java
index 282ebee..245177a 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/TestTargetUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/TestTargetUtils.java

@@ -13,21 +13,11 @@
 // limitations under the License.
 package com.google.devtools.build.lib.packages;
 
-import com.google.common.collect.ImmutableList;
-import com.google.devtools.build.lib.cmdline.Label;
-import com.google.devtools.build.lib.cmdline.ResolvedTargets;
-import com.google.devtools.build.lib.cmdline.TargetParsingException;
-import com.google.devtools.build.lib.events.Event;
-import com.google.devtools.build.lib.events.ExtendedEventHandler;
-import com.google.devtools.build.lib.pkgcache.TargetProvider;
 import com.google.devtools.build.lib.util.Pair;
-import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
-import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
-import java.util.Map;
 import java.util.Set;
 
 /**
@@ -148,161 +138,4 @@
     }
     return Pair.of(requiredTags, excludedTags);
   }
-
-  /**
-   * Returns the (new, mutable) set of test rules, expanding all 'test_suite' rules into the
-   * individual tests they group together and preserving other test target instances.
-   *
-   * <p>Method assumes that passed collection contains only *_test and test_suite rules. While, at
-   * this point it will successfully preserve non-test rules as well, there is no guarantee that
-   * this behavior will be kept in the future.
-   *
-   * @param targetProvider a target provider
-   * @param eventHandler a failure eventHandler to report loading failures to
-   * @param targets Collection of the *_test and test_suite configured targets
-   * @return a duplicate-free iterable of the tests under the specified targets
-   */
-  public static ResolvedTargets<Target> expandTestSuites(
-      TargetProvider targetProvider,
-      ExtendedEventHandler eventHandler,
-      Iterable<? extends Target> targets,
-      boolean strict,
-      boolean keepGoing)
-      throws TargetParsingException {
-    Closure closure = new Closure(targetProvider, eventHandler, strict, keepGoing);
-    ResolvedTargets.Builder<Target> result = ResolvedTargets.builder();
-    for (Target target : targets) {
-      if (TargetUtils.isTestRule(target)) {
-        result.add(target);
-      } else if (TargetUtils.isTestSuiteRule(target)) {
-        result.addAll(closure.getTestsInSuite((Rule) target));
-      } else {
-        result.add(target);
-      }
-    }
-    if (closure.hasError) {
-      result.setError();
-    }
-    return result.build();
-  }
-
-  // TODO(bazel-team): This is a copy of TestsExpression.Closure with some minor changes; this
-  // should be unified.
-  private static final class Closure {
-    private final TargetProvider targetProvider;
-
-    private final ExtendedEventHandler eventHandler;
-
-    private final boolean keepGoing;
-
-    private final boolean strict;
-
-    private final Map<Target, Set<Target>> testsInSuite = new HashMap<>();
-
-    private boolean hasError;
-
-    public Closure(
-        TargetProvider targetProvider,
-        ExtendedEventHandler eventHandler,
-        boolean strict,
-        boolean keepGoing) {
-      this.targetProvider = targetProvider;
-      this.eventHandler = eventHandler;
-      this.strict = strict;
-      this.keepGoing = keepGoing;
-    }
-
-    /**
-     * Computes and returns the set of test rules in a particular suite.  Uses
-     * dynamic programming---a memoized version of {@link #computeTestsInSuite}.
-     */
-    private Set<Target> getTestsInSuite(Rule testSuite) throws TargetParsingException {
-      Set<Target> tests = testsInSuite.get(testSuite);
-      if (tests == null) {
-        tests = new HashSet<>();
-        testsInSuite.put(testSuite, tests); // break cycles by inserting empty set early.
-        computeTestsInSuite(testSuite, tests);
-      }
-      return tests;
-    }
-
-    /**
-     * Populates 'result' with all the tests associated with the specified
-     * 'testSuite'.  Throws an exception if any target is missing.
-     *
-     * CAUTION!  Keep this logic consistent with {@code TestSuite} and {@code TestsInSuiteFunction}!
-     */
-    private void computeTestsInSuite(Rule testSuite, Set<Target> result)
-        throws TargetParsingException {
-      List<Target> testsAndSuites = new ArrayList<>();
-      // Note that testsAndSuites can contain input file targets; the test_suite rule does not
-      // restrict the set of targets that can appear in tests or suites.
-      testsAndSuites.addAll(getPrerequisites(testSuite, "tests"));
-
-      // 1. Add all tests
-      for (Target test : testsAndSuites) {
-        if (TargetUtils.isTestRule(test)) {
-          result.add(test);
-        } else if (strict && !TargetUtils.isTestSuiteRule(test)) {
-          // If strict mode is enabled, then give an error for any non-test, non-test-suite targets.
-          eventHandler.handle(Event.error(testSuite.getLocation(),
-              "in test_suite rule '" + testSuite.getLabel()
-              + "': expecting a test or a test_suite rule but '" + test.getLabel()
-              + "' is not one."));
-          hasError = true;
-          if (!keepGoing) {
-            throw new TargetParsingException("Test suite expansion failed.");
-          }
-        }
-      }
-
-      // 2. Add implicit dependencies on tests in same package, if any.
-      for (Target target : getPrerequisites(testSuite, "$implicit_tests")) {
-        // The Package construction of $implicit_tests ensures that this check never fails, but we
-        // add it here anyway for compatibility with future code.
-        if (TargetUtils.isTestRule(target)) {
-          result.add(target);
-        }
-      }
-
-      // 3. Filter based on tags, size, env.
-      filterTests(testSuite, result);
-
-      // 4. Expand all suites recursively.
-      for (Target suite : testsAndSuites) {
-        if (TargetUtils.isTestSuiteRule(suite)) {
-          result.addAll(getTestsInSuite((Rule) suite));
-        }
-      }
-    }
-
-    /**
-     * Returns the set of rules named by the attribute 'attrName' of test_suite rule 'testSuite'.
-     * The attribute must be a list of labels. If a target cannot be resolved, then an error is
-     * reported to the environment (which may throw an exception if {@code keep_going} is disabled).
-     */
-    private Collection<Target> getPrerequisites(Rule testSuite, String attrName)
-        throws TargetParsingException {
-      try {
-        List<Target> targets = new ArrayList<>();
-        // TODO(bazel-team): This serializes package loading in some cases. We might want to make
-        // this multi-threaded.
-        for (Label label :
-            NonconfigurableAttributeMapper.of(testSuite).get(attrName, BuildType.LABEL_LIST)) {
-          targets.add(targetProvider.getTarget(eventHandler, label));
-        }
-        return targets;
-      } catch (NoSuchThingException e) {
-        if (keepGoing) {
-          hasError = true;
-          eventHandler.handle(Event.error(e.getMessage()));
-          return ImmutableList.of();
-        }
-        throw new TargetParsingException(e.getMessage(), e);
-      } catch (InterruptedException e) {
-        Thread.currentThread().interrupt();
-        throw new TargetParsingException("interrupted", e);
-      }
-    }
-  }
 }

diff --git a/src/main/java/com/google/devtools/build/lib/query2/ConfiguredTargetValueAccessor.java b/src/main/java/com/google/devtools/build/lib/query2/ConfiguredTargetValueAccessor.java
index 9169f26..c44b78a0 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/ConfiguredTargetValueAccessor.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/ConfiguredTargetValueAccessor.java

@@ -39,7 +39,7 @@
 /**
  * A {@link TargetAccessor} for {@link ConfiguredTargetValue} objects.
  *
- * <p>Incomplete; we'll implement getLabelListAttr and getVisibility when needed.
+ * <p>Incomplete; we'll implement getPrerequisites and getVisibility when needed.
  */
 public class ConfiguredTargetValueAccessor implements TargetAccessor<ConfiguredTargetValue> {
 
@@ -94,7 +94,7 @@
   }
 
   @Override
-  public List<ConfiguredTargetValue> getLabelListAttr(
+  public List<ConfiguredTargetValue> getPrerequisites(
       QueryExpression caller,
       ConfiguredTargetValue configuredTargetValue,
       String attrName,

diff --git a/src/main/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetAccessor.java b/src/main/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetAccessor.java
index c317c5c..b6973fb 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetAccessor.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetAccessor.java

@@ -99,7 +99,7 @@
   }
 
   @Override
-  public List<ConfiguredTarget> getLabelListAttr(
+  public List<ConfiguredTarget> getPrerequisites(
       QueryExpression caller,
       ConfiguredTarget configuredTarget,
       String attrName,

diff --git a/src/main/java/com/google/devtools/build/lib/query2/engine/LabelsFunction.java b/src/main/java/com/google/devtools/build/lib/query2/engine/LabelsFunction.java
index 161fe5b..ef52541 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/engine/LabelsFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/engine/LabelsFunction.java

@@ -61,23 +61,35 @@
       final Callback<T> callback) {
     final String attrName = args.get(0).getWord();
     final Uniquifier<T> uniquifier = env.createUniquifier();
-    return env.eval(args.get(1).getExpression(), context, new Callback<T>() {
-      @Override
-      public void process(Iterable<T> partialResult) throws QueryException, InterruptedException {
-        for (T input : partialResult) {
-          if (env.getAccessor().isRule(input)) {
-            List<T> targets = uniquifier.unique(
-                env.getAccessor().getLabelListAttr(expression, input, attrName,
-                    "in '" + attrName + "' of rule " + env.getAccessor().getLabel(input) + ": "));
-            List<T> result = new ArrayList<>(targets.size());
-            for (T target : targets) {
-              result.add(env.getOrCreate(target));
+    return env.eval(
+        args.get(1).getExpression(),
+        context,
+        new Callback<T>() {
+          @Override
+          public void process(Iterable<T> partialResult)
+              throws QueryException, InterruptedException {
+            for (T input : partialResult) {
+              if (env.getAccessor().isRule(input)) {
+                List<T> targets =
+                    uniquifier.unique(
+                        env.getAccessor()
+                            .getPrerequisites(
+                                expression,
+                                input,
+                                attrName,
+                                "in '"
+                                    + attrName
+                                    + "' of rule "
+                                    + env.getAccessor().getLabel(input)
+                                    + ": "));
+                List<T> result = new ArrayList<>(targets.size());
+                for (T target : targets) {
+                  result.add(env.getOrCreate(target));
+                }
+                callback.process(result);
+              }
             }
-            callback.process(result);
           }
-        }
-
-      }
-    });
+        });
   }
 }

diff --git a/src/main/java/com/google/devtools/build/lib/query2/engine/QueryEnvironment.java b/src/main/java/com/google/devtools/build/lib/query2/engine/QueryEnvironment.java
index da1ae61..b3e0dbe 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/engine/QueryEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/engine/QueryEnvironment.java

@@ -567,7 +567,7 @@
      *
      * @throws IllegalArgumentException if target is not a rule (according to {@link #isRule})
      */
-    Iterable<T> getLabelListAttr(
+    Iterable<T> getPrerequisites(
         QueryExpression caller, T target, String attrName, String errorMsgPrefix)
         throws QueryException, InterruptedException;
 

diff --git a/src/main/java/com/google/devtools/build/lib/query2/engine/TestsFunction.java b/src/main/java/com/google/devtools/build/lib/query2/engine/TestsFunction.java
index 7e7cfdc..1bed494 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/engine/TestsFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/engine/TestsFunction.java

@@ -251,7 +251,7 @@
      */
     private Iterable<T> getPrerequisites(T testSuite, String attrName)
         throws QueryException, InterruptedException {
-      return accessor.getLabelListAttr(
+      return accessor.getPrerequisites(
           expression,
           testSuite,
           attrName,

diff --git a/src/main/java/com/google/devtools/build/lib/query2/query/BlazeTargetAccessor.java b/src/main/java/com/google/devtools/build/lib/query2/query/BlazeTargetAccessor.java
index 10b9922..332076c 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/query/BlazeTargetAccessor.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/query/BlazeTargetAccessor.java

@@ -64,7 +64,7 @@
   }
 
   @Override
-  public Iterable<Target> getLabelListAttr(
+  public Iterable<Target> getPrerequisites(
       QueryExpression caller, Target target, String attrName, String errorMsgPrefix)
       throws QueryException, InterruptedException {
     Preconditions.checkArgument(target instanceof Rule);

diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/TestSuite.java b/src/main/java/com/google/devtools/build/lib/rules/test/TestSuite.java
index c9c3da5..71e50a5 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/test/TestSuite.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/test/TestSuite.java

@@ -47,11 +47,11 @@
     }
 
     //
-    //  CAUTION!  Keep this logic consistent with lib.query2.TestsExpression!
+    //  CAUTION!  Keep this logic consistent with lib.query2.TestsFunction!
     //
 
-    List<String> tagsAttribute = new ArrayList<>(
-        ruleContext.attributes().get("tags", Type.STRING_LIST));
+    List<String> tagsAttribute =
+        new ArrayList<>(ruleContext.attributes().get("tags", Type.STRING_LIST));
     // TODO(ulfjack): This is inconsistent with the other places that do test_suite expansion.
     tagsAttribute.remove("manual");
     Pair<Collection<String>, Collection<String>> requiredExcluded =

diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java
index 12b400b..3a89854 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java

@@ -754,12 +754,17 @@
   }
 
   /**
-   * Return true iff {@code target} is a rule that has an executable file. This includes
-   * *_test rules, *_binary rules, generated outputs, and inputs.
+   * Return true iff it is possible that {@code target} is a rule that has an executable file. This
+   * *_test rules, *_binary rules, aliases, generated outputs, and inputs.
+   *
+   * <p>Determining definitively whether a rule produces an executable can only be done after
+   * analysis; this is only an early sanity check to quickly catch most mistakes.
    */
   private static boolean isExecutable(Target target) {
-    return isPlainFile(target) || isExecutableNonTestRule(target) || TargetUtils.isTestRule(target)
-        || isAliasRule(target);
+    return isPlainFile(target)
+        || isExecutableNonTestRule(target)
+        || TargetUtils.isTestRule(target)
+        || TargetUtils.isAlias(target);
   }
 
   /**
@@ -781,15 +786,6 @@
     return (target instanceof OutputFile) || (target instanceof InputFile);
   }
 
-  private static boolean isAliasRule(Target target) {
-    if (!(target instanceof Rule)) {
-      return false;
-    }
-
-    Rule rule = (Rule) target;
-    return rule.getRuleClass().equals("alias") || rule.getRuleClass().equals("bind");
-  }
-
   private String makeErrorMessageForNotHavingASingleTarget(
       String targetPatternString, Iterable<String> expandedTargetNames) {
     final int maxNumExpandedTargetsToIncludeInErrorMessage = 5;

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareAnalysisPhaseFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareAnalysisPhaseFunction.java
index 58390b0..d72f9cc 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareAnalysisPhaseFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareAnalysisPhaseFunction.java

@@ -142,7 +142,7 @@
     // We get the list of labels from the TargetPatternPhaseValue, so we are reasonably certain that
     // there will not be an error loading these again.
     ResolvedTargets<Target> resolvedTargets =
-        TestSuiteExpansionFunction.labelsToTargets(env, options.getLabels(), false);
+        TestsForTargetPatternFunction.labelsToTargets(env, options.getLabels(), false);
     if (resolvedTargets == null) {
       return null;
     }

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index 71ccc8c..b3f3d1d 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java

@@ -509,8 +509,8 @@
         SkyFunctions.BLACKLISTED_PACKAGE_PREFIXES,
         new BlacklistedPackagePrefixesFunction(
             hardcodedBlacklistedPackagePrefixes, additionalBlacklistedPackagePrefixesFile));
-    map.put(SkyFunctions.TESTS_IN_SUITE, new TestsInSuiteFunction());
-    map.put(SkyFunctions.TEST_SUITE_EXPANSION, new TestSuiteExpansionFunction());
+    map.put(SkyFunctions.TESTS_IN_SUITE, new TestExpansionFunction());
+    map.put(SkyFunctions.TEST_SUITE_EXPANSION, new TestsForTargetPatternFunction());
     map.put(SkyFunctions.TARGET_PATTERN_PHASE, new TargetPatternPhaseFunction());
     map.put(
         SkyFunctions.PREPARE_ANALYSIS_PHASE,
@@ -2760,7 +2760,7 @@
         new TransitiveTargetCycleReporter(packageManager),
         new ActionArtifactCycleReporter(packageManager),
         new ConfiguredTargetCycleReporter(packageManager),
-        new TestSuiteCycleReporter(packageManager),
+        new TestExpansionCycleReporter(packageManager),
         new RegisteredToolchainsCycleReporter(),
         // TODO(ulfjack): The SkylarkModuleCycleReporter swallows previously reported cycles
         // unconditionally! Is that intentional?

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java
index a3d4a79..1d4e59a 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java

@@ -125,7 +125,7 @@
       for (Target target : targets.getTargets()) {
         if (TargetUtils.isTestSuiteRule(target) && options.isExpandTestSuites()) {
           Label label = target.getLabel();
-          SkyKey testExpansionKey = TestSuiteExpansionValue.key(ImmutableSet.of(label));
+          SkyKey testExpansionKey = TestsForTargetPatternValue.key(ImmutableSet.of(label));
           testExpansionKeys.put(label, testExpansionKey);
         }
       }
@@ -199,8 +199,8 @@
       if (TargetUtils.isTestSuiteRule(target) && options.isExpandTestSuites()) {
         SkyKey expansionKey =
             Preconditions.checkNotNull(testExpansionKeys.get(target.getLabel()));
-        TestSuiteExpansionValue testExpansion =
-            (TestSuiteExpansionValue) expandedTests.get(expansionKey);
+        TestsForTargetPatternValue testExpansion =
+            (TestsForTargetPatternValue) expandedTests.get(expansionKey);
         expandedLabelsBuilder.merge(testExpansion.getLabels());
       } else {
         expandedLabelsBuilder.add(target.getLabel());
@@ -208,7 +208,7 @@
     }
     ResolvedTargets<Label> targetLabels = expandedLabelsBuilder.build();
     ResolvedTargets<Target> expandedTargets =
-        TestSuiteExpansionFunction.labelsToTargets(
+        TestsForTargetPatternFunction.labelsToTargets(
             env, targetLabels.getTargets(), targetLabels.hasError());
     Set<Target> testSuiteTargets =
         Sets.difference(targets.getTargets(), expandedTargets.getTargets());
@@ -335,8 +335,9 @@
         continue;
       }
       // TODO(ulfjack): This is terribly inefficient.
-      ResolvedTargets<Target> asTargets = TestSuiteExpansionFunction.labelsToTargets(
-          env, value.getTargets().getTargets(), value.getTargets().hasError());
+      ResolvedTargets<Target> asTargets =
+          TestsForTargetPatternFunction.labelsToTargets(
+              env, value.getTargets().getTargets(), value.getTargets().hasError());
       if (asTargets == null) {
         continue;
       }
@@ -427,7 +428,7 @@
         // Skip.
         continue;
       }
-      expandedSuiteKeys.add(TestSuiteExpansionValue.key(value.getTargets().getTargets()));
+      expandedSuiteKeys.add(TestsForTargetPatternValue.key(value.getTargets().getTargets()));
     }
     Map<SkyKey, SkyValue> expandedSuites = env.getValues(expandedSuiteKeys);
     if (env.valuesMissing()) {
@@ -444,11 +445,12 @@
         continue;
       }
 
-      TestSuiteExpansionValue expandedSuitesValue = (TestSuiteExpansionValue) expandedSuites.get(
-          TestSuiteExpansionValue.key(value.getTargets().getTargets()));
+      TestsForTargetPatternValue expandedSuitesValue =
+          (TestsForTargetPatternValue)
+              expandedSuites.get(TestsForTargetPatternValue.key(value.getTargets().getTargets()));
       if (pattern.isNegative()) {
         ResolvedTargets<Target> negativeTargets =
-            TestSuiteExpansionFunction.labelsToTargets(
+            TestsForTargetPatternFunction.labelsToTargets(
                 env,
                 expandedSuitesValue.getLabels().getTargets(),
                 expandedSuitesValue.getLabels().hasError());
@@ -456,7 +458,7 @@
         testTargetsBuilder.mergeError(negativeTargets.hasError());
       } else {
         ResolvedTargets<Target> positiveTargets =
-            TestSuiteExpansionFunction.labelsToTargets(
+            TestsForTargetPatternFunction.labelsToTargets(
                 env,
                 expandedSuitesValue.getLabels().getTargets(),
                 expandedSuitesValue.getLabels().hasError());

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TestSuiteCycleReporter.java b/src/main/java/com/google/devtools/build/lib/skyframe/TestExpansionCycleReporter.java
similarity index 75%
rename from src/main/java/com/google/devtools/build/lib/skyframe/TestSuiteCycleReporter.java
rename to src/main/java/com/google/devtools/build/lib/skyframe/TestExpansionCycleReporter.java
index caf99aa..6edd0bb 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TestSuiteCycleReporter.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TestExpansionCycleReporter.java

@@ -15,29 +15,29 @@
 
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.pkgcache.PackageProvider;
-import com.google.devtools.build.lib.skyframe.TestsInSuiteValue.TestsInSuiteKey;
+import com.google.devtools.build.lib.skyframe.TestExpansionValue.TestExpansionKey;
 import com.google.devtools.build.skyframe.CycleInfo;
 import com.google.devtools.build.skyframe.SkyKey;
 
 /** Reports cycles occurring in during the expansion of <code>test_suite</code> rules. */
-class TestSuiteCycleReporter extends AbstractLabelCycleReporter {
+class TestExpansionCycleReporter extends AbstractLabelCycleReporter {
 
-  public TestSuiteCycleReporter(PackageProvider packageProvider) {
+  public TestExpansionCycleReporter(PackageProvider packageProvider) {
     super(packageProvider);
   }
 
   @Override
   protected boolean canReportCycle(SkyKey topLevelKey, CycleInfo cycleInfo) {
-    return cycleInfo.getCycle().stream().allMatch(TestsInSuiteKey.class::isInstance);
+    return cycleInfo.getCycle().stream().allMatch(TestExpansionKey.class::isInstance);
   }
 
   @Override
   protected boolean shouldSkip(SkyKey key) {
-    return !(key instanceof TestsInSuiteKey);
+    return !(key instanceof TestExpansionKey);
   }
 
   @Override
   protected Label getLabel(SkyKey key) {
-    return ((TestsInSuiteKey) key).getTestSuiteLabel();
+    return ((TestExpansionKey) key).getLabel();
   }
 }

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TestsInSuiteFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TestExpansionFunction.java
similarity index 72%
rename from src/main/java/com/google/devtools/build/lib/skyframe/TestsInSuiteFunction.java
rename to src/main/java/com/google/devtools/build/lib/skyframe/TestExpansionFunction.java
index 16e2c43..ac6777b 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TestsInSuiteFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TestExpansionFunction.java

@@ -18,17 +18,16 @@
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.cmdline.ResolvedTargets;
 import com.google.devtools.build.lib.events.Event;
+import com.google.devtools.build.lib.packages.AggregatingAttributeMapper;
 import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
-import com.google.devtools.build.lib.packages.BuildType;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.NoSuchTargetException;
-import com.google.devtools.build.lib.packages.NonconfigurableAttributeMapper;
 import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.packages.TargetUtils;
 import com.google.devtools.build.lib.packages.TestTargetUtils;
-import com.google.devtools.build.lib.skyframe.TestsInSuiteValue.TestsInSuiteKey;
+import com.google.devtools.build.lib.skyframe.TestExpansionValue.TestExpansionKey;
 import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
@@ -44,25 +43,25 @@
 import javax.annotation.Nullable;
 
 /**
- * TestsInSuiteFunction takes a single test_suite target and expands all of the tests it contains,
+ * TestExpansionFunction takes a single test_suite target and expands all of the tests it contains,
  * possibly recursively.
  */
 // TODO(ulfjack): What about test_suite rules that include each other.
-final class TestsInSuiteFunction implements SkyFunction {
+final class TestExpansionFunction implements SkyFunction {
   @Override
   public SkyValue compute(SkyKey key, Environment env) throws InterruptedException {
-    TestsInSuiteKey expansion = (TestsInSuiteKey) key.argument();
-    SkyKey packageKey = PackageValue.key(expansion.getTestSuiteLabel().getPackageIdentifier());
+    TestExpansionKey expansion = (TestExpansionKey) key.argument();
+    SkyKey packageKey = PackageValue.key(expansion.getLabel().getPackageIdentifier());
     PackageValue pkg = (PackageValue) env.getValue(packageKey);
     if (env.valuesMissing()) {
       return null;
     }
-    Rule testSuite = pkg.getPackage().getRule(expansion.getTestSuiteLabel().getName());
-    ResolvedTargets<Label> result = computeTestsInSuite(env, testSuite, expansion.isStrict());
+    Rule rule = pkg.getPackage().getRule(expansion.getLabel().getName());
+    ResolvedTargets<Label> result = computeExpandedTests(env, rule, expansion.isStrict());
     if (env.valuesMissing()) {
       return null;
     }
-    return new TestsInSuiteValue(result);
+    return new TestExpansionValue(result);
   }
 
   private static Set<Label> toLabels(Set<Target> targets) {
@@ -70,40 +69,45 @@
   }
 
   /**
-   * Populates 'result' with all the tests associated with the specified 'testSuite'. Throws an
-   * exception if any target is missing.
+   * Populates 'result' with all the tests associated with the specified 'rule'. Throws an exception
+   * if any target is missing.
    *
    * <p>CAUTION! Keep this logic consistent with {@code TestSuite}!
    */
-  private static ResolvedTargets<Label> computeTestsInSuite(
-      Environment env, Rule testSuite, boolean strict) throws InterruptedException {
+  private static ResolvedTargets<Label> computeExpandedTests(
+      Environment env, Rule rule, boolean strict) throws InterruptedException {
     Set<Target> result = new HashSet<>();
     boolean hasError = false;
 
-    List<Target> testsAndSuites = new ArrayList<>();
-    // Note that testsAndSuites can contain input file targets; the test_suite rule does not
+    List<Target> prerequisites = new ArrayList<>();
+    // Note that prerequisites can contain input file targets; the test_suite rule does not
     // restrict the set of targets that can appear in tests or suites.
-    hasError |= getPrerequisites(env, testSuite, "tests", testsAndSuites);
+    hasError |= getPrerequisites(env, rule, "tests", prerequisites);
 
     // 1. Add all tests
-    for (Target test : testsAndSuites) {
+    for (Target test : prerequisites) {
       if (TargetUtils.isTestRule(test)) {
         result.add(test);
       } else if (strict && !TargetUtils.isTestSuiteRule(test)) {
         // If strict mode is enabled, then give an error for any non-test, non-test-suite targets.
         // TODO(ulfjack): We need to throw to end the process if we happen to be in --nokeep_going,
         // but we can't know whether or not we are at this point.
-        env.getListener().handle(Event.error(testSuite.getLocation(),
-            "in test_suite rule '" + testSuite.getLabel()
-            + "': expecting a test or a test_suite rule but '" + test.getLabel()
-            + "' is not one."));
+        env.getListener()
+            .handle(
+                Event.error(
+                    rule.getLocation(),
+                    "in test_suite rule '"
+                        + rule.getLabel()
+                        + "': expecting a test or a test_suite rule but '"
+                        + test.getLabel()
+                        + "' is not one."));
         hasError = true;
       }
     }
 
     // 2. Add implicit dependencies on tests in same package, if any.
     List<Target> implicitTests = new ArrayList<>();
-    hasError |= getPrerequisites(env, testSuite, "$implicit_tests", implicitTests);
+    hasError |= getPrerequisites(env, rule, "$implicit_tests", implicitTests);
     for (Target target : implicitTests) {
       // The Package construction of $implicit_tests ensures that this check never fails, but we
       // add it here anyway for compatibility with future code.
@@ -113,17 +117,17 @@
     }
 
     // 3. Filter based on tags, size, env.
-    TestTargetUtils.filterTests(testSuite, result);
+    TestTargetUtils.filterTests(rule, result);
 
-    // 4. Expand all suites recursively, collecting labels.
+    // 4. Expand all rules recursively, collecting labels.
     ResolvedTargets.Builder<Label> labelsBuilder = ResolvedTargets.builder();
     // Don't set filtered targets; they would be removed from the containing test suite.
     labelsBuilder.merge(new ResolvedTargets<>(toLabels(result), ImmutableSet.of(), hasError));
 
-    for (Target suite : testsAndSuites) {
+    for (Target suite : prerequisites) {
       if (TargetUtils.isTestSuiteRule(suite)) {
-        TestsInSuiteValue value =
-            (TestsInSuiteValue) env.getValue(TestsInSuiteValue.key(suite, strict));
+        TestExpansionValue value =
+            (TestExpansionValue) env.getValue(TestExpansionValue.key(suite, strict));
         if (value == null) {
           continue;
         }
@@ -136,19 +140,24 @@
 
   /**
    * Adds the set of targets found in the attribute named {@code attrName}, which must be of label
-   * list type, of the {@code test_suite} rule named {@code testSuite}. Returns true if the method
-   * found a problem during the lookup process; the actual error message is reported to the
+   * or label list type, of the {@code test_suite} rule named {@code testSuite}. Returns true if the
+   * method found a problem during the lookup process; the actual error message is reported to the
    * environment.
    */
   private static boolean getPrerequisites(
-      Environment env, Rule testSuite, String attrName, List<Target> targets)
+      Environment env, Rule rule, String attrName, List<Target> targets)
       throws InterruptedException {
+    AggregatingAttributeMapper mapper = AggregatingAttributeMapper.of(rule);
     List<Label> labels =
-        NonconfigurableAttributeMapper.of(testSuite).get(attrName, BuildType.LABEL_LIST);
+        mapper.visitLabels(mapper.getAttributeDefinition(attrName)).stream()
+            .map(e -> e.getLabel())
+            .collect(Collectors.toList());
+
     Set<PackageIdentifier> pkgIdentifiers = new LinkedHashSet<>();
     for (Label label : labels) {
       pkgIdentifiers.add(label.getPackageIdentifier());
     }
+
     Map<SkyKey, ValueOrException<NoSuchPackageException>> packages =
         env.getValuesOrThrow(PackageValue.keys(pkgIdentifiers), NoSuchPackageException.class);
     if (env.valuesMissing()) {

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TestsInSuiteValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TestExpansionValue.java
similarity index 71%
rename from src/main/java/com/google/devtools/build/lib/skyframe/TestsInSuiteValue.java
rename to src/main/java/com/google/devtools/build/lib/skyframe/TestExpansionValue.java
index da5124a..1b1d88f 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TestsInSuiteValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TestExpansionValue.java

@@ -35,10 +35,10 @@
  */
 @Immutable
 @ThreadSafe
-final class TestsInSuiteValue implements SkyValue {
+final class TestExpansionValue implements SkyValue {
   private ResolvedTargets<Label> labels;
 
-  TestsInSuiteValue(ResolvedTargets<Label> labels) {
+  TestExpansionValue(ResolvedTargets<Label> labels) {
     this.labels = Preconditions.checkNotNull(labels);
   }
 
@@ -64,24 +64,22 @@
   /**
    * Create a target pattern value key.
    *
-   * @param testSuiteTarget the test suite target to be expanded
+   * @param target the target to be expanded
    */
   @ThreadSafe
-  public static SkyKey key(Target testSuiteTarget, boolean strict) {
-    Preconditions.checkState(TargetUtils.isTestSuiteRule(testSuiteTarget));
-    return new TestsInSuiteKey(testSuiteTarget.getLabel(), strict);
+  public static SkyKey key(Target target, boolean strict) {
+    Preconditions.checkState(TargetUtils.isTestSuiteRule(target));
+    return new TestExpansionKey(target.getLabel(), strict);
   }
 
-  /**
-   * A list of targets of which all test suites should be expanded.
-   */
+  /** A list of targets of which all test suites should be expanded. */
   @ThreadSafe
-  static final class TestsInSuiteKey implements SkyKey, Serializable {
-    private final Label testSuiteLabel;
+  static final class TestExpansionKey implements SkyKey, Serializable {
+    private final Label label;
     private final boolean strict;
 
-    public TestsInSuiteKey(Label testSuiteLabel, boolean strict) {
-      this.testSuiteLabel = testSuiteLabel;
+    public TestExpansionKey(Label label, boolean strict) {
+      this.label = label;
       this.strict = strict;
     }
 
@@ -90,8 +88,8 @@
       return SkyFunctions.TESTS_IN_SUITE;
     }
 
-    public Label getTestSuiteLabel() {
-      return testSuiteLabel;
+    public Label getLabel() {
+      return label;
     }
 
     public boolean isStrict() {
@@ -100,12 +98,12 @@
 
     @Override
     public String toString() {
-      return "TestsInSuite(" + testSuiteLabel.toString() + ", strict=" + strict + ")";
+      return "TestsInSuite(" + label + ", strict=" + strict + ")";
     }
 
     @Override
     public int hashCode() {
-      return Objects.hash(testSuiteLabel, strict);
+      return Objects.hash(label, strict);
     }
 
     @Override
@@ -113,11 +111,11 @@
       if (this == obj) {
         return true;
       }
-      if (!(obj instanceof TestsInSuiteKey)) {
+      if (!(obj instanceof TestExpansionKey)) {
         return false;
       }
-      TestsInSuiteKey other = (TestsInSuiteKey) obj;
-      return other.testSuiteLabel.equals(testSuiteLabel) && other.strict == strict;
+      TestExpansionKey other = (TestExpansionKey) obj;
+      return other.label.equals(label) && other.strict == strict;
     }
   }
 }

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TestSuiteExpansionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TestsForTargetPatternFunction.java
similarity index 85%
rename from src/main/java/com/google/devtools/build/lib/skyframe/TestSuiteExpansionFunction.java
rename to src/main/java/com/google/devtools/build/lib/skyframe/TestsForTargetPatternFunction.java
index 0b563f7..45f767e 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TestSuiteExpansionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TestsForTargetPatternFunction.java

@@ -21,7 +21,7 @@
 import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.packages.TargetUtils;
-import com.google.devtools.build.lib.skyframe.TestSuiteExpansionValue.TestSuiteExpansionKey;
+import com.google.devtools.build.lib.skyframe.TestsForTargetPatternValue.TestsForTargetPatternKey;
 import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
@@ -34,17 +34,19 @@
 import javax.annotation.Nullable;
 
 /**
- * TestSuiteExpansionFunction takes a list of targets and expands all test suites in those targets.
+ * Returns all tests that need to be run when testing is requested for a given set of targets.
+ *
+ * <p>This requires resolving {@code test_suite} rules.
  */
-final class TestSuiteExpansionFunction implements SkyFunction {
+final class TestsForTargetPatternFunction implements SkyFunction {
   @Override
   public SkyValue compute(SkyKey key, Environment env) throws InterruptedException {
-    TestSuiteExpansionKey expansion = (TestSuiteExpansionKey) key.argument();
+    TestsForTargetPatternKey expansion = (TestsForTargetPatternKey) key.argument();
     ResolvedTargets<Target> targets = labelsToTargets(env, expansion.getTargets(), false);
     List<SkyKey> testsInSuitesKeys = new ArrayList<>();
     for (Target target : targets.getTargets()) {
       if (TargetUtils.isTestSuiteRule(target)) {
-        testsInSuitesKeys.add(TestsInSuiteValue.key(target, true));
+        testsInSuitesKeys.add(TestExpansionValue.key(target, true));
       }
     }
     Map<SkyKey, SkyValue> testsInSuites = env.getValues(testsInSuitesKeys);
@@ -58,8 +60,8 @@
       if (TargetUtils.isTestRule(target)) {
         result.add(target.getLabel());
       } else if (TargetUtils.isTestSuiteRule(target)) {
-        TestsInSuiteValue value = (TestsInSuiteValue) testsInSuites.get(
-            TestsInSuiteValue.key(target, true));
+        TestExpansionValue value =
+            (TestExpansionValue) testsInSuites.get(TestExpansionValue.key(target, true));
         if (value != null) {
           result.addAll(value.getLabels().getTargets());
           hasError |= value.getLabels().hasError();
@@ -73,7 +75,7 @@
     }
     // We use ResolvedTargets in order to associate an error flag; the result should never contain
     // any filtered targets.
-    return new TestSuiteExpansionValue(new ResolvedTargets<>(result, hasError));
+    return new TestsForTargetPatternValue(new ResolvedTargets<>(result, hasError));
   }
 
   static ResolvedTargets<Target> labelsToTargets(

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TestSuiteExpansionValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TestsForTargetPatternValue.java
similarity index 87%
rename from src/main/java/com/google/devtools/build/lib/skyframe/TestSuiteExpansionValue.java
rename to src/main/java/com/google/devtools/build/lib/skyframe/TestsForTargetPatternValue.java
index d35b19c..4502249 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TestSuiteExpansionValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TestsForTargetPatternValue.java

@@ -37,10 +37,10 @@
 @Immutable
 @ThreadSafe
 @VisibleForTesting
-public final class TestSuiteExpansionValue implements SkyValue {
+public final class TestsForTargetPatternValue implements SkyValue {
   private ResolvedTargets<Label> labels;
 
-  TestSuiteExpansionValue(ResolvedTargets<Label> labels) {
+  TestsForTargetPatternValue(ResolvedTargets<Label> labels) {
     this.labels = Preconditions.checkNotNull(labels);
   }
 
@@ -48,7 +48,6 @@
     return labels;
   }
 
-
   @SuppressWarnings("unused")
   private void writeObject(ObjectOutputStream out) {
     throw new NotSerializableRuntimeException();
@@ -71,16 +70,16 @@
    */
   @ThreadSafe
   public static SkyKey key(Collection<Label> targets) {
-    return new TestSuiteExpansionKey(ImmutableSortedSet.copyOf(targets));
+    return new TestsForTargetPatternKey(ImmutableSortedSet.copyOf(targets));
   }
 
   /** A list of targets of which all test suites should be expanded. */
   @AutoCodec
   @ThreadSafe
-  static final class TestSuiteExpansionKey implements SkyKey {
+  static final class TestsForTargetPatternKey implements SkyKey {
     private final ImmutableSortedSet<Label> targets;
 
-    public TestSuiteExpansionKey(ImmutableSortedSet<Label> targets) {
+    public TestsForTargetPatternKey(ImmutableSortedSet<Label> targets) {
       this.targets = targets;
     }
 
@@ -108,10 +107,10 @@
       if (this == obj) {
         return true;
       }
-      if (!(obj instanceof TestSuiteExpansionKey)) {
+      if (!(obj instanceof TestsForTargetPatternKey)) {
         return false;
       }
-      TestSuiteExpansionKey other = (TestSuiteExpansionKey) obj;
+      TestsForTargetPatternKey other = (TestsForTargetPatternKey) obj;
       return other.targets.equals(targets);
     }
   }