Skip to content

Commit

Permalink
With interleaving now enabled, clean up our tests.
Browse files Browse the repository at this point in the history 
Remove many calls from tests to SkyframeLabelVisitor.sync, which is generally
no longer necessary. In particular, BuildViewTestCase.ensureTarget(s)Visited
was calling it, so removing that, as well as simulateLoadingPhase.

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

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

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

Some of the tests are not externally visible yet.

--
PiperOrigin-RevId: 144813237
MOS_MIGRATED_REVID=144813237
  • Loading branch information
@ulfjack @vladmos
ulfjack authored and vladmos committed Jan 18, 2017
1 parent f2b5f27 commit af0b670
Show file tree
Hide file tree
Showing 6 changed files with 95 additions and 177 deletions.
39 changes: 39 additions & 0 deletions src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,30 @@ public void testGeneratedArtifact() throws Exception {
assertSame(FailAction.class, action.getClass());
}

@Test
public void testSyntaxErrorInDepPackage() throws Exception {
// Check that a loading error in a dependency is properly reported.
scratch.file("a/BUILD",
"genrule(name='x',",
" srcs = ['file.txt'],",
" outs = ['foo'],",
" cmd = 'echo')",
"@"); // syntax error

scratch.file("b/BUILD",
"genrule(name= 'cc',",
" tools = ['//a:x'],",
" outs = ['bar'],",
" cmd = 'echo')");

reporter.removeHandler(failFastHandler);
EventBus eventBus = new EventBus();
AnalysisResult result = update(eventBus, defaultFlags().with(Flag.KEEP_GOING), "//b:cc");

assertContainsEvent("invalid character: '@'");
assertThat(result.hasError()).isTrue();
}

