From de3035d605b4c89a62d6da060188e4ab0c5034b9 Mon Sep 17 00:00:00 2001 From: Alexandre Rostovtsev Date: Wed, 6 Apr 2022 15:16:14 -0400 Subject: [PATCH] Properly shell-quote diff_test's failure_message in bash (and document the failure_message attribute) (#364) Addresses #344 for Unix; #363 is needed as prerequisite for the corresponding Windows fix. --- docs/diff_test_doc.md | 3 ++- rules/BUILD | 1 + rules/diff_test.bzl | 11 +++++--- tests/diff_test/diff_test_tests.sh | 42 ++++++++++++++++++++++++++++++ 4 files changed, 53 insertions(+), 4 deletions(-) diff --git a/docs/diff_test_doc.md b/docs/diff_test_doc.md index fd62a38..0271f4d 100755 --- a/docs/diff_test_doc.md +++ b/docs/diff_test_doc.md @@ -11,7 +11,7 @@ command (fc.exe) on Windows (no Bash is required). ## diff_test 
-diff_test(name, file1, file2, kwargs)
+diff_test(name, file1, file2, failure_message, kwargs)
A test that compares two files. @@ -27,6 +27,7 @@ The test succeeds if the files' contents match. | name | The name of the test rule. | none | | file1 | Label of the file to compare to <code>file2</code>. | none | | file2 | Label of the file to compare to <code>file1</code>. | none | +| failure_message | Additional message to log if the files' contents do not match. | None | | kwargs | The <a href="https://docs.bazel.build/versions/main/be/common-definitions.html#common-attributes-tests">common attributes for tests</a>. | none | diff --git a/rules/BUILD b/rules/BUILD index b89495e..c1ffbf2 100644 --- a/rules/BUILD +++ b/rules/BUILD @@ -30,6 +30,7 @@ bzl_library( bzl_library( name = "diff_test", srcs = ["diff_test.bzl"], + deps = ["//lib:shell"], ) bzl_library( diff --git a/rules/diff_test.bzl b/rules/diff_test.bzl index 49a1968..04bf26b 100644 --- a/rules/diff_test.bzl +++ b/rules/diff_test.bzl @@ -18,6 +18,8 @@ The rule uses a Bash command (diff) on Linux/macOS/non-Windows, and a cmd.exe command (fc.exe) on Windows (no Bash is required). """ +load("//lib:shell.bzl", "shell") + def _runfiles_path(f): if f.root.path: return f.path[len(f.root.path) + 1:] # generated file @@ -76,6 +78,7 @@ if %ERRORLEVEL% neq 0 ( ) ) """.format( + # TODO(arostovtsev): use shell.escape_for_bat when https://github.com/bazelbuild/bazel-skylib/pull/363 is merged fail_msg = ctx.attr.failure_message, file1 = _runfiles_path(ctx.file.file1), file2 = _runfiles_path(ctx.file.file2), @@ -106,11 +109,11 @@ else exit 1 fi if ! diff "$RF1" "$RF2"; then - echo >&2 "FAIL: files \"{file1}\" and \"{file2}\" differ. {fail_msg}" + echo >&2 "FAIL: files \"{file1}\" and \"{file2}\" differ. "{fail_msg} exit 1 fi """.format( - fail_msg = ctx.attr.failure_message, + fail_msg = shell.quote(ctx.attr.failure_message), file1 = _runfiles_path(ctx.file.file1), file2 = _runfiles_path(ctx.file.file2), ), @@ -139,7 +142,7 @@ _diff_test = rule( implementation = _diff_test_impl, ) -def diff_test(name, file1, file2, **kwargs): +def diff_test(name, file1, file2, failure_message = None, **kwargs): """A test that compares two files. The test succeeds if the files' contents match. @@ -148,12 +151,14 @@ def diff_test(name, file1, file2, **kwargs): name: The name of the test rule. file1: Label of the file to compare to file2. file2: Label of the file to compare to file1. + failure_message: Additional message to log if the files' contents do not match. **kwargs: The common attributes for tests. """ _diff_test( name = name, file1 = file1, file2 = file2, + failure_message = failure_message, is_windows = select({ "@bazel_tools//src/conditions:host_windows": True, "//conditions:default": False, diff --git a/tests/diff_test/diff_test_tests.sh b/tests/diff_test/diff_test_tests.sh index 4b58e6c..619612e 100755 --- a/tests/diff_test/diff_test_tests.sh +++ b/tests/diff_test/diff_test_tests.sh @@ -43,9 +43,13 @@ source "$(rlocation bazel_skylib/tests/unittest.bash)" \ function import_diff_test() { local -r repo="$1" mkdir -p "${repo}/rules" + mkdir -p "${repo}/lib" + touch "${repo}/lib/BUILD" touch "${repo}/WORKSPACE" ln -sf "$(rlocation bazel_skylib/rules/diff_test.bzl)" \ "${repo}/rules/diff_test.bzl" + ln -sf "$(rlocation bazel_skylib/lib/shell.bzl)" \ + "${repo}/lib/shell.bzl" echo "exports_files(['diff_test.bzl'])" > "${repo}/rules/BUILD" } @@ -214,5 +218,43 @@ function test_from_ext_repo_without_legacy_external_runfiles() { assert_from_ext_repo "--nolegacy_external_runfiles" "${FUNCNAME[0]}" } +function test_failure_message() { + local -r ws="${TEST_TMPDIR}/${FUNCNAME[0]}" + + import_diff_test "$ws" + touch "$ws/WORKSPACE" + cat >"$ws/BUILD" <<'eof' +load("//rules:diff_test.bzl", "diff_test") + +diff_test( + name = "different_with_message", + failure_message = "This is an `$error`", # TODO(arostovtsev): also test Windows cmd.exe escapes when https://github.com/bazelbuild/bazel-skylib/pull/363 is merged + file1 = "a.txt", + file2 = "b.txt", +) + +diff_test( + name = "different_without_message", + file1 = "c.txt", + file2 = "d.txt", +) +eof + echo foo > "$ws/a.txt" + echo bar > "$ws/b.txt" + echo foo > "$ws/c.txt" + echo bar > "$ws/d.txt" + + (cd "$ws" && \ + bazel test //:different_with_message --test_output=errors 1>"$TEST_log" 2>&1 \ + && fail "expected failure" || true) + # TODO(arostovtsev): also test Windows cmd.exe escapes when https://github.com/bazelbuild/bazel-skylib/pull/363 is merged + expect_log "FAIL: files \"a.txt\" and \"b.txt\" differ. This is an \`\$error\`" + + (cd "$ws" && \ + bazel test //:different_without_message --test_output=errors 1>"$TEST_log" 2>&1 \ + && fail "expected failure" || true) + expect_log "FAIL: files \"c.txt\" and \"d.txt\" differ. $" +} + cd "$TEST_TMPDIR" run_suite "diff_test_tests test suite"