Under normal circumstances, if a test author misspells a filename passed
to test_cmp(), the error is quickly discovered when the test fails
unexpectedly due to test_cmp() being unable to find the file. However,
if the test is expected to fail, as with test_expect_failure(), a
misspelled filename as argument to test_cmp() will go unnoticed since
the test will indeed fail, but for the wrong reason. Make it easier for
test authors to discover such problems early by sanity-checking the
arguments to test_cmp(). To avoid penalizing all clients of test_cmp()
in the general case, only check for missing files if the comparison
fails.
While at it, make test_cmp_bin() sanity-check its arguments, as well.
Signed-off-by: Eric Sunshine <sunshine@xxxxxxxxxxxxxx>
---
Notes:
This change was motivated by seeing Elijah's patch[1] which fixed
several cases of bogus test_cmp() invocations which perhaps could
have been discovered earlier had test_cmp() done some
sanity-checking of its arguments. It turns out that the tests in
question[1] fail before test_cmp() is ever called, so this patch
would not have helped catch those mistakes after all. It also only
helps catch mistakes in test_expect_failure() cases, which are
relatively rare compared with test_expect_success() cases, thus may
not have a lot of value. Nevertheless, it's a relatively small and
simple change which only kicks in when test_cmp() fails, thus should
not penalize the typical case; there are a handful of `! test_cmp`
cases, however, which will trigger the additional argument sanity
checking but that cost is probably small enough to go unnoticed.
[1]: https://lore.kernel.org/git/7f408b7d4069403b969d334f4940ebf87f1dc797.1596906081.git.gitgitgadget@xxxxxxxxx/
t/test-lib-functions.sh | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index b791933ffd..8d77deebd2 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -952,7 +952,13 @@ test_expect_code () {
# - not all diff versions understand "-u"
test_cmp() {
- eval "$GIT_TEST_CMP" '"$@"'
+ test $# -eq 2 || BUG "test_cmp requires two arguments"
+ if ! eval "$GIT_TEST_CMP" '"$@"'
+ then
+ test -e "$1" || BUG "test_cmp 'expect' file missing"
+ test -e "$2" || BUG "test_cmp 'actual' file missing"
+ return 1
+ fi
}
# Check that the given config key has the expected value.
@@ -981,7 +987,13 @@ test_cmp_config() {
# test_cmp_bin - helper to compare binary files
test_cmp_bin() {
- cmp "$@"
+ test $# -eq 2 || BUG "test_cmp_bin requires two arguments"
+ if ! cmp "$@"
+ then
+ test -e "$1" || BUG "test_cmp_bin 'expect' file missing"
+ test -e "$2" || BUG "test_cmp_bin 'actual' file missing"
+ return 1
+ fi
}
# Use this instead of test_cmp to compare files that contain expected and
--
2.28.0.213.gdd9653da77
