diff options
author | vladmos <vladmos@google.com> | 2017-07-05 10:25:01 -0400 |
---|---|---|
committer | John Cater <jcater@google.com> | 2017-07-05 10:59:40 -0400 |
commit | 6ff634d3f3582c74190a5dd5051a4b0253aec604 (patch) | |
tree | 0f3756c6b63539c17c409b5d8893c447b015017a /src/test/java/com/google/devtools | |
parent | fd04ce8e20c62acc357a9473dcde727a413e915e (diff) |
Clean up string representations for labels
If --incompatible_descriptive_string_representations is passed, labels are converted
to strings using `repr` differently: `Label("//package:name")` instead of
`"//package:name"`
This CL doesn't affect representations of other object types but provides the
necessary infrastructure for it.
PiperOrigin-RevId: 160955284
Diffstat (limited to 'src/test/java/com/google/devtools')
7 files changed, 379 insertions, 19 deletions
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 4f4126e455..833cd9c6d3 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 @@ -168,7 +168,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase { protected ConfigurationFactory configurationFactory; protected BuildView view; - private SequencedSkyframeExecutor skyframeExecutor; + protected SequencedSkyframeExecutor skyframeExecutor; protected TimestampGranularityMonitor tsgm; protected BlazeDirectories directories; diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java index 5dde93b421..860f1d702f 100644 --- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java +++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java @@ -509,7 +509,7 @@ public class SkylarkRuleContextTest extends SkylarkTestCase { "macro()", "cc_library(name = 'a', srcs = [])"); getConfiguredTarget("//test:a"); - assertContainsEvent("selector({\"//foo:foo\": [\"//bar:bar\"]})"); + assertContainsEvent("selector({Label(\"//foo:foo\"): [Label(\"//bar:bar\")]})"); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkStringRepresentationsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkStringRepresentationsTest.java new file mode 100644 index 0000000000..7c777e28f5 --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkStringRepresentationsTest.java @@ -0,0 +1,303 @@ +// Copyright 2017 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.skylark; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.common.collect.ImmutableList; +import com.google.devtools.build.lib.analysis.ConfiguredTarget; +import com.google.devtools.build.lib.skylark.util.SkylarkTestCase; +import com.google.devtools.build.lib.util.Pair; +import com.google.devtools.build.lib.vfs.ModifiedFileSet; +import com.google.devtools.build.lib.vfs.PathFragment; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests for string representations of Skylark objects. */ +@RunWith(JUnit4.class) +public class SkylarkStringRepresentationsTest extends SkylarkTestCase { + + // Different ways to format objects, these suffixes are used in the `prepare_params` function + private static final ImmutableList<String> SUFFIXES = + ImmutableList.of("_str", "_repr", "_format", "_str_perc", "_repr_perc"); + + private Object skylarkLoadingEval(String code) throws Exception { + return skylarkLoadingEval(code, ""); + } + + /** + * Evaluates {@code code} in the loading phase in a .bzl file + * + * @param code The code to execute + * @param definition Additional code to define necessary variables + */ + private Object skylarkLoadingEval(String code, String definition) throws Exception { + scratch.overwriteFile("eval/BUILD", "load(':eval.bzl', 'eval')", "eval(name='eval')"); + scratch.overwriteFile( + "eval/eval.bzl", + definition, + String.format("x = %s", code), // Should be placed here to execute during the loading phase + "def _impl(ctx):", + " return struct(result = x)", + "eval = rule(implementation = _impl)"); + skyframeExecutor.invalidateFilesUnderPathForTesting( + reporter, + new ModifiedFileSet.Builder() + .modify(PathFragment.create("eval/BUILD")) + .modify(PathFragment.create("eval/eval.bzl")) + .build(), + rootDirectory); + + ConfiguredTarget target = getConfiguredTarget("//eval"); + return target.get("result"); + } + + /** + * Asserts that all 5 different ways to convert an object to a string of {@code expression} + * ({@code str}, {@code repr}, {@code '%s'}, {@code '%r'}, {@code '{}'.format} return the correct + * {@code representation}. Not applicable for objects that have different {@code str} and {@code + * repr} representations. + * + * @param definition optional definition required to evaluate the {@code expression} + * @param expression the expression to evaluate a string representation of + * @param representation desired string representation + */ + private void assertStringRepresentation( + String definition, String expression, String representation) throws Exception { + assertThat(skylarkLoadingEval(String.format("str(%s)", expression), definition)) + .isEqualTo(representation); + assertThat(skylarkLoadingEval(String.format("repr(%s)", expression), definition)) + .isEqualTo(representation); + assertThat(skylarkLoadingEval(String.format("'%%s' %% (%s,)", expression), definition)) + .isEqualTo(representation); + assertThat(skylarkLoadingEval(String.format("'%%r' %% (%s,)", expression), definition)) + .isEqualTo(representation); + assertThat(skylarkLoadingEval(String.format("'{}'.format(%s)", expression), definition)) + .isEqualTo(representation); + } + + private void assertStringRepresentation(String expression, String representation) + throws Exception { + assertStringRepresentation("", expression, representation); + } + + /** + * Creates a set of BUILD and .bzl files that gathers objects of many different types available in + * Skylark and creates their string representations by calling `str` and `repr` on them. The + * strings are available in the configured target for //test/skylark:check + */ + private void generateFilesToTestStrings() throws Exception { + // Generate string representations of different Skylark types. Objects are generated in + // test/skylark/rules.bzl: the top-level objects dict contains objects + // available during the loading phase, and _check_impl(ctx) returns objects that are available + // during the analysis phase. prepare_params(objects) converts a list of objects to a list of + // their string representations. + + scratch.file( + "test/skylark/rules.bzl", + "aspect_ctx_provider = provider()", + "def prepare_params(objects):", + " params = {}", + " for k, v in objects.items():", + " params[k + '_str'] = str(v)", + " params[k + '_repr'] = repr(v)", + " params[k + '_format'] = '{}'.format(v)", + " params[k + '_str_perc'] = '%s' % (v,)", + " params[k + '_repr_perc'] = '%r' % (v,)", + " return params", + "", + "def _impl_aspect(target, ctx):", + " return [aspect_ctx_provider(ctx = ctx)]", + "my_aspect = aspect(implementation = _impl_aspect)", + "", + "def _impl(ctx): pass", + "dep = rule(implementation = _impl)", + "", + "def _genfile_impl(ctx):", + " ctx.file_action(output = ctx.outputs.my_output, content = 'foo')", + "genfile = rule(", + " implementation = _genfile_impl,", + " outputs = {", + " 'my_output': '%{name}.txt',", + " },", + ")", + "", + "def _check_impl(ctx):", + " objects = {", + " 'target': ctx.attr.deps[0],", + " 'alias_target': ctx.attr.deps[1],", + " 'aspect_target': ctx.attr.asp_deps[0],", + " 'input_target': ctx.attr.srcs[0],", + " 'output_target': ctx.attr.srcs[1],", + " 'rule_ctx': ctx,", + " 'aspect_ctx': ctx.attr.asp_deps[0][aspect_ctx_provider].ctx,", + " }", + " return struct(**prepare_params(objects))", + "check = rule(", + " implementation = _check_impl,", + " attrs = {", + " 'deps': attr.label_list(),", + " 'asp_deps': attr.label_list(aspects = [my_aspect]),", + " 'srcs': attr.label_list(allow_files = True),", + " },", + ")"); + + scratch.file( + "test/skylark/BUILD", + "load(':rules.bzl', 'check', 'dep', 'genfile')", + "", + "dep(name = 'foo')", + "dep(name = 'bar')", + "alias(name = 'foobar', actual = ':foo')", + "genfile(name = 'output')", + "check(", + " name = 'check',", + " deps = [':foo', ':foobar'],", + " asp_deps = [':bar'],", + " srcs = ['input.txt', 'output.txt'],", + ")"); + } + + @Test + public void testStringRepresentations_Strings() throws Exception { + setSkylarkSemanticsOptions("--incompatible_descriptive_string_representations=true"); + + assertThat(skylarkLoadingEval("str('foo')")).isEqualTo("foo"); + assertThat(skylarkLoadingEval("'%s' % 'foo'")).isEqualTo("foo"); + assertThat(skylarkLoadingEval("'{}'.format('foo')")).isEqualTo("foo"); + assertThat(skylarkLoadingEval("repr('foo')")).isEqualTo("\"foo\""); + assertThat(skylarkLoadingEval("'%r' % 'foo'")).isEqualTo("\"foo\""); + } + + @Test + public void testStringRepresentations_Labels() throws Exception { + setSkylarkSemanticsOptions("--incompatible_descriptive_string_representations=true"); + + assertThat(skylarkLoadingEval("str(Label('//foo:bar'))")).isEqualTo("//foo:bar"); + assertThat(skylarkLoadingEval("'%s' % Label('//foo:bar')")).isEqualTo("//foo:bar"); + assertThat(skylarkLoadingEval("'{}'.format(Label('//foo:bar'))")).isEqualTo("//foo:bar"); + assertThat(skylarkLoadingEval("repr(Label('//foo:bar'))")).isEqualTo("Label(\"//foo:bar\")"); + assertThat(skylarkLoadingEval("'%r' % Label('//foo:bar')")).isEqualTo("Label(\"//foo:bar\")"); + + assertThat(skylarkLoadingEval("'{}'.format([Label('//foo:bar')])")).isEqualTo("[Label(\"//foo:bar\")]"); + } + + @Test + public void testStringRepresentations_Primitives() throws Exception { + // Strings are tested in a separate test case as they have different str and repr values. + setSkylarkSemanticsOptions("--incompatible_descriptive_string_representations=true"); + + assertStringRepresentation("1543", "1543"); + assertStringRepresentation("True", "True"); + assertStringRepresentation("False", "False"); + } + + @Test + public void testStringRepresentations_Containers() throws Exception { + setSkylarkSemanticsOptions("--incompatible_descriptive_string_representations=true"); + + assertStringRepresentation("['a', 'b']", "[\"a\", \"b\"]"); + assertStringRepresentation("('a', 'b')", "(\"a\", \"b\")"); + assertStringRepresentation("{'a': 'b', 'c': 'd'}", "{\"a\": \"b\", \"c\": \"d\"}"); + assertStringRepresentation("struct(d = 4, c = 3)", "struct(c = 3, d = 4)"); + } + + @Test + public void testLegacyStringRepresentations_Labels() throws Exception { + setSkylarkSemanticsOptions("--incompatible_descriptive_string_representations=false"); + + assertThat(skylarkLoadingEval("str(Label('//foo:bar'))")).isEqualTo("//foo:bar"); + assertThat(skylarkLoadingEval("'%s' % Label('//foo:bar')")).isEqualTo("//foo:bar"); + assertThat(skylarkLoadingEval("'{}'.format(Label('//foo:bar'))")).isEqualTo("//foo:bar"); + assertThat(skylarkLoadingEval("repr(Label('//foo:bar'))")).isEqualTo("\"//foo:bar\""); + assertThat(skylarkLoadingEval("'%r' % Label('//foo:bar')")).isEqualTo("\"//foo:bar\""); + + // Also test that str representations (as opposed to repr) also use legacy formatting + // They are equivalent for labels, but not equivalent for lists of labels, because + // containers always render their items with repr + assertThat(skylarkLoadingEval("str([Label('//foo:bar')])")).isEqualTo("[\"//foo:bar\"]"); + assertThat(skylarkLoadingEval("'{}'.format([Label('//foo:bar')])")).isEqualTo("[\"//foo:bar\"]"); + assertThat(skylarkLoadingEval("'%s' % [Label('//foo:bar')]")).isEqualTo("[\"//foo:bar\"]"); + } + + @Test + public void testLegacyStringRepresentations_Functions() throws Exception { + setSkylarkSemanticsOptions("--incompatible_descriptive_string_representations=false"); + + assertStringRepresentation("all", "<function all>"); + assertStringRepresentation("def f(): pass", "f", "<function f>"); + } + + @Test + public void testLegacyStringRepresentations_Rules() throws Exception { + setSkylarkSemanticsOptions("--incompatible_descriptive_string_representations=false"); + + assertStringRepresentation("native.cc_library", "<function cc_library>"); + assertStringRepresentation("rule(implementation=str)", "<function rule>"); + assertStringRepresentation("aspect(implementation=str)", "Aspect:<function str>"); + } + + @Test + public void testLegacyStringRepresentations_Providers() throws Exception { + setSkylarkSemanticsOptions("--incompatible_descriptive_string_representations=false"); + + assertStringRepresentation("provider()", "<function <no name>>"); + assertStringRepresentation("p = provider()", "p(b = 2, a = 1)", "p(a = 1, b = 2)"); + } + + @Test + public void testLegacyStringRepresentations_Select() throws Exception { + setSkylarkSemanticsOptions("--incompatible_descriptive_string_representations=false"); + + assertStringRepresentation( + "select({'//foo': ['//bar']}) + select({'//foo2': ['//bar2']})", + "selector({\"//foo\": [\"//bar\"]}) + selector({\"//foo2\": [\"//bar2\"]})"); + } + + @Test + public void testLegacyStringRepresentations_Targets() throws Exception { + // alias targets in skylark used to leak their memory addresses in string representations, + // we don't try to preserve this behaviour as it's harmful. + // An example of their legacy representation: + // "<com.google.devtools.build.lib.rules.AliasConfiguredTarget@12da9140>" + + setSkylarkSemanticsOptions("--incompatible_descriptive_string_representations=false"); + + generateFilesToTestStrings(); + ConfiguredTarget target = getConfiguredTarget("//test/skylark:check"); + + + ImmutableList<Pair<String, String>> parameters = ImmutableList.of( + new Pair<>("rule_ctx", "//test/skylark:check"), + new Pair<>("aspect_ctx", "//test/skylark:bar"), + new Pair<>("input_target", "InputFileConfiguredTarget(//test/skylark:input.txt)")); + for (String suffix : SUFFIXES) { + for (Pair<String, String > pair : parameters) { + assertThat(target.get(pair.getFirst() + suffix)).isEqualTo(pair.getSecond()); + } + } + + // Legacy representation of several types of objects may contain nondeterministic chunks + parameters = ImmutableList.of( + new Pair<>("target", "ConfiguredTarget\\(//test/skylark:foo, [0-9a-f]+\\)"), + new Pair<>("aspect_target", "ConfiguredTarget\\(//test/skylark:bar, [0-9a-f]+\\)"), + new Pair<>("output_target", "ConfiguredTarget\\(//test/skylark:output.txt, [0-9a-f]+\\)")); + for (String suffix : SUFFIXES) { + for (Pair<String, String > pair : parameters) { + assertThat((String) target.get(pair.getFirst() + suffix)).matches(pair.getSecond()); + } + } + } +} diff --git a/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java index 5e9391f54e..0e1bdab147 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java @@ -47,8 +47,8 @@ public class EvaluationTest extends EvaluationTestCase { * <p>If a test uses this method, it allows potential subclasses to run the very same test in a * different mode in subclasses */ - protected ModalTestCase newTest() { - return new BuildTest(); + protected ModalTestCase newTest(String... skylarkOptions) { + return new BuildTest(skylarkOptions); } @Test @@ -601,7 +601,12 @@ public class EvaluationTest extends EvaluationTestCase { return new SkylarkValue() { @Override public void repr(SkylarkPrinter printer) { - printer.append("str marker"); + printer.append("<str marker>"); + } + + @Override + public void reprLegacy(SkylarkPrinter printer) { + printer.append("<str legacy marker>"); } @Override @@ -615,36 +620,45 @@ public class EvaluationTest extends EvaluationTestCase { return new Object() { @Override public String toString() { - return "unknown object"; + return "<unknown object>"; } }; } @Test public void testPercOnObject() throws Exception { - newTest() + newTest("--incompatible_descriptive_string_representations=true") + .update("obj", createObjWithStr()) + .testStatement("'%s' % obj", "<str marker>"); + newTest("--incompatible_descriptive_string_representations=false") .update("obj", createObjWithStr()) - .testStatement("'%s' % obj", "str marker"); + .testStatement("'%s' % obj", "<str legacy marker>"); newTest() .update("unknown", createUnknownObj()) - .testStatement("'%s' % unknown", "unknown object"); + .testStatement("'%s' % unknown", "<unknown object>"); } @Test public void testPercOnObjectList() throws Exception { - newTest() + newTest("--incompatible_descriptive_string_representations=true") + .update("obj", createObjWithStr()) + .testStatement("'%s %s' % (obj, obj)", "<str marker> <str marker>"); + newTest("--incompatible_descriptive_string_representations=false") .update("obj", createObjWithStr()) - .testStatement("'%s %s' % (obj, obj)", "str marker str marker"); + .testStatement("'%s %s' % (obj, obj)", "<str legacy marker> <str legacy marker>"); newTest() .update("unknown", createUnknownObj()) - .testStatement("'%s %s' % (unknown, unknown)", "unknown object unknown object"); + .testStatement("'%s %s' % (unknown, unknown)", "<unknown object> <unknown object>"); } @Test public void testPercOnObjectInvalidFormat() throws Exception { - newTest() + newTest("--incompatible_descriptive_string_representations=true") + .update("obj", createObjWithStr()) + .testIfExactError("invalid argument <str marker> for format pattern %d", "'%d' % obj"); + newTest("--incompatible_descriptive_string_representations=false") .update("obj", createObjWithStr()) - .testIfExactError("invalid argument str marker for format pattern %d", "'%d' % obj"); + .testIfExactError("invalid argument <str marker> for format pattern %d", "'%d' % obj"); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/syntax/PrinterTest.java b/src/test/java/com/google/devtools/build/lib/syntax/PrinterTest.java index fdf43271bb..f1e56915b4 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/PrinterTest.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/PrinterTest.java @@ -21,6 +21,8 @@ import com.google.common.base.Joiner; import com.google.common.base.Strings; import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.skylarkinterface.SkylarkPrinter; +import com.google.devtools.build.lib.skylarkinterface.SkylarkValue; import com.google.devtools.build.lib.syntax.SkylarkList.MutableList; import com.google.devtools.build.lib.syntax.SkylarkList.Tuple; import java.util.Arrays; @@ -55,7 +57,7 @@ public class PrinterTest { assertThat(Printer.repr(Runtime.NONE)).isEqualTo("None"); assertThat(Printer.str(Label.parseAbsolute("//x"))).isEqualTo("//x:x"); - assertThat(Printer.repr(Label.parseAbsolute("//x"))).isEqualTo("\"//x:x\""); + assertThat(Printer.repr(Label.parseAbsolute("//x"))).isEqualTo("Label(\"//x:x\")"); List<?> list = MutableList.of(null, "foo", "bar"); List<?> tuple = Tuple.of("foo", "bar"); @@ -215,6 +217,14 @@ public class PrinterTest { assertThat(Printer.str(list)).isEqualTo(String.format("[%s]", Joiner.on(", ").join(list))); } + @Test + public void testLegacyPrinter() throws Exception { + assertThat(new Printer.LegacyPrinter().str(createObjWithStr()).toString()) + .isEqualTo("<str legacy marker>"); + assertThat(new Printer.LegacyPrinter().repr(createObjWithStr()).toString()) + .isEqualTo("<repr legacy marker>"); + } + private String printListWithLimit(List<?> list) { return printList(list, Printer.SUGGESTED_CRITICAL_LIST_ELEMENTS_COUNT, Printer.SUGGESTED_CRITICAL_LIST_ELEMENTS_STRING_LENGTH); @@ -224,4 +234,33 @@ public class PrinterTest { return Printer.printAbbreviatedList( list, "[", ", ", "]", "", criticalElementsCount, criticalStringLength); } + + private SkylarkValue createObjWithStr() { + return new SkylarkValue() { + @Override + public void repr(SkylarkPrinter printer) { + printer.append("<repr marker>"); + } + + @Override + public void reprLegacy(SkylarkPrinter printer) { + printer.append("<repr legacy marker>"); + } + + @Override + public void str(SkylarkPrinter printer) { + printer.append("<str marker>"); + } + + @Override + public void strLegacy(SkylarkPrinter printer) { + printer.append("<str legacy marker>"); + } + + @Override + public boolean isImmutable() { + return false; + } + }; + } } diff --git a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java index 256985c686..b5c6a70600 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java @@ -55,8 +55,8 @@ public class SkylarkEvaluationTest extends EvaluationTest { * Skylark context */ @Override - protected ModalTestCase newTest() { - return new SkylarkTest(); + protected ModalTestCase newTest(String... skylarkOptions) { + return new SkylarkTest(skylarkOptions); } @Immutable diff --git a/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java b/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java index 19665c698e..70604c6349 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java @@ -579,11 +579,15 @@ public class EvaluationTestCase { * A class that runs all tests in Build mode */ protected class BuildTest extends ModalTestCase { - public BuildTest() {} + private final String[] skylarkOptions; + + public BuildTest(String... skylarkOptions) { + this.skylarkOptions = skylarkOptions; + } @Override protected void run(Testable testable) throws Exception { - enableBuildMode(); + enableBuildMode(skylarkOptions); testable.run(); } } |