On Sun, Nov 13, 2016 at 11:05 AM, Jim Meyering <[email protected]> wrote: > On Sun, Nov 13, 2016 at 9:24 AM, Bernhard Voelker > <[email protected]> wrote: >> On 11/13/2016 03:38 AM, Jim Meyering wrote: >>> I noticed many tests that compare directly with "$?". However, we now >>> have the "returns_" function (from init.sh) that can be used to make >>> the resulting code a little less error-prone: all on one line/stmt, it >>> is harder to accidentally insert code that accidentally clobbers "$?". >>> >>> Here's an example of what most of these changes look like: >>> >>> -ls -l --time-style=XX > out 2> err >>> -test $? = 2 || fail=1 >>> +returns_ 2 ls -l --time-style=XX > out 2> err || fail=1 >> >> Nice work, thanks! >> >> BTW: what was your search pattern? I mean, is there a reason >> you left places like the following alone? > > Good catch. Thank you. Amended patch below. > I did the conversion in two steps. Initially I searched only for > single-digit values on the RHS, then realized my error and > found/converted the multi-digit ones, too, initially in a separate > commit. Could have sworn I merged them, but reflog shows no trace. > > However, I did deliberately leave two unconverted, e.g., > > tests/ls/readdir-mountpoint-inode.sh-while read dir; do > tests/ls/readdir-mountpoint-inode.sh- readdir_inode=$(inode_via_readdir > "$dir") > tests/ls/readdir-mountpoint-inode.sh: test $? = 77 && continue > tests/ls/readdir-mountpoint-inode.sh- stat_inode=$(timeout 1 stat > --format=%i "$dir") > tests/ls/readdir-mountpoint-inode.sh- # If stat fails or says the > inode is 0, skip $dir. > -- > tests/misc/chroot-fail.sh- returns_ 127 chroot / no_such || fail=1 # > no such command > tests/misc/chroot-fail.sh-else > tests/misc/chroot-fail.sh: test $? = 125 || fail=1 > tests/misc/chroot-fail.sh- can_chroot_root=0 > tests/misc/chroot-fail.sh-fi > > We may want to convert those, too (if it can be done cleanly), so that > we can add an exception-free syntax-check. Or just add an exception > for each. > >> $ git grep -B1 -F '$? = ' -- tests >> .. >> tests/misc/stdbuf.sh-stdbuf -ol true # Capital 'L' required >> tests/misc/stdbuf.sh:test $? = 125 || fail=1 # Internal error is a >> particular status >> .. >> >> $ git grep -B1 -F '$? != ' -- tests >> tests/du/8gb.sh-dd bs=1 seek=8G of=big < /dev/null 2> /dev/null >> tests/du/8gb.sh:if test $? != 0; then >> .. >> >> Furthermore, we need to tweak a syntax-check, see attached. > > Good catch. Please push. > > Thanks again for catching those. > I've attached both the delta and the merged V2.
Here's the patch I expect to push:
From cfce58e453a56f0e60b73157e01d17c193af19a4 Mon Sep 17 00:00:00 2001 From: Jim Meyering <[email protected]> Date: Tue, 8 Nov 2016 19:57:41 -0600 Subject: [PATCH] tests: use "returns_" rather than explicit comparison with "$?" The previous "returns_"-using change failed to convert many uses of "$?". Convert all but two of the remaining ones. * tests/ls/stat-vs-dirent.sh: Likewise. * tests/misc/head-write-error.sh: Likewise. * tests/misc/nice.sh: Likewise. * tests/misc/nohup.sh: Likewise. * tests/misc/stdbuf.sh: Likewise. * tests/misc/sync.sh: Likewise. * tests/tail-2/pid.sh: Likewise. * tests/tail-2/wait.sh: Likewise. Thanks to Bernhard Volker for spotting this. --- tests/ls/stat-vs-dirent.sh | 3 +-- tests/misc/head-write-error.sh | 7 +++---- tests/misc/nice.sh | 3 +-- tests/misc/nohup.sh | 9 +++------ tests/misc/stdbuf.sh | 22 ++++++++++------------ tests/misc/sync.sh | 3 +-- tests/tail-2/pid.sh | 6 ++---- tests/tail-2/wait.sh | 18 ++++++------------ 8 files changed, 27 insertions(+), 44 deletions(-) diff --git a/tests/ls/stat-vs-dirent.sh b/tests/ls/stat-vs-dirent.sh index b9f3cd1..fa2087f 100755 --- a/tests/ls/stat-vs-dirent.sh +++ b/tests/ls/stat-vs-dirent.sh @@ -23,8 +23,7 @@ print_ver_ ls root_dev_ino=$(stat --format=%d-%i /) t=$(pwd) while :; do - ls -i1 "$t" > tmp - if test $? = 0; then + if ls -i1 "$t" > tmp; then # Extract the inode number from the first line of output from ls -i1. # This value comes from dirent.d_ino, on systems with d_ino support. d_ino=$(sed -n '1s/^ *\([0-9][0-9]*\) .*/\1/p;q' tmp) diff --git a/tests/misc/head-write-error.sh b/tests/misc/head-write-error.sh index 4c021f5..1e8e566 100755 --- a/tests/misc/head-write-error.sh +++ b/tests/misc/head-write-error.sh @@ -36,14 +36,13 @@ printf '%s\n' "head: error writing 'standard output'" > exp for item in lines bytes; do for N in 0 1; do # pipe case - yes | timeout 10s head --$item=-$N > /dev/full 2> errt && fail=1 - test $? = 124 && fail=1 + yes | returns_ 1 timeout 10s head --$item=-$N > /dev/full 2> errt || fail=1 sed 's/\(head:.*\):.*/\1/' errt > err compare exp err || fail=1 # seekable case - timeout 10s head --$item=-$N bigseek > /dev/full 2> errt && fail=1 - test $? = 124 && fail=1 + returns_ 1 timeout 10s head --$item=-$N bigseek > /dev/full 2> errt \ + || fail=1 sed 's/\(head:.*\):.*/\1/' errt > err compare exp err || fail=1 done diff --git a/tests/misc/nice.sh b/tests/misc/nice.sh index 85523eb..36d6f91 100755 --- a/tests/misc/nice.sh +++ b/tests/misc/nice.sh @@ -79,8 +79,7 @@ if test x$(nice -n -1 nice 2> /dev/null) = x0 ; then compare exp err || fail=1 # Failure to write advisory message is fatal. Buggy through coreutils 8.0. if test -w /dev/full && test -c /dev/full; then - nice -n -1 nice > out 2> /dev/full - test $? = 125 || fail=1 + returns_ 125 nice -n -1 nice > out 2> /dev/full || fail=1 compare /dev/null out || fail=1 fi else diff --git a/tests/misc/nohup.sh b/tests/misc/nohup.sh index ad60185..e9c8c38 100755 --- a/tests/misc/nohup.sh +++ b/tests/misc/nohup.sh @@ -70,8 +70,7 @@ if test -w /dev/full && test -c /dev/full; then exec >/dev/tty test -t 1 || exit 0 - nohup echo hi 2> /dev/full - test $? = 125 || fail=1 + returns_ 125 nohup echo hi 2> /dev/full || fail=1 test -f nohup.out || fail=1 compare /dev/null nohup.out || fail=1 rm -f nohup.out @@ -118,9 +117,7 @@ EOF # Make sure it fails with exit status of 125 when given too few arguments, # except that POSIX requires 127 in this case. -nohup >/dev/null 2>&1 -test $? = 125 || fail=1 -POSIXLY_CORRECT=1 nohup >/dev/null 2>&1 -test $? = 127 || fail=1 +returns_ 125 nohup >/dev/null 2>&1 || fail=1 +POSIXLY_CORRECT=1 returns_ 127 nohup >/dev/null 2>&1 || fail=1 Exit $fail diff --git a/tests/misc/stdbuf.sh b/tests/misc/stdbuf.sh index 7b9aed5..31f02b7 100755 --- a/tests/misc/stdbuf.sh +++ b/tests/misc/stdbuf.sh @@ -43,19 +43,17 @@ stdbuf -o1 true || fail=1 # verify size syntax stdbuf -oK true || fail=1 # verify size syntax stdbuf -o0 true || fail=1 # verify unbuffered syntax stdbuf -oL true || fail=1 # verify line buffered syntax -stdbuf -ol true # Capital 'L' required -test $? = 125 || fail=1 # Internal error is a particular status -stdbuf -o$SIZE_OFLOW true # size too large -test $? = 125 || fail=1 -stdbuf -iL true # line buffering stdin disallowed -test $? = 125 || fail=1 -stdbuf true # a buffering mode must be specified -test $? = 125 || fail=1 + +# Capital 'L' required +# Internal error is a particular status +returns_ 125 stdbuf -ol true || fail=1 + +returns_ 125 stdbuf -o$SIZE_OFLOW true || fail=1 # size too large +returns_ 125 stdbuf -iL true || fail=1 # line buffering stdin disallowed +returns_ 125 stdbuf true || fail=1 # a buffering mode must be specified stdbuf -i0 -o0 -e0 true || fail=1 #check all files -stdbuf -o1 . # invalid command -test $? = 126 || fail=1 -stdbuf -o1 no_such # no such command -test $? = 127 || fail=1 +returns_ 126 stdbuf -o1 . || fail=1 # invalid command +returns_ 127 stdbuf -o1 no_such || fail=1 # no such command # Terminate any background processes cleanup_() { kill $pid 2>/dev/null && wait $pid; } diff --git a/tests/misc/sync.sh b/tests/misc/sync.sh index 5bf569c..cd89c5b 100755 --- a/tests/misc/sync.sh +++ b/tests/misc/sync.sh @@ -45,8 +45,7 @@ fi if test "$fail" != '1'; then # Ensure a fifo doesn't block mkfifo_or_skip_ fifo - timeout 10 sync fifo - test $? = 124 && fail=1 + returns_ 124 timeout 10 sync fifo && fail=1 fi Exit $fail diff --git a/tests/tail-2/pid.sh b/tests/tail-2/pid.sh index 9e73b84..ba67b46 100755 --- a/tests/tail-2/pid.sh +++ b/tests/tail-2/pid.sh @@ -31,8 +31,7 @@ for mode in '' '---disable-inotify'; do tail -f $mode here & pid=$! # Ensure that tail --pid=PID does not exit when PID is alive. - timeout 1 tail -f -s.1 --pid=$pid $mode here - test $? = 124 || fail=1 + returns_ 124 timeout 1 tail -f -s.1 --pid=$pid $mode here || fail=1 cleanup_ @@ -44,8 +43,7 @@ for mode in '' '---disable-inotify'; do test $ret = 0 || fail=1 # Ensure tail doesn't wait for data when PID is dead - timeout 10 tail -f -s10 --pid=$PID_T_MAX $mode empty - test $? = 124 && fail=1 + returns_ 124 timeout 10 tail -f -s10 --pid=$PID_T_MAX $mode empty && fail=1 done Exit $fail diff --git a/tests/tail-2/wait.sh b/tests/tail-2/wait.sh index 59f796a..1b54327 100755 --- a/tests/tail-2/wait.sh +++ b/tests/tail-2/wait.sh @@ -35,29 +35,23 @@ cleanup_() { kill $pid 2>/dev/null && wait $pid; } fastpoll='-s.1 --max-unchanged-stats=1' for mode in '' '---disable-inotify'; do - timeout 10 tail $fastpoll -f $mode not_here - test $? = 124 && fail=1 + returns_ 124 timeout 10 tail $fastpoll -f $mode not_here && fail=1 if test ! -r unreadable; then # can't test this when root - timeout 10 tail $fastpoll -f $mode unreadable - test $? = 124 && fail=1 + returns_ 124 timeout 10 tail $fastpoll -f $mode unreadable && fail=1 fi - timeout .1 tail $fastpoll -f $mode here 2>tail.err - test $? = 124 || fail=1 + returns_ 124 timeout .1 tail $fastpoll -f $mode here 2>tail.err || fail=1 # 'tail -F' must wait in any case. - timeout .1 tail $fastpoll -F $mode here 2>>tail.err - test $? = 124 || fail=1 + returns_ 124 timeout .1 tail $fastpoll -F $mode here 2>>tail.err || fail=1 if test ! -r unreadable; then # can't test this when root - timeout .1 tail $fastpoll -F $mode unreadable - test $? = 124 || fail=1 + returns_ 124 timeout .1 tail $fastpoll -F $mode unreadable || fail=1 fi - timeout .1 tail $fastpoll -F $mode not_here - test $? = 124 || fail=1 + returns_ 124 timeout .1 tail $fastpoll -F $mode not_here || fail=1 grep -Ev "$inotify_failed_re" tail.err > x mv x tail.err -- 2.9.3
