From ef98ef99b2ff8d177891b1b093295b11913921d6 Mon Sep 17 00:00:00 2001 From: Googler Date: Fri, 15 Dec 2023 10:54:51 -0800 Subject: [PATCH] Modify the error message that occurs when a requested target does not exist. First, we removed the suggestion to "use `query`" as it was misleading to some users. Second, we fine-tuned the suggested target behaviors. The existing method retrieves only the closest target within a reasonable distance. We updated this to return potentially more targets, with a preference for those that are particularly close. Performance-wise, this should be not diverge much from the status quo. The existing behavior already checks edit-distance for each target in the package against the requested target. This adds minimal behavior on top of that to identify the most likely candidate(s). RELNOTES: None. PiperOrigin-RevId: 591302621 Change-Id: I9bd272b68def2f9a75db01061287697d23936bca --- .../google/devtools/build/lib/packages/BUILD | 2 - .../devtools/build/lib/packages/Package.java | 27 +--- .../build/lib/packages/TargetSuggester.java | 136 ++++++++++++++++++ .../java/net/starlark/java/spelling/BUILD | 4 +- .../build/lib/analysis/BuildViewTest.java | 3 +- .../build/lib/packages/ExportsFilesTest.java | 5 +- .../lib/packages/PackageFactoryTest.java | 11 +- .../lib/packages/TargetSuggesterTest.java | 100 +++++++++++++ .../google/devtools/build/lib/pkgcache/BUILD | 4 +- .../CompileOneDependencyTransformerTest.java | 7 +- .../lib/pkgcache/PackageLoadingTest.java | 7 +- .../query2/testutil/AbstractQueryTest.java | 18 ++- .../testutil/PostAnalysisQueryTest.java | 11 +- .../build/lib/testutil/TestUtils.java | 43 ++++-- 14 files changed, 300 insertions(+), 78 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/packages/TargetSuggester.java create mode 100644 src/test/java/com/google/devtools/build/lib/packages/TargetSuggesterTest.java diff --git a/src/main/java/com/google/devtools/build/lib/packages/BUILD b/src/main/java/com/google/devtools/build/lib/packages/BUILD index fe6369a6c785f7..24b258ad43e4d7 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/BUILD +++ b/src/main/java/com/google/devtools/build/lib/packages/BUILD @@ -68,8 +68,6 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_class_set", "//src/main/java/com/google/devtools/build/lib/analysis:config/toolchain_type_requirement", "//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/no_transition", - "//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/patch_transition", - "//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/split_transition", "//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/transition_factory", "//src/main/java/com/google/devtools/build/lib/analysis:rule_definition_environment", "//src/main/java/com/google/devtools/build/lib/analysis:transitive_info_provider", diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java index 34e72227f2499c..70c3739210cf61 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Package.java +++ b/src/main/java/com/google/devtools/build/lib/packages/Package.java @@ -17,7 +17,6 @@ import static com.google.common.base.MoreObjects.firstNonNull; import com.google.common.annotations.VisibleForTesting; -import com.google.common.base.Joiner; import com.google.common.base.Preconditions; import com.google.common.collect.BiMap; import com.google.common.collect.ImmutableList; @@ -81,7 +80,6 @@ import net.starlark.java.eval.Module; import net.starlark.java.eval.StarlarkSemantics; import net.starlark.java.eval.StarlarkThread; -import net.starlark.java.spelling.SpellChecker; import net.starlark.java.syntax.Location; /** @@ -188,13 +186,6 @@ public enum ConfigSettingVisibilityPolicy { private ImmutableMap> externalPackageRepositoryMappings; - /** - * The repository mapping of the main repository. This is only used internally to obtain - * user-friendly apparent names from canonical repository names in error message arising from this - * package. - */ - private RepositoryMapping mainRepositoryMapping; - /** * A rough approximation of the memory and general accounting costs associated with a loaded * package. A value of -1 means it is unset. Stored as a long to take up less memory per pkg. @@ -376,7 +367,6 @@ private void finishInit(Builder builder) { this.registeredToolchains = ImmutableList.copyOf(builder.registeredToolchains); this.firstWorkspaceSuffixRegisteredToolchain = builder.firstWorkspaceSuffixRegisteredToolchain; this.metadata.repositoryMapping = Preconditions.checkNotNull(builder.repositoryMapping); - this.mainRepositoryMapping = Preconditions.checkNotNull(builder.mainRepositoryMapping); ImmutableMap.Builder> repositoryMappingsBuilder = ImmutableMap.builder(); if (!builder.externalPackageRepositoryMappings.isEmpty() && !builder.isRepoRulePackage()) { @@ -686,15 +676,7 @@ private String getAlternateTargetSuggestion(String targetName) { + getName() + "/BUILD?)"; } else { - String suggestedTarget = SpellChecker.suggest(targetName, targets.keySet()); - String targetSuggestion = - suggestedTarget == null ? null : String.format("did you mean '%s'?", suggestedTarget); - String blazeQuerySuggestion = - String.format( - "Tip: use `query \"%s:*\"` to see all the targets in that package", - metadata.packageIdentifier.getDisplayForm(mainRepositoryMapping)); - return String.format( - " (%s)", Joiner.on(" ").skipNulls().join(targetSuggestion, blazeQuerySuggestion)); + return TargetSuggester.suggestTargets(targetName, targets.keySet()); } } @@ -889,12 +871,6 @@ default boolean precomputeTransitiveLoads() { */ private final RepositoryMapping repositoryMapping; - /** - * The repository mapping of the main repository. This is only used to resolve user-friendly - * apparent names from canonical repository names in error message arising from this package. - */ - private final RepositoryMapping mainRepositoryMapping; - /** Converts label literals to Label objects within this package. */ private final LabelConverter labelConverter; @@ -1043,7 +1019,6 @@ public T intern(T sample) { this.precomputeTransitiveLoads = packageSettings.precomputeTransitiveLoads(); this.noImplicitFileExport = noImplicitFileExport; this.repositoryMapping = repositoryMapping; - this.mainRepositoryMapping = mainRepositoryMapping; this.labelConverter = new LabelConverter(id, repositoryMapping); if (pkg.getName().startsWith("javatests/")) { mergePackageArgsFrom(PackageArgs.builder().setDefaultTestOnly(true)); diff --git a/src/main/java/com/google/devtools/build/lib/packages/TargetSuggester.java b/src/main/java/com/google/devtools/build/lib/packages/TargetSuggester.java new file mode 100644 index 00000000000000..c6698ef1437005 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/packages/TargetSuggester.java @@ -0,0 +1,136 @@ +// Copyright 2023 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.packages; + +import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Joiner; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableListMultimap; +import com.google.common.collect.Iterables; +import java.util.Locale; +import java.util.Set; +import net.starlark.java.spelling.SpellChecker; + +/** + * Some static utility functions for determining suggested targets when a user requests a + * non-existent target. + */ +public final class TargetSuggester { + private static final int MAX_SUGGESTED_TARGETS_SIZE = 10; + + private static final int MAX_SUGGESTION_EDIT_DISTANCE = 5; + + private TargetSuggester() {} + + /** + * Given a nonexistent target and the targets in its package, suggest what the user may have + * intended based on lexicographic closeness to the possibilities. + * + *

