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

@@ -11,7 +11,7 @@ command (fc.exe) on Windows (no Bash is required).
 ## diff_test
 
 <pre>
-diff_test(<a href="#diff_test-name">name</a>, <a href="#diff_test-file1">file1</a>, <a href="#diff_test-file2">file2</a>, <a href="#diff_test-kwargs">kwargs</a>)
+diff_test(<a href="#diff_test-name">name</a>, <a href="#diff_test-file1">file1</a>, <a href="#diff_test-file2">file2</a>, <a href="#diff_test-failure_message">failure_message</a>, <a href="#diff_test-kwargs">kwargs</a>)
 </pre>
 
 A test that compares two files.
@@ -27,6 +27,7 @@ The test succeeds if the files' contents match.
 | <a id="diff_test-name"></a>name |  The name of the test rule.   |  none |
 | <a id="diff_test-file1"></a>file1 |  Label of the file to compare to &lt;code&gt;file2&lt;/code&gt;.   |  none |
 | <a id="diff_test-file2"></a>file2 |  Label of the file to compare to &lt;code&gt;file1&lt;/code&gt;.   |  none |
+| <a id="diff_test-failure_message"></a>failure_message |  Additional message to log if the files' contents do not match.   |  <code>None</code> |
 | <a id="diff_test-kwargs"></a>kwargs |  The &lt;a href="https://docs.bazel.build/versions/main/be/common-definitions.html#common-attributes-tests"&gt;common attributes for tests&lt;/a&gt;.   |  none |
 
 

rules/BUILD

@@ -30,6 +30,7 @@ bzl_library(
 bzl_library(
     name = "diff_test",
     srcs = ["diff_test.bzl"],
+    deps = ["//lib:shell"],
 )
 
 bzl_library(

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 <code>file2</code>.
       file2: Label of the file to compare to <code>file1</code>.
+      failure_message: Additional message to log if the files' contents do not match.
       **kwargs: The <a href="https://docs.bazel.build/versions/main/be/common-definitions.html#common-attributes-tests">common attributes for tests</a>.
     """
     _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,

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"