@Test
public void testReportsAnalysisRootCauses() throws Exception {
scratch.file("private/BUILD",
Expand Down Expand Up @@ -1284,6 +1308,21 @@ public void ruleExtraActionsDontHideAspectExtraActions() throws Exception {
assertThat(owners).containsExactly("//x:b", "//x:b");
}

@Test
public void testErrorMessageForMissingPackageGroup() throws Exception {
scratch.file(
"apple/BUILD",
"py_library(name='apple', visibility=['//non:existent'])");
reporter.removeHandler(failFastHandler);
try {
update("//apple");
fail();
} catch (ViewCreationFailedException e) {
// Expected.
}
assertDoesNotContainEvent("implicitly depends upon");
}

/** Runs the same test with the reduced loading phase. */
@TestSpec(size = Suite.SMALL_TESTS)
@RunWith(JUnit4.class)
Expand Down
107 changes: 20 additions & 87 deletions src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,6 @@ protected final BuildConfigurationCollection createConfigurations(String... args
optionsPolicyEnforcer.enforce(optionsParser);

BuildOptions buildOptions = ruleClassProvider.createBuildOptions(optionsParser);
ensureTargetsVisited(buildOptions.getAllLabels().values());
skyframeExecutor.invalidateConfigurationCollection();
return skyframeExecutor.createConfigurations(reporter, configurationFactory, buildOptions,
ImmutableSet.<String>of(), false);
Expand Down Expand Up @@ -441,7 +440,6 @@ protected final void createBuildView() throws Exception {

view.setArtifactRoots(
ImmutableMap.of(PackageIdentifier.createInMainRepo(""), rootDirectory));
simulateLoadingPhase();
}

protected CachingAnalysisEnvironment getTestAnalysisEnvironment() {
Expand Down Expand Up @@ -646,27 +644,10 @@ protected SpawnAction getGeneratingSpawnAction(ConfiguredTarget target, String o
Iterables.find(getFilesToBuild(target), artifactNamed(outputName)));
}

protected void simulateLoadingPhase() {
try {
ensureTargetsVisited(targetConfig.getAllLabels().values());
} catch (Exception e) {
throw new RuntimeException(e);
}
}

protected ActionsTestUtil actionsTestUtil() {
return new ActionsTestUtil(getActionGraph());
}

private Set<Target> getTargets(Iterable<Label> labels) throws InterruptedException,
NoSuchTargetException, NoSuchPackageException{
Set<Target> targets = Sets.newHashSet();
for (Label label : labels) {
targets.add(skyframeExecutor.getPackageManager().getTarget(reporter, label));
}
return targets;
}

// Get a MutableActionGraph for testing purposes.
protected MutableActionGraph getMutableActionGraph() {
return mutableActionGraph;
Expand All @@ -677,51 +658,12 @@ protected TransitivePackageLoader makeVisitor() {
return skyframeExecutor.pkgLoader();
}

/**
* Construct the containing package of the specified labels, and all of its transitive
* dependencies. This must be done prior to configuration, as the latter is intolerant of
* NoSuchTargetExceptions.
*/
protected boolean ensureTargetsVisited(TransitivePackageLoader visitor,
Collection<Label> targets, Collection<Label> labels, boolean keepGoing)
throws InterruptedException, NoSuchTargetException, NoSuchPackageException {
boolean success = visitor.sync(reporter,
getTargets(BlazeTestUtils.convertLabels(targets)),
ImmutableSet.copyOf(BlazeTestUtils.convertLabels(labels)),
keepGoing,
/*parallelThreads=*/4,
/*maxDepth=*/Integer.MAX_VALUE);
return success;
}

protected boolean ensureTargetsVisited(Collection<Label> labels)
throws InterruptedException, NoSuchTargetException, NoSuchPackageException {
return ensureTargetsVisited(makeVisitor(), ImmutableSet.<Label>of(), labels,
/*keepGoing=*/false);
}

protected boolean ensureTargetsVisited(Label label)
throws InterruptedException, NoSuchTargetException, NoSuchPackageException {
return ensureTargetsVisited(ImmutableList.of(label));
}

protected boolean ensureTargetsVisited(String... labels)
throws InterruptedException, NoSuchTargetException, NoSuchPackageException,
LabelSyntaxException {
List<Label> actualLabels = new ArrayList<>();
for (String label : labels) {
actualLabels.add(Label.parseAbsolute(label));
}
return ensureTargetsVisited(actualLabels);
}

/**
* Returns the ConfiguredTarget for the specified label, configured for the "build" (aka "target")
* configuration.
*/
public ConfiguredTarget getConfiguredTarget(String label)
throws NoSuchPackageException, NoSuchTargetException, LabelSyntaxException,
InterruptedException {
throws LabelSyntaxException {
return getConfiguredTarget(label, targetConfig);
}

Expand All @@ -730,8 +672,7 @@ public ConfiguredTarget getConfiguredTarget(String label)
* given build configuration.
*/
protected ConfiguredTarget getConfiguredTarget(String label, BuildConfiguration config)
throws NoSuchPackageException, NoSuchTargetException,
LabelSyntaxException, InterruptedException {
throws LabelSyntaxException {
return getConfiguredTarget(Label.parseAbsolute(label), config);
}

Expand All @@ -745,8 +686,7 @@ protected ConfiguredTarget getConfiguredTarget(String label, BuildConfiguration
* {@link MemoizingEvaluator#getExistingValueForTesting} call in
* {@link SkyframeExecutor#getConfiguredTargetForTesting}. See also b/26382502.
*/
protected ConfiguredTarget getConfiguredTarget(Label label, BuildConfiguration config)
throws NoSuchPackageException, NoSuchTargetException, InterruptedException {
protected ConfiguredTarget getConfiguredTarget(Label label, BuildConfiguration config) {
return view.getConfiguredTargetForTesting(
reporter, BlazeTestUtils.convertLabel(label), config);
}
Expand All @@ -756,8 +696,7 @@ protected ConfiguredTarget getConfiguredTarget(Label label, BuildConfiguration c
* the "build" (aka "target") configuration.
*/
protected FileConfiguredTarget getFileConfiguredTarget(String label)
throws NoSuchPackageException, NoSuchTargetException,
LabelSyntaxException, InterruptedException {
throws LabelSyntaxException {
return (FileConfiguredTarget) getConfiguredTarget(label, targetConfig);
}

Expand All @@ -766,8 +705,7 @@ protected FileConfiguredTarget getFileConfiguredTarget(String label)
* the "host" configuration.
*/
protected ConfiguredTarget getHostConfiguredTarget(String label)
throws NoSuchPackageException, NoSuchTargetException,
LabelSyntaxException, InterruptedException {
throws LabelSyntaxException {
return getConfiguredTarget(label, getHostConfiguration());
}

Expand All @@ -776,8 +714,7 @@ protected ConfiguredTarget getHostConfiguredTarget(String label)
* the "host" configuration.
*/
protected FileConfiguredTarget getHostFileConfiguredTarget(String label)
throws NoSuchPackageException, NoSuchTargetException,
LabelSyntaxException, InterruptedException {
throws LabelSyntaxException {
return (FileConfiguredTarget) getHostConfiguredTarget(label);
}

Expand Down Expand Up @@ -839,11 +776,7 @@ protected ConfiguredTarget scratchConfiguredTarget(String packageName,
String... lines)
throws IOException, Exception {
Target rule = scratchRule(packageName, ruleName, lines);
if (ensureTargetsVisited(rule.getLabel())) {
return view.getConfiguredTargetForTesting(reporter, rule.getLabel(), config);
} else {
return null;
}
return view.getConfiguredTargetForTesting(reporter, rule.getLabel(), config);
}

/**
Expand Down Expand Up @@ -1000,7 +933,7 @@ private Artifact getPackageRelativeDerivedArtifact(String packageRelativePath, R
}

/**
* Gets a derived Artifact for testing in the {@link BuildConfiguration#getBinDirectory()}. This
* Gets a derived Artifact for testing in the {@link BuildConfiguration#getBinDirectory}. This
* method should only be used for tests that do no analysis, and so there is no ConfiguredTarget
* to own this artifact. If the test runs the analysis phase, {@link
* #getBinArtifact(String, ArtifactOwner)} or its convenience methods should be
Expand All @@ -1014,7 +947,7 @@ protected Artifact getBinArtifactWithNoOwner(String rootRelativePath) {

/**
* Gets a derived Artifact for testing in the subdirectory of the {@link
* BuildConfiguration#getBinDirectory()} corresponding to the package of {@code owner}. So
* BuildConfiguration#getBinDirectory} corresponding to the package of {@code owner}. So
* to specify a file foo/foo.o owned by target //foo:foo, {@code packageRelativePath} should just
* be "foo.o".
*/
Expand All @@ -1024,7 +957,7 @@ protected Artifact getBinArtifact(String packageRelativePath, String owner) {

/**
* Gets a derived Artifact for testing in the subdirectory of the {@link
* BuildConfiguration#getBinDirectory()} corresponding to the package of {@code owner}. So
* BuildConfiguration#getBinDirectory} corresponding to the package of {@code owner}. So
* to specify a file foo/foo.o owned by target //foo:foo, {@code packageRelativePath} should just
* be "foo.o".
*/
Expand All @@ -1036,7 +969,7 @@ protected Artifact getBinArtifact(String packageRelativePath, ConfiguredTarget o

/**
* Gets a derived Artifact for testing in the subdirectory of the {@link
* BuildConfiguration#getBinDirectory()} corresponding to the package of {@code owner}, where the
* BuildConfiguration#getBinDirectory} corresponding to the package of {@code owner}, where the
* given artifact belongs to the given ConfiguredTarget together with the given Aspect. So to
* specify a file foo/foo.o owned by target //foo:foo with an aspect from FooAspect, {@code
* packageRelativePath} should just be "foo.o", and aspectOfOwner should be FooAspect.class. This
Expand All @@ -1051,7 +984,7 @@ protected Artifact getBinArtifact(

/**
* Gets a derived Artifact for testing in the subdirectory of the {@link
* BuildConfiguration#getBinDirectory()} corresponding to the package of {@code owner}, where the
* BuildConfiguration#getBinDirectory} corresponding to the package of {@code owner}, where the
* given artifact belongs to the given ConfiguredTarget together with the given Aspect. So to
* specify a file foo/foo.o owned by target //foo:foo with an aspect from FooAspect, {@code
* packageRelativePath} should just be "foo.o", and aspectOfOwner should be FooAspect.class. This
Expand All @@ -1076,7 +1009,7 @@ protected Artifact getBinArtifact(

/**
* Gets a derived Artifact for testing in the subdirectory of the {@link
* BuildConfiguration#getBinDirectory()} corresponding to the package of {@code owner}. So
* BuildConfiguration#getBinDirectory} corresponding to the package of {@code owner}. So
* to specify a file foo/foo.o owned by target //foo:foo, {@code packageRelativePath} should just
* be "foo.o".
*/
Expand All @@ -1087,7 +1020,7 @@ private Artifact getBinArtifact(String packageRelativePath, ArtifactOwner owner)
}

/**
* Gets a derived Artifact for testing in the {@link BuildConfiguration#getGenfilesDirectory()}.
* Gets a derived Artifact for testing in the {@link BuildConfiguration#getGenfilesDirectory}.
* This method should only be used for tests that do no analysis, and so there is no
* ConfiguredTarget to own this artifact. If the test runs the analysis phase, {@link
* #getGenfilesArtifact(String, ArtifactOwner)} or its convenience methods should be used instead.
Expand All @@ -1100,7 +1033,7 @@ protected Artifact getGenfilesArtifactWithNoOwner(String rootRelativePath) {

/**
* Gets a derived Artifact for testing in the subdirectory of the {@link
* BuildConfiguration#getGenfilesDirectory()} corresponding to the package of {@code owner}.
* BuildConfiguration#getGenfilesDirectory} corresponding to the package of {@code owner}.
* So to specify a file foo/foo.o owned by target //foo:foo, {@code packageRelativePath} should
* just be "foo.o".
*/
Expand All @@ -1110,7 +1043,7 @@ protected Artifact getGenfilesArtifact(String packageRelativePath, String owner)

/**
* Gets a derived Artifact for testing in the subdirectory of the {@link
* BuildConfiguration#getGenfilesDirectory()} corresponding to the package of {@code owner}.
* BuildConfiguration#getGenfilesDirectory} corresponding to the package of {@code owner}.
* So to specify a file foo/foo.o owned by target //foo:foo, {@code packageRelativePath} should
* just be "foo.o".
*/
Expand All @@ -1120,7 +1053,7 @@ protected Artifact getGenfilesArtifact(String packageRelativePath, ConfiguredTar

/**
* Gets a derived Artifact for testing in the subdirectory of the {@link
* BuildConfiguration#getGenfilesDirectory()} corresponding to the package of {@code owner},
* BuildConfiguration#getGenfilesDirectory} corresponding to the package of {@code owner},
* where the given artifact belongs to the given ConfiguredTarget together with the given Aspect.
* So to specify a file foo/foo.o owned by target //foo:foo with an apsect from FooAspect,
* {@code packageRelativePath} should just be "foo.o", and aspectOfOwner should be
Expand Down Expand Up @@ -1164,7 +1097,7 @@ protected String stripCppPrefixForDynamicConfigs(String outputPath) {

/**
* Gets a derived Artifact for testing in the subdirectory of the {@link
* BuildConfiguration#getGenfilesDirectory()} corresponding to the package of {@code owner}.
* BuildConfiguration#getGenfilesDirectory} corresponding to the package of {@code owner}.
* So to specify a file foo/foo.o owned by target //foo:foo, {@code packageRelativePath} should
* just be "foo.o".
*/
Expand All @@ -1176,7 +1109,7 @@ private Artifact getGenfilesArtifact(String packageRelativePath, ArtifactOwner o

/**
* Gets a derived Artifact for testing in the subdirectory of the {@link
* BuildConfiguration#getIncludeDirectory()} corresponding to the package of {@code owner}.
* BuildConfiguration#getIncludeDirectory} corresponding to the package of {@code owner}.
* So to specify a file foo/foo.o owned by target //foo:foo, {@code packageRelativePath} should
* just be "foo.h".
*/
Expand All @@ -1186,7 +1119,7 @@ protected Artifact getIncludeArtifact(String packageRelativePath, String owner)

/**
* Gets a derived Artifact for testing in the subdirectory of the {@link
* BuildConfiguration#getIncludeDirectory()} corresponding to the package of {@code owner}.
* BuildConfiguration#getIncludeDirectory} corresponding to the package of {@code owner}.
* So to specify a file foo/foo.o owned by target //foo:foo, {@code packageRelativePath} should
* just be "foo.h".
*/
Expand Down
Loading

0 comments on commit af0b670

Please sign in to comment.