This will be pretty printed in the following form No suggested targets -> "". Suggested + * target "a" -> "a". Suggested targets "a", "b", "c" -> "a, b, or c". + */ + static String suggestTargets(String input, Set words) { + ImmutableList suggestedTargets = suggestedTargets(input, words); + return prettyPrintTargets(suggestedTargets); + } + + /** + * Given a requested target and a Set of targets in the same package, return a list of strings + * closest based on edit distance. + * + *

If any strings are identical minus capitalization changes, they will be returned. If any + * other strings are exactly 1 character off, they will be returned. Otherwise, the 10 nearest + * (within a small edit distance) will be returned. + */ + @VisibleForTesting + static ImmutableList suggestedTargets(String input, Set words) { + + final String lowerCaseInput = input.toLowerCase(Locale.US); + + // Add words based on edit distance + ImmutableListMultimap.Builder editDistancesBuilder = + ImmutableListMultimap.builder(); + + int maxEditDistance = Math.min(MAX_SUGGESTION_EDIT_DISTANCE, (input.length() + 1) / 2); + for (String word : words) { + String lowerCaseWord = word.toLowerCase(Locale.US); + + int editDistance = SpellChecker.editDistance(lowerCaseInput, lowerCaseWord, maxEditDistance); + + if (editDistance >= 0) { + editDistancesBuilder.put(editDistance, word); + } + } + ImmutableListMultimap editDistanceToWords = editDistancesBuilder.build(); + + ImmutableList zeroEditDistanceWords = editDistanceToWords.get(0); + ImmutableList oneEditDistanceWords = editDistanceToWords.get(1); + + if (editDistanceToWords.isEmpty()) { + return ImmutableList.of(); + } else if (!zeroEditDistanceWords.isEmpty()) { + int sublistLength = Math.min(zeroEditDistanceWords.size(), MAX_SUGGESTED_TARGETS_SIZE); + return ImmutableList.copyOf(zeroEditDistanceWords.subList(0, sublistLength)); + } else if (!oneEditDistanceWords.isEmpty()) { + int sublistLength = Math.min(oneEditDistanceWords.size(), MAX_SUGGESTED_TARGETS_SIZE); + return ImmutableList.copyOf(oneEditDistanceWords.subList(0, sublistLength)); + } else { + return getSuggestedTargets(editDistanceToWords, maxEditDistance); + } + } + + /** + * Given a map of edit distance values to words that are that distance from the requested target, + * returns up to MAX_SUGGESTED_TARGETS_SIZE targets that are at least edit distance 2 but no more + * than the given max away. + */ + private static ImmutableList getSuggestedTargets( + ImmutableListMultimap editDistanceToWords, int maxEditDistance) { + // iterate through until MAX is achieved + int total = 0; + ImmutableList.Builder suggestedTargets = ImmutableList.builder(); + for (int editDistance = 2; + editDistance < maxEditDistance && total < MAX_SUGGESTED_TARGETS_SIZE; + editDistance++) { + + ImmutableList values = editDistanceToWords.get(editDistance); + int addAmount = Math.min(values.size(), MAX_SUGGESTED_TARGETS_SIZE - total); + suggestedTargets.addAll(values.subList(0, addAmount)); + total += addAmount; + } + + return suggestedTargets.build(); + } + + /** + * Create a pretty-printable String for a list. Joiner doesn't currently support multiple + * separators so this is a custom roll for now. Returns a comma-delimited list with " or " before + * the last element. + */ + @VisibleForTesting + public static String prettyPrintTargets(ImmutableList targets) { + String targetString; + if (targets.isEmpty()) { + return ""; + } else if (targets.size() == 1) { + targetString = targets.get(0); + } else { + + String firstPart = Joiner.on(", ").join(targets.subList(0, targets.size() - 1)); + + targetString = Joiner.on(", or ").join(firstPart, Iterables.getLast(targets)); + } + return " (did you mean " + targetString + "?)"; + } +} diff --git a/src/main/java/net/starlark/java/spelling/BUILD b/src/main/java/net/starlark/java/spelling/BUILD index 85c4b581af62dd..cbd814eb9913f7 100644 --- a/src/main/java/net/starlark/java/spelling/BUILD +++ b/src/main/java/net/starlark/java/spelling/BUILD @@ -19,5 +19,7 @@ java_library( "//src/main/java/net/starlark/java:bazel", "//src/main/java/net/starlark/java:starlark", ], - deps = ["//third_party:jsr305"], + deps = [ + "//third_party:jsr305", + ], ) 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 e8058f8be573e1..6a9281c0acfffc 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 @@ -657,8 +657,7 @@ public void testHelpfulErrorForWrongPackageLabels() throws Exception { assertThat(result.hasError()).isTrue(); assertContainsEvent( "no such target '//x:z': target 'z' not declared in package 'x' defined by" - + " /workspace/x/BUILD (Tip: use `query \"//x:*\"` to see all the targets in that" - + " package) and referenced by '//y:y'"); + + " /workspace/x/BUILD"); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/packages/ExportsFilesTest.java b/src/test/java/com/google/devtools/build/lib/packages/ExportsFilesTest.java index 34c08ba5ce9b59..fee8006a28ecfc 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/ExportsFilesTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/ExportsFilesTest.java @@ -59,10 +59,7 @@ public void testFileThatsNotRegisteredYieldsUnknownTargetException() throws Exce assertThrows(NoSuchTargetException.class, () -> pkg().getTarget("baz.txt")); assertThat(e) .hasMessageThat() - .isEqualTo( - "no such target '//pkg:baz.txt': target 'baz.txt' not declared in package 'pkg' " - + "defined by /workspace/pkg/BUILD (did you mean 'bar.txt'? Tip: use `query " - + "\"//pkg:*\"` to see all the targets in that package)"); + .contains("no such target '//pkg:baz.txt': target 'baz.txt' not declared in package 'pkg'"); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java index 82de1b5d01980a..044b13f49859f2 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java @@ -303,10 +303,7 @@ public void testCreationOfInputFiles() throws Exception { NoSuchTargetException e = assertThrows(NoSuchTargetException.class, () -> pkg.getTarget("B")); assertThat(e) .hasMessageThat() - .isEqualTo( - "no such target '//foo:B': target 'B' not declared in package 'foo' defined by" - + " /workspace/foo/BUILD (Tip: use `query \"//foo:*\"` to see all the targets in" - + " that package)"); + .contains("no such target '//foo:B': target 'B' not declared in package 'foo'"); // These are the only input files: BUILD, Z Set inputFiles = Sets.newTreeSet(); @@ -443,9 +440,9 @@ public void testHelpfulErrorForMissingExportsFiles() throws Exception { assertThat(e) .hasMessageThat() .isEqualTo( - "no such target '//x:z.cc': target 'z.cc' not declared in package 'x' defined by" - + " /workspace/x/BUILD (did you mean 'x.cc'? Tip: use `query \"//x:*\"` to see all" - + " the targets in that package)"); + "no such target '//x:z.cc': " + + "target 'z.cc' not declared in package 'x' " + + "defined by /workspace/x/BUILD (did you mean x.cc?)"); e = assertThrows(NoSuchTargetException.class, () -> pkg.getTarget("dir")); assertThat(e) diff --git a/src/test/java/com/google/devtools/build/lib/packages/TargetSuggesterTest.java b/src/test/java/com/google/devtools/build/lib/packages/TargetSuggesterTest.java new file mode 100644 index 00000000000000..79be4f6b504a0f --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/packages/TargetSuggesterTest.java @@ -0,0 +1,100 @@ +// Copyright 2023 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.packages; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.common.collect.ImmutableList; +import java.util.HashSet; +import java.util.Set; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class TargetSuggesterTest { + + @Test + public void testRangeDoesntSuggestTarget() { + String requestedTarget = "range"; + Set packageTargets = new HashSet<>(); + packageTargets.add("target"); + + ImmutableList suggestedTargets = + TargetSuggester.suggestedTargets(requestedTarget, packageTargets); + assertThat(suggestedTargets).isEmpty(); + } + + @Test + public void testMisspelledTargetRetrievesProperSuggestion() { + String misspelledTarget = "AnrdiodTest"; + + Set packageTargets = new HashSet<>(); + packageTargets.add("AndroidTest"); + packageTargets.add("AndroidTest_deploy"); + packageTargets.add("AndroidTest_java"); + + ImmutableList suggestedTargets = + TargetSuggester.suggestedTargets(misspelledTarget, packageTargets); + assertThat(suggestedTargets).containsExactly("AndroidTest"); + } + + @Test + public void testRetrieveMultipleTargets() { + String misspelledTarget = "pixel_2_test"; + + Set packageTargets = new HashSet<>(); + packageTargets.add("pixel_5_test"); + packageTargets.add("pixel_6_test"); + packageTargets.add("android_2_test"); + + ImmutableList suggestedTargets = + TargetSuggester.suggestedTargets(misspelledTarget, packageTargets); + assertThat(suggestedTargets).containsExactly("pixel_5_test", "pixel_6_test"); + } + + @Test + public void testOnlyClosestTargetIsReturned() { + String misspelledTarget = "Pixel_5_test"; + + Set packageTargets = new HashSet<>(); + packageTargets.add("pixel_5_test"); + packageTargets.add("pixel_6_test"); + packageTargets.add("android_2_test"); + + ImmutableList suggestedTargets = + TargetSuggester.suggestedTargets(misspelledTarget, packageTargets); + assertThat(suggestedTargets).containsExactly("pixel_5_test"); + } + + @Test + public void prettyPrintEmpty() { + String empty = TargetSuggester.prettyPrintTargets(ImmutableList.of()); + assertThat(empty).isEmpty(); + } + + @Test + public void prettyPrintSingleTarget_returnsSingleTarget() { + ImmutableList targets = ImmutableList.of("pixel_5_test"); + String targetString = TargetSuggester.prettyPrintTargets(targets); + assertThat(targetString).isEqualTo(" (did you mean pixel_5_test?)"); + } + + @Test + public void prettyPrintMultipleTargets_returnsMultipleTargets() { + ImmutableList targets = ImmutableList.of("pixel_5_test", "pixel_6_test"); + String targetString = TargetSuggester.prettyPrintTargets(targets); + assertThat(targetString).isEqualTo(" (did you mean pixel_5_test, or pixel_6_test?)"); + } +} diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/BUILD b/src/test/java/com/google/devtools/build/lib/pkgcache/BUILD index b8320a75a10961..486cb6eff657c5 100644 --- a/src/test/java/com/google/devtools/build/lib/pkgcache/BUILD +++ b/src/test/java/com/google/devtools/build/lib/pkgcache/BUILD @@ -88,6 +88,7 @@ java_test( "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", "//src/test/java/com/google/devtools/build/lib/packages:testutil", + "//src/test/java/com/google/devtools/build/lib/testutil:TestUtils", "//third_party:guava", "//third_party:junit4", "//third_party:truth", @@ -173,8 +174,6 @@ java_test( "//src/main/java/com/google/devtools/build/lib/packages", "//src/main/java/com/google/devtools/build/lib/packages/semantics", "//src/main/java/com/google/devtools/build/lib/pkgcache", - "//src/main/java/com/google/devtools/build/lib/rules:repository/repository_function", - "//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value", "//src/main/java/com/google/devtools/build/lib/skyframe:skyframe_cluster", "//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception", "//src/main/java/com/google/devtools/build/lib/util/io", @@ -186,6 +185,7 @@ java_test( "//src/test/java/com/google/devtools/build/lib/testutil", "//src/test/java/com/google/devtools/build/lib/testutil:JunitUtils", "//src/test/java/com/google/devtools/build/lib/testutil:SkyframeExecutorTestHelper", + "//src/test/java/com/google/devtools/build/lib/testutil:TestUtils", "//third_party:guava", "//third_party:junit4", "//third_party:truth", diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/CompileOneDependencyTransformerTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/CompileOneDependencyTransformerTest.java index 5ab9d27baf5847..130358b6283b49 100644 --- a/src/test/java/com/google/devtools/build/lib/pkgcache/CompileOneDependencyTransformerTest.java +++ b/src/test/java/com/google/devtools/build/lib/pkgcache/CompileOneDependencyTransformerTest.java @@ -27,6 +27,7 @@ import com.google.devtools.build.lib.packages.util.PackageLoadingTestCase; import com.google.devtools.build.lib.rules.repository.RepositoryDelegatorFunction; import com.google.devtools.build.lib.skyframe.PrecomputedValue; +import com.google.devtools.build.lib.testutil.TestUtils; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import java.io.IOException; @@ -158,10 +159,8 @@ public void testCompileOneDepOnMissingFile() throws Exception { assertThrows(TargetParsingException.class, () -> parseCompileOneDep("//foo:missing.cc")); assertThat(e) .hasMessageThat() - .isEqualTo( - "no such target '//foo:missing.cc': target 'missing.cc' not declared in package " - + "'foo' defined by /workspace/foo/BUILD (Tip: use `query \"//foo:*\"` to see all " - + "the targets in that package)"); + .matches( + TestUtils.createMissingTargetAssertionString("missing.cc", "foo", "/workspace", "")); // Also, try a valid input file which has no dependent rules in its package. e = assertThrows(TargetParsingException.class, () -> parseCompileOneDep("//foo:baz/bang")); diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/PackageLoadingTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/PackageLoadingTest.java index 3c0c67efd43775..3efd95ca64dc09 100644 --- a/src/test/java/com/google/devtools/build/lib/pkgcache/PackageLoadingTest.java +++ b/src/test/java/com/google/devtools/build/lib/pkgcache/PackageLoadingTest.java @@ -45,6 +45,7 @@ import com.google.devtools.build.lib.testutil.FoundationTestCase; import com.google.devtools.build.lib.testutil.MoreAsserts; import com.google.devtools.build.lib.testutil.SkyframeExecutorTestHelper; +import com.google.devtools.build.lib.testutil.TestUtils; import com.google.devtools.build.lib.util.AbruptExitException; import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor; import com.google.devtools.build.lib.vfs.ModifiedFileSet; @@ -232,10 +233,8 @@ public void testGetNonexistentTarget() throws Exception { assertThrows(NoSuchTargetException.class, () -> getTarget("//pkg1:not-there")); assertThat(e) .hasMessageThat() - .isEqualTo( - "no such target '//pkg1:not-there': target 'not-there' " - + "not declared in package 'pkg1' defined by /workspace/pkg1/BUILD (Tip: use " - + "`query \"//pkg1:*\"` to see all the targets in that package)"); + .matches( + TestUtils.createMissingTargetAssertionString("not-there", "pkg1", "/workspace", "")); } /** diff --git a/src/test/java/com/google/devtools/build/lib/query2/testutil/AbstractQueryTest.java b/src/test/java/com/google/devtools/build/lib/query2/testutil/AbstractQueryTest.java index 1ad7c550f5a8bd..1175516b4c5812 100644 --- a/src/test/java/com/google/devtools/build/lib/query2/testutil/AbstractQueryTest.java +++ b/src/test/java/com/google/devtools/build/lib/query2/testutil/AbstractQueryTest.java @@ -63,6 +63,7 @@ import com.google.devtools.build.lib.server.FailureDetails.TargetPatterns; import com.google.devtools.build.lib.testutil.TestConstants; import com.google.devtools.build.lib.testutil.TestRuleClassProvider; +import com.google.devtools.build.lib.testutil.TestUtils; import com.google.devtools.build.lib.util.AbruptExitException; import com.google.devtools.build.lib.util.FileTypeSet; import com.google.devtools.build.lib.vfs.Path; @@ -72,7 +73,6 @@ import java.io.OutputStreamWriter; import java.io.PrintWriter; import java.util.Collections; -import java.util.List; import java.util.Set; import org.junit.After; import org.junit.Before; @@ -283,11 +283,9 @@ public void testTargetLiteralWithMissingTargets() throws Exception { writeFile("a/BUILD"); EvalThrowsResult evalThrowsResult = evalThrows("//a:b", false); assertThat(evalThrowsResult.getMessage()) - .isEqualTo( - "no such target '//a:b': target 'b' not declared in package 'a' " - + "defined by " - + helper.getRootDirectory().getPathString() - + "/a/BUILD (Tip: use `query \"//a:*\"` to see all the targets in that package)"); + .matches( + TestUtils.createMissingTargetAssertionString( + "b", "a", helper.getRootDirectory().getPathString(), "")); assertThat(evalThrowsResult.getFailureDetail().getPackageLoading().getCode()) .isEqualTo(FailureDetails.PackageLoading.Code.TARGET_MISSING); } @@ -635,10 +633,10 @@ public void testSomePathOperatorOrdering() throws Exception { writeFile("c/BUILD", "genrule(name='c', srcs=['//d'], outs=['out'], cmd=':')"); writeFile("d/BUILD", "exports_files(['d'])"); - List pathList1 = ImmutableList.of("//a:a0", "//a:a1", "//b:b", "//d:d"); - List pathList2 = ImmutableList.of("//a:a0", "//a:a1", "//c:c", "//d:d"); + ImmutableList pathList1 = ImmutableList.of("//a:a0", "//a:a1", "//b:b", "//d:d"); + ImmutableList pathList2 = ImmutableList.of("//a:a0", "//a:a1", "//c:c", "//d:d"); - List somepathAToD = evalToListOfStrings("somepath(//a:a0, //d)"); + ImmutableList somepathAToD = evalToListOfStrings("somepath(//a:a0, //d)"); if (somepathAToD.contains("//b:b")) { assertThat(pathList1).isEqualTo(somepathAToD); } else { @@ -750,7 +748,7 @@ public void testDepsDoesNotIncludeBuildFiles() throws Exception { " srcs = [':dep1.txt', ':dep2.txt'],", " cmd = 'echo $(SRCS) >$@')"); - List result = evalToListOfStrings("deps(//s:my_rule)"); + ImmutableList result = evalToListOfStrings("deps(//s:my_rule)"); assertThat(result).containsAtLeast("//s:dep2", "//s:dep1.txt", "//s:dep2.txt", "//s:my_rule"); assertThat(result) .containsNoneOf("//deps:BUILD", "//deps:build_def", "//deps:starlark.bzl", "//s:BUILD"); diff --git a/src/test/java/com/google/devtools/build/lib/query2/testutil/PostAnalysisQueryTest.java b/src/test/java/com/google/devtools/build/lib/query2/testutil/PostAnalysisQueryTest.java index 748cb5f9cb495d..ab434840e0efda 100644 --- a/src/test/java/com/google/devtools/build/lib/query2/testutil/PostAnalysisQueryTest.java +++ b/src/test/java/com/google/devtools/build/lib/query2/testutil/PostAnalysisQueryTest.java @@ -155,11 +155,12 @@ public void testTargetLiteralWithMissingTargets() { assertThrows(TargetParsingException.class, super::testTargetLiteralWithMissingTargets); assertThat(e) .hasMessageThat() - .isEqualTo( - "no such target '//a:b': target 'b' not declared in package 'a' " - + "defined by " - + helper.getRootDirectory().getPathString() - + "/a/BUILD (Tip: use `query \"//a:*\"` to see all the targets in that package)"); + .matches( + TestUtils.createMissingTargetAssertionString( + /* target= */ "b", + /* packageStr= */ "a", + helper.getRootDirectory().getPathString(), + "")); assertThat(e.getDetailedExitCode().getFailureDetail().getPackageLoading().getCode()) .isEqualTo(FailureDetails.PackageLoading.Code.TARGET_MISSING); } diff --git a/src/test/java/com/google/devtools/build/lib/testutil/TestUtils.java b/src/test/java/com/google/devtools/build/lib/testutil/TestUtils.java index cec002aca6fe04..76a2ff25171322 100644 --- a/src/test/java/com/google/devtools/build/lib/testutil/TestUtils.java +++ b/src/test/java/com/google/devtools/build/lib/testutil/TestUtils.java @@ -29,10 +29,8 @@ import java.util.UUID; import javax.annotation.Nullable; -/** - * Some static utility functions for testing. - */ -public class TestUtils { +/** Some static utility functions for testing. */ +public final class TestUtils { public static final UUID ZERO_UUID = UUID.fromString("00000000-0000-0000-0000-000000000000"); @@ -154,20 +152,43 @@ public void clear() {} }; } + /** Creates the assertion String to match against when a target isn't found. */ + public static String createMissingTargetAssertionString( + String target, String packageStr, String workspaceRoot, String expectedTargets) { + if (workspaceRoot == null) { + workspaceRoot = ""; + } + + String buildFilePath = workspaceRoot + "/" + packageStr + "/BUILD"; + + String fullTarget = "//" + packageStr + ":" + target; + + final String suggestedTargetsBaseString = + "no such target '%s': target '%s' not declared in package '%s' " + + "defined by %s" + + expectedTargets; + + return String.format( + suggestedTargetsBaseString, fullTarget, target, packageStr, buildFilePath, expectedTargets); + } + /** * Timeouts for asserting that an arbitrary event occurs eventually. * *

In general, it's not appropriate to use a small constant timeout for an arbitrary * computation since there is no guarantee that a snippet of code will execute within a given - * amount of time - you are at the mercy of the jvm, your machine, and your OS. In theory we - * could try to take all of these factors into account but instead we took the simpler and - * obviously correct approach of not having timeouts. + * amount of time - you are at the mercy of the jvm, your machine, and your OS. In theory we could + * try to take all of these factors into account but instead we took the simpler and obviously + * correct approach of not having timeouts. * - *

If a test that uses these timeout values is failing due to a "timeout" at the - * 'blaze test' level, it could be because of a legitimate deadlock that would have been caught - * if the timeout values below were small. So you can rule out such a deadlock by changing these - * values to small numbers (also note that the --test_timeout blaze flag may be useful). + *

If a test that uses these timeout values is failing due to a "timeout" at the 'blaze test' + * level, it could be because of a legitimate deadlock that would have been caught if the timeout + * values below were small. So you can rule out such a deadlock by changing these values to small + * numbers (also note that the --test_timeout blaze flag may be useful). */ public static final long WAIT_TIMEOUT_MILLISECONDS = Long.MAX_VALUE; + public static final long WAIT_TIMEOUT_SECONDS = WAIT_TIMEOUT_MILLISECONDS / 1000; + + private TestUtils() {} }