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);
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/packages/TestTargetUtilsTest.java b/src/test/java/com/google/devtools/build/lib/packages/TestTargetUtilsTest.java
index a1a1a8e..a71401c 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/TestTargetUtilsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/TestTargetUtilsTest.java
@@ -22,16 +22,13 @@
import com.google.common.collect.Sets;
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.events.Location;
import com.google.devtools.build.lib.events.StoredEventHandler;
import com.google.devtools.build.lib.packages.util.PackageLoadingTestCase;
import com.google.devtools.build.lib.pkgcache.LoadingOptions;
-import com.google.devtools.build.lib.pkgcache.TargetProvider;
import com.google.devtools.build.lib.pkgcache.TestFilter;
-import com.google.devtools.build.lib.skyframe.TestSuiteExpansionValue;
+import com.google.devtools.build.lib.skyframe.TestsForTargetPatternValue;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.skyframe.EvaluationContext;
import com.google.devtools.build.skyframe.EvaluationResult;
@@ -157,18 +154,6 @@
}
@Test
- public void testExpandTestSuites() throws Exception {
- assertExpandedSuites(Sets.newHashSet(test1, test2), Sets.newHashSet(test1, test2));
- assertExpandedSuites(Sets.newHashSet(test1, test2), Sets.newHashSet(suite));
- assertExpandedSuites(
- Sets.newHashSet(test1, test2, test1b), Sets.newHashSet(test1, suite, test1b));
- // The large test if returned as filtered from the test_suite rule, but should still be in the
- // result set as it's explicitly added.
- assertExpandedSuites(
- Sets.newHashSet(test1, test2, test1b), ImmutableSet.<Target>of(test1b, suite));
- }
-
- @Test
public void testSkyframeExpandTestSuites() throws Exception {
assertExpandedSuitesSkyframe(
Sets.newHashSet(test1, test2), ImmutableSet.<Target>of(test1, test2));
@@ -181,30 +166,6 @@
Sets.newHashSet(test1, test2, test1b), ImmutableSet.<Target>of(test1b, suite));
}
- @Test
- public void testExpandTestSuitesKeepGoing() throws Exception {
- reporter.removeHandler(failFastHandler);
- scratch.file("broken/BUILD", "test_suite(name = 'broken', tests = ['//missing:missing_test'])");
- ResolvedTargets<Target> actual =
- TestTargetUtils.expandTestSuites(
- getPackageManager(),
- reporter,
- Sets.newHashSet(getTarget("//broken")),
- /*strict=*/ false,
- /* keepGoing= */ true);
- assertThat(actual.hasError()).isTrue();
- assertThat(actual.getTargets()).isEmpty();
- }
-
- private void assertExpandedSuites(Iterable<Target> expected, Collection<Target> suites)
- throws Exception {
- ResolvedTargets<Target> actual =
- TestTargetUtils.expandTestSuites(
- getPackageManager(), reporter, suites, /*strict=*/ false, /* keepGoing= */ true);
- assertThat(actual.hasError()).isFalse();
- assertThat(actual.getTargets()).containsExactlyElementsIn(expected);
- }
-
private static final Function<Target, Label> TO_LABEL =
new Function<Target, Label>() {
@Override
@@ -218,40 +179,17 @@
ImmutableSet<Label> expectedLabels =
ImmutableSet.copyOf(Iterables.transform(expected, TO_LABEL));
ImmutableSet<Label> suiteLabels = ImmutableSet.copyOf(Iterables.transform(suites, TO_LABEL));
- SkyKey key = TestSuiteExpansionValue.key(suiteLabels);
+ SkyKey key = TestsForTargetPatternValue.key(suiteLabels);
EvaluationContext evaluationContext =
EvaluationContext.newBuilder()
.setKeepGoing(false)
.setNumThreads(1)
.setEventHander(reporter)
.build();
- EvaluationResult<TestSuiteExpansionValue> result =
+ EvaluationResult<TestsForTargetPatternValue> result =
getSkyframeExecutor().getDriver().evaluate(ImmutableList.of(key), evaluationContext);
ResolvedTargets<Label> actual = result.get(key).getLabels();
assertThat(actual.hasError()).isFalse();
assertThat(actual.getTargets()).containsExactlyElementsIn(expectedLabels);
}
-
- @Test
- public void testExpandTestSuitesInterrupted() throws Exception {
- reporter.removeHandler(failFastHandler);
- scratch.file("broken/BUILD", "test_suite(name = 'broken', tests = ['//missing:missing_test'])");
- try {
- TestTargetUtils.expandTestSuites(
- new TargetProvider() {
- @Override
- public Target getTarget(ExtendedEventHandler eventHandler, Label label)
- throws InterruptedException {
- throw new InterruptedException();
- }
- },
- reporter,
- Sets.newHashSet(getTarget("//broken")),
- /*strict=*/ false,
- /* keepGoing= */ true);
- } catch (TargetParsingException e) {
- assertThat(e).hasMessageThat().isNotNull();
- }
- assertThat(Thread.currentThread().isInterrupted()).isTrue();
- }
}
