diff options
Diffstat (limited to 'src/test/java/com/google/devtools')
6 files changed, 95 insertions, 177 deletions
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java index 63e7484e6d..d7be1ffaf4 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java @@ -167,6 +167,30 @@ public class BuildViewTest extends BuildViewTestBase { } @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", "genrule(", @@ -1284,6 +1308,21 @@ public class BuildViewTest extends BuildViewTestBase { 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) diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java index f4c57c2cfd..476f90dd67 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java @@ -304,7 +304,6 @@ public abstract class BuildViewTestCase extends FoundationTestCase { optionsPolicyEnforcer.enforce(optionsParser); BuildOptions buildOptions = ruleClassProvider.createBuildOptions(optionsParser); - ensureTargetsVisited(buildOptions.getAllLabels().values()); skyframeExecutor.invalidateConfigurationCollection(); return skyframeExecutor.createConfigurations(reporter, configurationFactory, buildOptions, ImmutableSet.<String>of(), false); @@ -441,7 +440,6 @@ public abstract class BuildViewTestCase extends FoundationTestCase { view.setArtifactRoots( ImmutableMap.of(PackageIdentifier.createInMainRepo(""), rootDirectory)); - simulateLoadingPhase(); } protected CachingAnalysisEnvironment getTestAnalysisEnvironment() { @@ -646,27 +644,10 @@ public abstract class BuildViewTestCase extends FoundationTestCase { 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; @@ -678,50 +659,11 @@ public abstract class BuildViewTestCase extends FoundationTestCase { } /** - * 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); } @@ -730,8 +672,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase { * given build configuration. */ protected ConfiguredTarget getConfiguredTarget(String label, BuildConfiguration config) - throws NoSuchPackageException, NoSuchTargetException, - LabelSyntaxException, InterruptedException { + throws LabelSyntaxException { return getConfiguredTarget(Label.parseAbsolute(label), config); } @@ -745,8 +686,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase { * {@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); } @@ -756,8 +696,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase { * the "build" (aka "target") configuration. */ protected FileConfiguredTarget getFileConfiguredTarget(String label) - throws NoSuchPackageException, NoSuchTargetException, - LabelSyntaxException, InterruptedException { + throws LabelSyntaxException { return (FileConfiguredTarget) getConfiguredTarget(label, targetConfig); } @@ -766,8 +705,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase { * the "host" configuration. */ protected ConfiguredTarget getHostConfiguredTarget(String label) - throws NoSuchPackageException, NoSuchTargetException, - LabelSyntaxException, InterruptedException { + throws LabelSyntaxException { return getConfiguredTarget(label, getHostConfiguration()); } @@ -776,8 +714,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase { * the "host" configuration. */ protected FileConfiguredTarget getHostFileConfiguredTarget(String label) - throws NoSuchPackageException, NoSuchTargetException, - LabelSyntaxException, InterruptedException { + throws LabelSyntaxException { return (FileConfiguredTarget) getHostConfiguredTarget(label); } @@ -839,11 +776,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase { 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); } /** @@ -1000,7 +933,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase { } /** - * 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 @@ -1014,7 +947,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase { /** * 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". */ @@ -1024,7 +957,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase { /** * 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". */ @@ -1036,7 +969,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase { /** * 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 @@ -1051,7 +984,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase { /** * 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 @@ -1076,7 +1009,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase { /** * 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". */ @@ -1087,7 +1020,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase { } /** - * 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. @@ -1100,7 +1033,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase { /** * 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". */ @@ -1110,7 +1043,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase { /** * 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". */ @@ -1120,7 +1053,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase { /** * 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 @@ -1164,7 +1097,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase { /** * 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". */ @@ -1176,7 +1109,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase { /** * 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". */ @@ -1186,7 +1119,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase { /** * 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". */ diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/IOExceptionsTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/IOExceptionsTest.java index 901e0bfabf..0f546ae21c 100644 --- a/src/test/java/com/google/devtools/build/lib/pkgcache/IOExceptionsTest.java +++ b/src/test/java/com/google/devtools/build/lib/pkgcache/IOExceptionsTest.java @@ -13,16 +13,14 @@ // limitations under the License. package com.google.devtools.build.lib.pkgcache; -import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import com.google.common.base.Function; -import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.packages.ConstantRuleVisibility; -import com.google.devtools.build.lib.packages.Target; import com.google.devtools.build.lib.packages.util.PackageLoadingTestCase; +import com.google.devtools.build.lib.skyframe.TransitiveTargetValue; import com.google.devtools.build.lib.util.BlazeClock; import com.google.devtools.build.lib.vfs.FileStatus; import com.google.devtools.build.lib.vfs.FileSystem; @@ -30,16 +28,16 @@ import com.google.devtools.build.lib.vfs.ModifiedFileSet; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; - +import com.google.devtools.build.skyframe.EvaluationResult; +import com.google.devtools.build.skyframe.SkyKey; +import com.google.devtools.build.skyframe.SkyValue; +import java.io.IOException; +import javax.annotation.Nullable; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; -import java.io.IOException; - -import javax.annotation.Nullable; - /** * Tests for recovering from IOExceptions thrown by the filesystem when reading BUILD files. Needs * its own test class because it uses a custom filesystem. @@ -67,6 +65,16 @@ public class IOExceptionsTest extends PackageLoadingTestCase { this.visitor = skyframeExecutor.pkgLoader(); } + private boolean visitTransitively(Label label) throws InterruptedException { + SkyKey key = TransitiveTargetValue.key(label); + EvaluationResult<SkyValue> result = + skyframeExecutor.prepareAndGet(key, /*numThreads=*/5, reporter); + TransitiveTargetValue value = (TransitiveTargetValue) result.get(key); + System.out.println(value); + boolean hasTransitiveError = (value == null) || value.getTransitiveRootCauses() != null; + return !result.hasError() && !hasTransitiveError; + } + protected void syncPackages() throws Exception { skyframeExecutor.invalidateFilesUnderPathForTesting( reporter, ModifiedFileSet.EVERYTHING_MODIFIED, rootDirectory); @@ -100,11 +108,7 @@ public class IOExceptionsTest extends PackageLoadingTestCase { return null; } }; - assertFalse(visitor.sync(reporter, ImmutableSet.<Target>of(), - ImmutableSet.of(Label.parseAbsolute("//pkg:x")), /*keepGoing=*/false, - /*parallelThreads=*/5, Integer.MAX_VALUE)); - assertContainsEvent("no such package 'pkg'"); - assertEquals(1, eventCollector.count()); + assertFalse(visitTransitively(Label.parseAbsolute("//pkg:x"))); scratch.overwriteFile("pkg/BUILD", "# another comment to force reload", "sh_library(name = 'x')"); @@ -112,9 +116,7 @@ public class IOExceptionsTest extends PackageLoadingTestCase { syncPackages(); eventCollector.clear(); reporter.addHandler(failFastHandler); - assertTrue(visitor.sync(reporter, ImmutableSet.<Target>of(), - ImmutableSet.of(Label.parseAbsolute("//pkg:x")), /*keepGoing=*/false, - /*parallelThreads=*/5, Integer.MAX_VALUE)); + assertTrue(visitTransitively(Label.parseAbsolute("//pkg:x"))); assertNoEvents(); } @@ -135,9 +137,7 @@ public class IOExceptionsTest extends PackageLoadingTestCase { return null; } }; - assertFalse(visitor.sync(reporter, ImmutableSet.<Target>of(), - ImmutableSet.of(Label.parseAbsolute("//top:top")), /*keepGoing=*/false, - /*parallelThreads=*/5, Integer.MAX_VALUE)); + assertFalse(visitTransitively(Label.parseAbsolute("//top:top"))); assertContainsEvent("no such package 'pkg'"); // The traditional label visitor does not propagate the original IOException message. // assertContainsEvent("custom crash"); @@ -151,9 +151,7 @@ public class IOExceptionsTest extends PackageLoadingTestCase { syncPackages(); eventCollector.clear(); reporter.addHandler(failFastHandler); - assertTrue(visitor.sync(reporter, ImmutableSet.<Target>of(), - ImmutableSet.of(Label.parseAbsolute("//top:top")), /*keepGoing=*/false, - /*parallelThreads=*/5, Integer.MAX_VALUE)); + assertTrue(visitTransitively(Label.parseAbsolute("//top:top"))); assertNoEvents(); } @@ -172,10 +170,6 @@ public class IOExceptionsTest extends PackageLoadingTestCase { return null; } }; - assertFalse(visitor.sync(reporter, ImmutableSet.<Target>of(), - ImmutableSet.of(Label.parseAbsolute("//top/pkg:x")), /*keepGoing=*/false, - /*parallelThreads=*/5, Integer.MAX_VALUE)); - assertContainsEvent("no such package 'top/pkg'"); - assertEquals(1, eventCollector.count()); + assertFalse(visitTransitively(Label.parseAbsolute("//top/pkg:x"))); } } diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/PackageCacheTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/PackageCacheTest.java index fa22c66ea7..a7a5918c8c 100644 --- a/src/test/java/com/google/devtools/build/lib/pkgcache/PackageCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/pkgcache/PackageCacheTest.java @@ -40,7 +40,6 @@ import com.google.devtools.build.lib.packages.Package; import com.google.devtools.build.lib.packages.Preprocessor; import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.packages.Target; -import com.google.devtools.build.lib.packages.util.MockToolsConfig; import com.google.devtools.build.lib.skyframe.DiffAwareness; import com.google.devtools.build.lib.skyframe.PackageLookupFunction.CrossRepositoryLabelViolationStrategy; import com.google.devtools.build.lib.skyframe.PackageLookupValue.BuildFileName; @@ -59,9 +58,7 @@ import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.common.options.OptionsParser; import com.google.devtools.common.options.OptionsParsingException; import java.io.IOException; -import java.util.HashSet; import java.util.List; -import java.util.Set; import java.util.UUID; import org.junit.Before; import org.junit.Test; @@ -582,41 +579,6 @@ public class PackageCacheTest extends FoundationTestCase { assertThat(cc.getFeatures()).hasSize(1); } - /** Visit label and its dependencies and load all of them. */ - private void visitLabel(String label) throws Exception { - TransitivePackageLoader visitor = getPackageManager().newTransitiveLoader(); - Set<Target> targets = new HashSet<>(); - targets.add(getPackageManager().getTarget(reporter, Label.parseAbsolute(label))); - visitor.sync(reporter, targets, ImmutableSet.<Label>of(), - false, 1, Integer.MAX_VALUE); - } - - @Test - public void testSyntaxErrorInDepPackage() throws Exception { - reporter.removeHandler(failFastHandler); - AnalysisMock.get().setupMockClient(new MockToolsConfig(rootDirectory)); - - scratch.file("a/BUILD", - "genrule(name='x',", - " srcs = ['file.txt'],", - " outs = ['foo'],", - " cmd = 'echo')", - "@"); // syntax error - - scratch.file("b/BUILD", - "genrule(name= 'cc',", - " tools = ['//a:x'],", - " outs = ['bar'],", - " cmd = 'echo')"); - - Package pkgB = getPackage("b"); - - // We should get error message from package a, but package is properly loaded. - visitLabel("//b:cc"); - assertContainsEvent("invalid character: '@'"); - assertFalse(pkgB.containsErrors()); - } - @Test public void testBrokenPackageOnMultiplePackagePathEntries() throws Exception { reporter.removeHandler(failFastHandler); diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java index 9d0c8fb6c9..ad36b3c92a 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java @@ -45,7 +45,6 @@ import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.flags.InvocationPolicyEnforcer; import com.google.devtools.build.lib.packages.RuleClass; -import com.google.devtools.build.lib.packages.Target; import com.google.devtools.build.lib.rules.ToolchainLookup; import com.google.devtools.build.lib.testutil.MoreAsserts; import com.google.devtools.build.lib.util.FileType; @@ -558,12 +557,9 @@ public class CcCommonTest extends BuildViewTestCase { "cc_library(name = 'lib',", " srcs = ['foo.cc'],", " includes = ['./'])"); - Target target = getTarget("@pkg//bar:lib"); - ensureTargetsVisited(target.getLabel()); - assertThat( - view.hasErrors( - view.getConfiguredTargetForTesting(reporter, target.getLabel(), targetConfig))) - .isFalse(); + Label label = Label.parseAbsolute("@pkg//bar:lib"); + ConfiguredTarget target = view.getConfiguredTargetForTesting(reporter, label, targetConfig); + assertThat(view.hasErrors(target)).isFalse(); assertNoEvents(); } diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java index 47905d58a6..fb86314aaa 100644 --- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java @@ -1245,7 +1245,7 @@ public class SkylarkIntegrationTest extends BuildViewTestCase { ((PackageFunction) skyFunctions.get(SkyFunctions.PACKAGE)) .setSkylarkImportLookupFunctionForInliningForTesting(skylarkImportLookupFunction); } - + @Override @Test public void testRecursiveImport() throws Exception { @@ -1260,19 +1260,15 @@ public class SkylarkIntegrationTest extends BuildViewTestCase { reporter.removeHandler(failFastHandler); try { - // ensureTargetsVisited() produces a different event than getTarget, and it doesn't fail - // even though there is an error in the rule. What's going on here? - ensureTargetsVisited("//test/skylark:rule"); getTarget("//test/skylark:rule"); fail(); } catch (BuildFileContainsErrorsException e) { - // This is expected + // The reason that this is an exception and not reported to the event handler is that the + // error is reported by the parent sky function, which we don't have here. + assertThat(e.getMessage()).contains("Skylark import cycle"); + assertThat(e.getMessage()).contains("test/skylark:ext1.bzl"); + assertThat(e.getMessage()).contains("test/skylark:ext2.bzl"); } - assertContainsEvent("test/skylark:ext1.bzl"); - assertContainsEvent("test/skylark:ext2.bzl"); - assertContainsEvent("Skylark import cycle"); - assertContainsEvent("Loading of target '//test/skylark:rule' failed; build aborted"); - assertThat(eventCollector).hasSize(1); } @Override @@ -1290,18 +1286,16 @@ public class SkylarkIntegrationTest extends BuildViewTestCase { reporter.removeHandler(failFastHandler); try { - ensureTargetsVisited("//test/skylark:rule"); getTarget("//test/skylark:rule"); fail(); } catch (BuildFileContainsErrorsException e) { - // This is expected + // The reason that this is an exception and not reported to the event handler is that the + // error is reported by the parent sky function, which we don't have here. + assertThat(e.getMessage()).contains("Skylark import cycle"); + assertThat(e.getMessage()).contains("//test/skylark:ext2.bzl"); + assertThat(e.getMessage()).contains("//test/skylark:ext3.bzl"); + assertThat(e.getMessage()).contains("//test/skylark:ext4.bzl"); } - assertContainsEvent("//test/skylark:ext2.bzl"); - assertContainsEvent("//test/skylark:ext3.bzl"); - assertContainsEvent("//test/skylark:ext4.bzl"); - assertContainsEvent("Skylark import cycle"); - assertContainsEvent("Loading of target '//test/skylark:rule' failed; build aborted"); - assertThat(eventCollector).hasSize(1); } } |