From b11d760369b3ad14c32f25dbd7a56f6b5c826733 Mon Sep 17 00:00:00 2001 From: Benjamin Peterson Date: Wed, 7 Mar 2018 00:19:00 -0800 Subject: Post PatternExpandingError from the skyframe target pattern evaluator as needed. This fixes https://github.com/bazelbuild/bazel/issues/4731. Also, add test coverage for posting of ParsingFailedEvent and handling of cycles in LoadingPhaseRunnerTest. Change-Id: I88c9d33417b9c3c7a06c92a6137d58f37b991b0c PiperOrigin-RevId: 188138972 --- .../build/lib/pkgcache/LoadingPhaseRunnerTest.java | 83 ++++++++++++++++++---- 1 file changed, 70 insertions(+), 13 deletions(-) (limited to 'src/test') diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java index cf7c61bba0..de8523eecf 100644 --- a/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java @@ -24,6 +24,8 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; +import com.google.common.collect.Iterators; +import com.google.common.collect.MoreCollectors; import com.google.devtools.build.lib.actions.ActionKeyContext; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.BuildView; @@ -43,6 +45,7 @@ import com.google.devtools.build.lib.packages.Target; import com.google.devtools.build.lib.packages.util.MockToolsConfig; import com.google.devtools.build.lib.skyframe.BazelSkyframeExecutorConstants; import com.google.devtools.build.lib.skyframe.DiffAwareness; +import com.google.devtools.build.lib.skyframe.PatternExpandingError; import com.google.devtools.build.lib.skyframe.SequencedSkyframeExecutor; import com.google.devtools.build.lib.skyframe.SkyValueDirtinessChecker; import com.google.devtools.build.lib.skyframe.SkyframeExecutor; @@ -133,6 +136,9 @@ public class LoadingPhaseRunnerTest { assertThat(loadingResult.getTestsToRun()).isNull(); tester.assertContainsError("Skipping '//base:missing': no such package 'base'"); tester.assertContainsWarning("Target pattern parsing failed."); + PatternExpandingError err = tester.findPostOnce(PatternExpandingError.class); + assertThat(err.getPattern()).containsExactly("//base:missing"); + assertThat(err.getSkipped()).isTrue(); } @Test @@ -145,6 +151,9 @@ public class LoadingPhaseRunnerTest { assertThat(loadingResult.getTestsToRun()).isNull(); tester.assertContainsError("Skipping '//base:missing': no such target '//base:missing'"); tester.assertContainsWarning("Target pattern parsing failed."); + PatternExpandingError err = tester.findPostOnce(PatternExpandingError.class); + assertThat(err.getPattern()).containsExactly("//base:missing"); + assertThat(err.getSkipped()).isTrue(); } @Test @@ -158,6 +167,8 @@ public class LoadingPhaseRunnerTest { + "invalid package name 'foo//bar': " + "package names may not contain '//' path separators"); } + ParsingFailedEvent err = tester.findPostOnce(ParsingFailedEvent.class); + assertThat(err.getPattern()).isEqualTo("foo//bar:missing"); } @Test @@ -182,6 +193,8 @@ public class LoadingPhaseRunnerTest { "invalid target format 'foo//bar:missing': " + "invalid package name 'foo//bar': " + "package names may not contain '//' path separators"); + ParsingFailedEvent err = tester.findPostOnce(ParsingFailedEvent.class); + assertThat(err.getPattern()).isEqualTo("foo//bar:missing"); } @Test @@ -540,6 +553,9 @@ public class LoadingPhaseRunnerTest { } catch (TargetParsingException expected) { } tester.assertContainsEventWithFrequency("name 'undefined_symbol' is not defined", 1); + PatternExpandingError err = tester.findPostOnce(PatternExpandingError.class); + assertThat(err.getPattern()).containsExactly("//bad"); + assertThat(err.getSkipped()).isFalse(); } @Test @@ -607,11 +623,50 @@ public class LoadingPhaseRunnerTest { public void testParsingFailureReported() throws Exception { LoadingResult loadingResult = tester.loadKeepGoing("//does_not_exist"); assertThat(loadingResult.hasTargetPatternError()).isTrue(); - ParsingFailedEvent event = tester.findPost(ParsingFailedEvent.class); - assertThat(event).isNotNull(); + ParsingFailedEvent event = tester.findPostOnce(ParsingFailedEvent.class); assertThat(event.getPattern()).isEqualTo("//does_not_exist"); assertThat(event.getMessage()).contains("BUILD file not found on package path"); - assertThat(Iterables.filter(tester.getPosts(), ParsingFailedEvent.class)).hasSize(1); + } + + @Test + public void testCyclesKeepGoing() throws Exception { + tester.addFile("test/BUILD", "load(':cycle1.bzl', 'make_cycle')"); + tester.addFile("test/cycle1.bzl", "load(':cycle2.bzl', 'make_cycle')"); + tester.addFile("test/cycle2.bzl", "load(':cycle1.bzl', 'make_cycle')"); + if (useSkyframeTargetPatternEval()) { + // The skyframe target pattern evaluator isn't able to provide partial results in the presence + // of cycles, so it simply raises an exception rather than returning an empty LoadingResult. + try { + tester.load("//test:cycle1"); + fail(); + } catch (TargetParsingException e) { + assertThat(e).hasMessageThat().contains("cycles detected"); + } + } else { + LoadingResult loadingResult = tester.loadKeepGoing("//test:cycle1"); + assertThat(loadingResult.hasTargetPatternError()).isTrue(); + } + tester.assertContainsEventWithFrequency("cycle detected in extension", 1); + PatternExpandingError err = tester.findPostOnce(PatternExpandingError.class); + assertThat(err.getPattern()).containsExactly("//test:cycle1"); + assertThat(err.getSkipped()).isEqualTo(!useSkyframeTargetPatternEval()); + } + + @Test + public void testCyclesNoKeepGoing() throws Exception { + tester.addFile("test/BUILD", "load(':cycle1.bzl', 'make_cycle')"); + tester.addFile("test/cycle1.bzl", "load(':cycle2.bzl', 'make_cycle')"); + tester.addFile("test/cycle2.bzl", "load(':cycle1.bzl', 'make_cycle')"); + try { + tester.load("//test:cycle1"); + fail(); + } catch (TargetParsingException e) { + assertThat(e).hasMessageThat().contains("cycles detected"); + } + tester.assertContainsEventWithFrequency("cycle detected in extension", 1); + PatternExpandingError err = tester.findPostOnce(PatternExpandingError.class); + assertThat(err.getPattern()).containsExactly("//test:cycle1"); + assertThat(err.getSkipped()).isFalse(); } private void assertCircularSymlinksDuringTargetParsing(String targetPattern) throws Exception { @@ -870,17 +925,19 @@ public class LoadingPhaseRunnerTest { filteredEvents(), expectedMessage, expectedFrequency); } - public Iterable getPosts() { - return storedErrors.getPosts(); - } - public T findPost(Class clazz) { - for (Postable p : storedErrors.getPosts()) { - if (clazz.isInstance(p)) { - return clazz.cast(p); - } - } - return null; + return Iterators.getNext( + storedErrors.getPosts().stream().filter(clazz::isInstance).map(clazz::cast).iterator(), + null); + } + + public T findPostOnce(Class clazz) { + return storedErrors + .getPosts() + .stream() + .filter(clazz::isInstance) + .map(clazz::cast) + .collect(MoreCollectors.onlyElement()); } } } -- cgit v1.2.3