On 05/06/2014 07:43 AM, Bernhard Voelker wrote:
> On 05/06/2014 03:46 AM, Pádraig Brady wrote:
>> * tests/chmod/c-option.sh: Use `compare /dev/null ... || fail=1`
>> rather than `test -s ... && fail=1`, so that the file contents
>> are output, thus improving diagnostics for failing tests.
>> * tests/cp/cp-a-selinux.sh: Likewise.
>> * tests/cp/cp-mv-enotsup-xattr.sh: Likewise.
>> * tests/dd/misc.sh: Likewise.
>> * tests/misc/nice.sh: Likewise.
>> * tests/misc/xattr.sh: Likewise.
>> * tests/mv/update.sh: Likewise.
>> * tests/rm/deep-2.sh: Likewise.
>> * tests/rm/read-only.sh: Likewise.
>> * tests/split/r-chunk.sh: Likewise.
>> * tests/tail-2/follow-stdin.sh: Likewise.
>> * tests/tail-2/wait.sh: Likewise.
>> * tests/touch/no-dereference.sh: Likewise.
>
> Great idea!
>
> I'm curious: what was the criteria to choose those tests?
> I mean, "cd tests && git grep 'test -s.*&&' | wc -l" still
> counts 21 matches, even one where the part after the '&&'
> could be simplified like in tail-2/inotify-race.sh:55:
>
> test -s gdb.out && { cat gdb.out; skip_ "can't set breakpoints in tail"; }
> vs.
> compare /dev/null gdb.out || skip_ "can't set breakpoints in tail"
Yes it would be better to convert the above also.
For most of the other ones, outputting the contents on failure
wouldn't impart more info, but consistency beats edge case noise here,
so I've converted them all and added a syntax-check to avoid
future occurrences. New version is attached.
thanks for the thorough review!
Pádraig.
>From d25345875c6bba22e9d221fd26157a8b9d51ee9d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email protected]>
Date: Tue, 6 May 2014 02:37:43 +0100
Subject: [PATCH] tests: improve diagnostics when asserting empty files
* tests/chmod/c-option.sh: Use `compare /dev/null ... || fail=1`
rather than `test -s ... && fail=1`, so that the file contents
are output, thus improving diagnostics for failing tests.
* tests/cp/acl.sh: Likewise.
* tests/cp/cp-a-selinux.sh: Likewise.
* tests/cp/cp-mv-enotsup-xattr.sh: Likewise.
* tests/cp/reflink-perm.sh: Likewise.
* tests/dd/misc.sh: Likewise.
* tests/misc/env-null.sh: Likewise.
* tests/misc/env.sh: Likewise.
* tests/misc/nice.sh: Likewise.
* tests/misc/nohup.sh: Likewise.
* tests/misc/printenv.sh: Likewise.
* tests/misc/xattr.sh: Likewise.
* tests/mv/update.sh: Likewise.
* tests/rm/deep-2.sh: Likewise.
* tests/rm/read-only.sh: Likewise.
* tests/split/r-chunk.sh: Likewise.
* tests/tail-2/follow-stdin.sh: Likewise.
* tests/tail-2/inotify-race.sh: Likewise.
* tests/tail-2/wait.sh: Likewise.
* tests/touch/no-dereference.sh: Likewise.
* cfg.mk (sc_prohibit_test_empty:): New syntax-check.
* tests/cp/proc-zero-len.sh: Adjust to avoid false syntax-check failure.
* tests/cp/proc-zero-len.sh: Likewise.
* tests/mv/part-symlink.sh: Likewise.
* tests/tail-2/infloop-1.sh: Likewise.
---
cfg.mk | 7 +++++++
tests/chmod/c-option.sh | 2 +-
tests/cp/acl.sh | 2 +-
tests/cp/cp-a-selinux.sh | 9 +++++----
tests/cp/cp-mv-enotsup-xattr.sh | 8 ++++----
tests/cp/proc-zero-len.sh | 6 ++++--
tests/cp/reflink-perm.sh | 4 ++--
tests/dd/misc.sh | 6 +++---
tests/misc/env-null.sh | 4 ++--
tests/misc/env.sh | 6 +++---
tests/misc/nice.sh | 4 ++--
tests/misc/nohup.sh | 6 +++---
tests/misc/printenv.sh | 4 ++--
tests/misc/xattr.sh | 2 +-
tests/mv/part-symlink.sh | 3 ++-
tests/mv/update.sh | 2 +-
tests/rm/deep-2.sh | 2 +-
tests/rm/read-only.sh | 2 +-
tests/split/r-chunk.sh | 2 +-
tests/tail-2/follow-stdin.sh | 2 +-
tests/tail-2/infloop-1.sh | 3 ++-
tests/tail-2/inotify-race.sh | 2 +-
tests/tail-2/wait.sh | 2 +-
tests/touch/no-dereference.sh | 4 ++--
24 files changed, 53 insertions(+), 41 deletions(-)
diff --git a/cfg.mk b/cfg.mk
index b3c0f70..f84b1a4 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -423,6 +423,13 @@ sc_prohibit_test_backticks:
halt='use $$(...), not `...` in tests/' \
$(_sc_search_regexp)
+# Ensure that compare is used to check empty files
+# so that the unexpected contents are displayed
+sc_prohibit_test_empty:
+ @prohibit='test -s.*&&' in_vc_files='^tests/' \
+ halt='use `compare /dev/null ...`, not `test -s ...` in tests/' \
+ $(_sc_search_regexp)
+
# Programs like sort, ls, expr use PROG_FAILURE in place of EXIT_FAILURE.
# Others, use the EXIT_CANCELED, EXIT_ENOENT, etc. macros defined in system.h.
# In those programs, ensure that EXIT_FAILURE is not used by mistake.
diff --git a/tests/chmod/c-option.sh b/tests/chmod/c-option.sh
index a1782c3..03e1db9 100755
--- a/tests/chmod/c-option.sh
+++ b/tests/chmod/c-option.sh
@@ -31,7 +31,7 @@ chmod u=rwx $file || fail=1
chmod -c g=rwx $file > out || fail=1
chmod -c g=rwx $file > empty || fail=1
-test -s empty && fail=1
+compare /dev/null empty || fail=1
case "$(cat out)" in
"mode of 'f' changed from 0744 "?rwxr--r--?" to 0774 "?rwxrwxr--?) ;;
*) cat out; fail=1 ;;
diff --git a/tests/cp/acl.sh b/tests/cp/acl.sh
index 0dc9f88..36a5d29 100755
--- a/tests/cp/acl.sh
+++ b/tests/cp/acl.sh
@@ -53,7 +53,7 @@ test "$acl1" = "$acl2" || fail=1
echo > a/file || framework_failure_ # add some data
test -s a/file || framework_failure_
cp -p --attributes-only a/file b/ || fail=1
-test -s b/file && fail=1
+compare /dev/null b/file || fail=1
acl2=$(cd b && getfacl file) || framework_failure_
test "$acl1" = "$acl2" || fail=1
diff --git a/tests/cp/cp-a-selinux.sh b/tests/cp/cp-a-selinux.sh
index 3ab7e0e..db0d689 100755
--- a/tests/cp/cp-a-selinux.sh
+++ b/tests/cp/cp-a-selinux.sh
@@ -37,7 +37,8 @@ cp -a c d 2>err || framework_failure_
cp --preserve=context c e || framework_failure_
cp --preserve=all c f || framework_failure_
ls -Z d | grep $ctx || fail=1
-test -s err && fail=1 #there must be no stderr output for -a
+# there must be no stderr output for -a
+compare /dev/null err || fail=1
ls -Z e | grep $ctx || fail=1
ls -Z f | grep $ctx || fail=1
@@ -116,7 +117,7 @@ echo > g || framework_failure_
# succeed (giving no diagnostics), yet leaving the destination file empty.
cp -a f g 2>err || fail=1
test -s g || fail=1 # The destination file must not be empty.
-test -s err && fail=1 # There must be no stderr output.
+compare /dev/null err || fail=1
# =====================================================
# Here, we expect cp to succeed and not warn with "Operation not supported"
@@ -151,7 +152,7 @@ echo > g
# security context through NFS or a mount with fixed context.
cp --preserve=context f g 2> out && fail=1
# Here, we *do* expect the destination to be empty.
-test -s g && fail=1
+compare /dev/null g || fail=1
sed "s/ .g'.*//" out > k
mv k out
compare exp out || fail=1
@@ -161,7 +162,7 @@ echo > g
# Check if -a option doesn't silence --preserve=context option diagnostics
cp -a --preserve=context f g 2> out2 && fail=1
# Here, we *do* expect the destination to be empty.
-test -s g && fail=1
+compare /dev/null g || fail=1
sed "s/ .g'.*//" out2 > k
mv k out2
compare exp out2 || fail=1
diff --git a/tests/cp/cp-mv-enotsup-xattr.sh b/tests/cp/cp-mv-enotsup-xattr.sh
index d711683..6c8fa83 100755
--- a/tests/cp/cp-mv-enotsup-xattr.sh
+++ b/tests/cp/cp-mv-enotsup-xattr.sh
@@ -69,19 +69,19 @@ grep -F "$xattr_pair" out_a >/dev/null \
# This should pass without diagnostics
cp -a xattr/a noxattr/ 2>err || fail=1
test -s noxattr/a || fail=1 # destination file must not be empty
-test -s err && fail=1 # there must be no stderr output
+compare /dev/null err || fail=1
rm -f err noxattr/a
# This should pass without diagnostics (new file)
cp --preserve=all xattr/a noxattr/ 2>err || fail=1
test -s noxattr/a || fail=1 # destination file must not be empty
-test -s err && fail=1 # there must be no stderr output
+compare /dev/null err || fail=1
# This should pass without diagnostics (existing file)
cp --preserve=all xattr/a noxattr/ 2>err || fail=1
test -s noxattr/a || fail=1 # destination file must not be empty
-test -s err && fail=1 # there must be no stderr output
+compare /dev/null err || fail=1
rm -f err noxattr/a
@@ -104,7 +104,7 @@ rm -f err noxattr/a
# This should pass without diagnostics
mv xattr/a noxattr/ 2>err || fail=1
test -s noxattr/a || fail=1 # destination file must not be empty
-test -s err && fail=1 # there must be no stderr output
+compare /dev/null err || fail=1
# This should pass and copy xattrs of the symlink
# since they're not in the 'user.' namespace.
diff --git a/tests/cp/proc-zero-len.sh b/tests/cp/proc-zero-len.sh
index 3369cfb..3fcd5aa 100755
--- a/tests/cp/proc-zero-len.sh
+++ b/tests/cp/proc-zero-len.sh
@@ -37,8 +37,10 @@ cp $f exp 2>err \
# Don't simply compare contents; they might differ,
# e.g., if CPU freq changes between cat and cp invocations.
# Instead, simply compare whether they're both nonempty.
-test -s out && { rm -f out; echo nonempty > out; }
-test -s exp && { rm -f exp; echo nonempty > exp; }
+test -s out \
+ && { rm -f out; echo nonempty > out; }
+test -s exp \
+ && { rm -f exp; echo nonempty > exp; }
compare exp out || fail=1
diff --git a/tests/cp/reflink-perm.sh b/tests/cp/reflink-perm.sh
index 52f83fc..52f5e25 100755
--- a/tests/cp/reflink-perm.sh
+++ b/tests/cp/reflink-perm.sh
@@ -38,8 +38,8 @@ test copy -nt file && fail=1
# Ensure that --attributes-only overrides --reflink completely
echo > file2 # file with data
cp --reflink=auto --preserve --attributes-only file2 empty_copy || fail=1
-test -s empty_copy && fail=1
+compare /dev/null empty_copy || fail=1
cp --reflink=always --preserve --attributes-only file2 empty_copy || fail=1
-test -s empty_copy && fail=1
+compare /dev/null empty_copy || fail=1
Exit $fail
diff --git a/tests/dd/misc.sh b/tests/dd/misc.sh
index bb4748c..f877fdd 100755
--- a/tests/dd/misc.sh
+++ b/tests/dd/misc.sh
@@ -32,12 +32,12 @@ ln -s $tmp_in $tmp_sym || framework_failure_
# check status=none suppresses all output to stderr
dd status=none if=$tmp_in of=/dev/null 2> err || fail=1
-test -s err && { cat err; fail=1; }
+compare /dev/null err || fail=1
dd status=none if=$tmp_in skip=2 of=/dev/null 2> err || fail=1
-test -s err && { cat err; fail=1; }
+compare /dev/null err || fail=1
# check status=none is cumulative with status=noxfer
dd status=none status=noxfer if=$tmp_in of=/dev/null 2> err || fail=1
-test -s err && { cat err; fail=1; }
+compare /dev/null err || fail=1
dd if=$tmp_in of=$tmp_out 2> /dev/null || fail=1
compare $tmp_in $tmp_out || fail=1
diff --git a/tests/misc/env-null.sh b/tests/misc/env-null.sh
index 6544fb3..9b9c95e 100755
--- a/tests/misc/env-null.sh
+++ b/tests/misc/env-null.sh
@@ -40,7 +40,7 @@ compare out1 out2 || fail=1
# env -0 does not work if a command is specified.
env -0 echo hi > out
test $? = 125 || fail=1
-test -s out && fail=1
+compare /dev/null out || fail=1
# Test env -0 on a one-variable environment.
printf 'a=b\nc=\0' > exp || framework_failure_
@@ -53,7 +53,7 @@ env "$(printf 'a=b\nc=')" printenv -0 a > out || fail=1
compare exp out || fail=1
env -u a printenv -0 a > out
test $? = 1 || fail=1
-test -s out && fail=1
+compare /dev/null out || fail=1
env -u b "$(printf 'a=b\nc=')" printenv -0 b a > out
test $? = 1 || fail=1
compare exp out || fail=1
diff --git a/tests/misc/env.sh b/tests/misc/env.sh
index 877a5e3..c4b9737 100755
--- a/tests/misc/env.sh
+++ b/tests/misc/env.sh
@@ -25,11 +25,11 @@ print_ver_ env
a=1
export a
env - > out || fail=1
-test -s out && fail=1
+compare /dev/null out || fail=1
env -i > out || fail=1
-test -s out && fail=1
+compare /dev/null out || fail=1
env -u a -i -u a -- > out || fail=1
-test -s out && fail=1
+compare /dev/null out || fail=1
env -i -- a=b > out || fail=1
echo a=b > exp || framework_failure_
compare exp out || fail=1
diff --git a/tests/misc/nice.sh b/tests/misc/nice.sh
index 8efb7d8..26a01ca 100755
--- a/tests/misc/nice.sh
+++ b/tests/misc/nice.sh
@@ -81,12 +81,12 @@ if test x$(nice -n -1 nice 2> /dev/null) = x0 ; then
if test -w /dev/full && test -c /dev/full; then
nice -n -1 nice > out 2> /dev/full
test $? = 125 || fail=1
- test -s out && fail=1
+ compare /dev/null out || fail=1
fi
else
# superuser - change succeeds
nice -n -1 nice 2> err || fail=1
- test -s err && fail=1
+ compare /dev/null err || fail=1
test x$(nice -n -1 nice) = x-1 || fail=1
test x$(nice --1 nice) = x-1 || fail=1
fi
diff --git a/tests/misc/nohup.sh b/tests/misc/nohup.sh
index 2328b43..b3f4274 100755
--- a/tests/misc/nohup.sh
+++ b/tests/misc/nohup.sh
@@ -73,7 +73,7 @@ if test -w /dev/full && test -c /dev/full; then
nohup echo hi 2> /dev/full
test $? = 125 || fail=1
test -f nohup.out || fail=1
- test -s nohup.out && fail=1
+ compare /dev/null nohup.out || fail=1
rm -f nohup.out
exit $fail
) || fail=1
@@ -86,7 +86,7 @@ if test -t 1; then
# It must exist.
test -f nohup.out || fail=1
# It must be empty.
- test -s nohup.out && fail=1
+ compare /dev/null nohup.out || fail=1
fi
cat <<\EOF > exp || fail=1
@@ -106,7 +106,7 @@ if test -t 1; then
# It must exist.
test -f nohup.out || fail=1
# It must be empty.
- test -s nohup.out && fail=1
+ compare /dev/null nohup.out || fail=1
fi
cat <<\EOF > exp || fail=1
diff --git a/tests/misc/printenv.sh b/tests/misc/printenv.sh
index fb911d0..054b02c 100755
--- a/tests/misc/printenv.sh
+++ b/tests/misc/printenv.sh
@@ -37,7 +37,7 @@ fi
# Printing a single variable's value.
env -- printenv ENV_TEST > out
test $? = 1 || fail=1
-test -s out && fail=1
+compare /dev/null out || fail=1
echo a > exp || framework_failure_
ENV_TEST=a env -- printenv ENV_TEST > out || fail=1
compare exp out || fail=1
@@ -76,6 +76,6 @@ compare exp out || fail=1
# Bug present through coreutils 8.0.
env a=b=c printenv a=b > out
test $? = 1 || fail=1
-test -s out && fail=1
+compare /dev/null out || fail=1
Exit $fail
diff --git a/tests/misc/xattr.sh b/tests/misc/xattr.sh
index 2694246..f208090 100755
--- a/tests/misc/xattr.sh
+++ b/tests/misc/xattr.sh
@@ -66,7 +66,7 @@ getfattr -d c >out_c || skip_ "failed to get xattr of file"
grep -F "$xattr_pair" out_c || fail=1
# cp's -a option must produce no diagnostics.
-cp -a a d 2>err && test -s err && fail=1
+cp -a a d 2>err && { compare /dev/null err || fail=1; }
getfattr -d d >out_d || skip_ "failed to get xattr of file"
grep -F "$xattr_pair" out_d || fail=1
diff --git a/tests/mv/part-symlink.sh b/tests/mv/part-symlink.sh
index 9d1d6a4..9604d87 100755
--- a/tests/mv/part-symlink.sh
+++ b/tests/mv/part-symlink.sh
@@ -88,7 +88,8 @@ for copy in cp mv; do
# Normalize the program name in the error output,
# remove any site-dependent part of other-partition file name,
# and put brackets around the output.
- test -s .err && {
+ test -s .err \
+ && {
echo ' [' | tr -d '\n'
sed 's/^[^:][^:]*\(..\):/\1:/;s,'"$other_partition_tmpdir/,," .err |
tr -d '\n'
diff --git a/tests/mv/update.sh b/tests/mv/update.sh
index 688a8f2..4c0553a 100755
--- a/tests/mv/update.sh
+++ b/tests/mv/update.sh
@@ -30,7 +30,7 @@ for interactive in '' -i; do
# With coreutils-6.9 and earlier, using --update with -i would
# mistakenly elicit a prompt.
$cp_or_mv $interactive --update old new < /dev/null > out 2>&1 || fail=1
- test -s out && fail=1
+ compare /dev/null out || fail=1
case "$(cat new)" in new) ;; *) fail=1 ;; esac
case "$(cat old)" in old) ;; *) fail=1 ;; esac
done
diff --git a/tests/rm/deep-2.sh b/tests/rm/deep-2.sh
index 24415ed..c3ea085 100755
--- a/tests/rm/deep-2.sh
+++ b/tests/rm/deep-2.sh
@@ -46,7 +46,7 @@ echo n > no || framework_failure_
rm ---presume-input-tty -r x < no > out || fail=1
# expect empty output
-test -s out && fail=1
+compare /dev/null out || fail=1
# the directory must have been removed
test -d x && fail=1
diff --git a/tests/rm/read-only.sh b/tests/rm/read-only.sh
index 1ac8f27..377305c 100755
--- a/tests/rm/read-only.sh
+++ b/tests/rm/read-only.sh
@@ -42,7 +42,7 @@ test $skip = 1 \
# Applying rm -f to a nonexistent file on a read-only file system must succeed.
rm -f mnt/no-such > out 2>&1 || fail=1
# It must produce no diagnostic.
-test -s out && fail=1
+compare /dev/null out || fail=1
# However, trying to remove an existing file must fail.
rm -f mnt/f > out 2>&1 && fail=1
diff --git a/tests/split/r-chunk.sh b/tests/split/r-chunk.sh
index e84dfa5..8a5f221 100755
--- a/tests/split/r-chunk.sh
+++ b/tests/split/r-chunk.sh
@@ -32,7 +32,7 @@ stat x?? 2>/dev/null && fail=1
printf '1\n2\n3\n4\n5\n' > in || framework_failure_
split -n r/3 in > out || fail=1
-test -s out && fail=1
+compare /dev/null out || fail=1
split -n r/1/3 in > r1 || fail=1
split -n r/2/3 in > r2 || fail=1
diff --git a/tests/tail-2/follow-stdin.sh b/tests/tail-2/follow-stdin.sh
index 8ca10d6..1769ee2 100755
--- a/tests/tail-2/follow-stdin.sh
+++ b/tests/tail-2/follow-stdin.sh
@@ -28,7 +28,7 @@ timeout 1 tail -f < in > out 2> err
test $? = 124 || fail=1
# Ensure there was no error output.
-test -s err && fail=1
+compare /dev/null err || fail=1
# Ensure there was
compare exp out || fail=1
diff --git a/tests/tail-2/infloop-1.sh b/tests/tail-2/infloop-1.sh
index f553e9a..37d081f 100755
--- a/tests/tail-2/infloop-1.sh
+++ b/tests/tail-2/infloop-1.sh
@@ -22,7 +22,8 @@ print_ver_ tail
yes > t &
yes_pid=$!
while :; do
- test -s t && break
+ test -s t \
+ && break
sleep .1
done
tail -n 1 t &
diff --git a/tests/tail-2/inotify-race.sh b/tests/tail-2/inotify-race.sh
index efef7d6..c25f354 100755
--- a/tests/tail-2/inotify-race.sh
+++ b/tests/tail-2/inotify-race.sh
@@ -52,7 +52,7 @@ timeout 10s gdb -nx --batch-silent \
# FIXME: The above is seen to _intermittently_ fail with:
# warning: .dynamic section for "/lib/libc.so.6" is not at the expected address
# warning: difference appears to be caused by prelink, adjusting expectations
-test -s gdb.out && { cat gdb.out; skip_ "can't set breakpoints in tail"; }
+compare /dev/null gdb.out || skip_ "can't set breakpoints in tail"
# Run "tail -f file", stopping to append a line just before
# inotify initialization, and then continue. Before the fix,
diff --git a/tests/tail-2/wait.sh b/tests/tail-2/wait.sh
index 13898ac..3dec55c 100755
--- a/tests/tail-2/wait.sh
+++ b/tests/tail-2/wait.sh
@@ -51,7 +51,7 @@ for inotify in ---disable-inotify ''; do
grep -Ev 'inotify (resources exhausted|cannot be used)' tail.err > x
mv x tail.err
- test -s tail.err && fail=1
+ compare /dev/null tail.err || fail=1
>tail.err
tail_F()
diff --git a/tests/touch/no-dereference.sh b/tests/touch/no-dereference.sh
index 718fea7..1d012a1 100755
--- a/tests/touch/no-dereference.sh
+++ b/tests/touch/no-dereference.sh
@@ -29,7 +29,7 @@ ln -s file link || framework_failure_
touch -h no-file 2> err && fail=1
test -s err || fail=1
touch -h -c no-file 2> err || fail=1
-test -s err && fail=1
+compare /dev/null err || fail=1
# -h works on regular files
touch -h file || fail=1
@@ -49,7 +49,7 @@ grep '^#define HAVE_LUTIMES 1' "$CONFIG_HEADER" > /dev/null ||
touch -h dangling 2> err
case $? in
0) test -f nowhere && fail=1
- test -s err && fail=1;;
+ compare /dev/null err || fail=1;;
1) grep 'Function not implemented' err \
&& skip_ 'this system lacks the utimensat function'
fail=1;;
--
1.7.7.6
