Re: [PATCH] tests: add extra protection against unexpected exits

[Bernhard Voelker]

On 01/14/2015 02:51 AM, Pádraig Brady wrote:
> On 13/01/15 08:13, Bernhard Voelker wrote:
>>   expectExit 1 program ... || fail=1
>
> Very good suggestions.  I implemented the simplification wrapper
> (I called it returns_), and that in turn made a syntax check feasible.

great, 'returns_' is a much better name, thanks.

> From 92288f467759f84674bf00d2ffe8e4419347f46c Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?P=C3=A1draig=20Brady?= <address@hidden>
> Date: Tue, 13 Jan 2015 03:30:33 +0000
> Subject: [PATCH] tests: add extra protection against unexpected exits

> --- a/cfg.mk
> +++ b/cfg.mk
> @@ -377,6 +377,17 @@ sc_prohibit_fail_0:
>       halt='fail=0 initialization'                                    \
>         $(_sc_search_regexp)
>  
> +# Ensure that tests don't use `cmd ... && fail=1` as that hides crashes.
> +# The "exclude" expression allows common idoms like `test ... && fail=1`

s/idoms/idioms/

> +# and the 2>... portion allows commands that redirect stderr and so probably
> +# independently check its contents and thus detect any crash messages.
> +sc_prohibit_and_fail_1:
> +     @prohibit='&& fail=1'                                           \
> +     exclude='(stat|kill|test |EGREP|grep|env|2> *[^/])'             \
> +     halt='&& fail=1 detected. Please use: returns_ 1 ... || fail=1' \
> +     in_vc_files='^tests/'                                           \
> +       $(_sc_search_regexp)
> +

Adding the 2> ... redirection may hide some of the matches, but is good
enough for the first approach.  In the long run, we should try to get
rid of all "&& fail=1", shouldn't we?  E.g. there are many superfluous
stderr redirections via "2>/dev/null" while in case of an error one would
have to remove it anyway to get the actual error message.

> --- a/tests/dd/misc.sh
> +++ b/tests/dd/misc.sh
> @@ -40,7 +40,7 @@ dd status=noxfer status=none if=$tmp_in of=/dev/null 2> err 
> || fail=1
>  compare /dev/null err || fail=1
>  # check later status=noxfer overrides earlier status=none
>  dd status=none status=noxfer if=$tmp_in of=/dev/null 2> err || fail=1
> -compare /dev/null err && fail=1
> +test -s err || fail=1

nice one!

> --- a/tests/id/context.sh
> +++ b/tests/id/context.sh
> @@ -26,7 +26,10 @@ id | grep context= >/dev/null || fail=1
>  
>  # Check with specified user, no context string should be present.
>  # But if the current user is nameless, skip this part.
> -id -nu > /dev/null \
> -  && id $(id -nu) | grep context= >/dev/null && fail=1
> +name=$(id -nu) || { test $? -ne 1 && fail=1; }
> +if test x"$name" != x; then

if test "$name"; then ...

;-)
http://www.pixelbeat.org/programming/shell_script_mistakes.html
well, there are many of theses in the tests ...

> +  id "$(id -nu)" > id_name || fail=1

we know "$(id -nu)" already:

  id "$name" ...

> +  grep context= >/dev/null id_name && fail=1
> +fi

file name after redirection looks odd to me, better this?

  grep context= id_name >/dev/null && fail=1

> --- a/tests/id/zero.sh
> +++ b/tests/id/zero.sh
> @@ -51,8 +51,10 @@ while read u ; do
>        printf '\n%s: ' "id -${o}${n}[z] $u" >> out || framework_failure_
>        # There may be no name corresponding to an id, so don't check
>        # exit status when in name lookup mode
> -      id -${o}${n}  $u >> exp || { test -z "$n" && fail=1; }
> -      id -${o}${n}z $u  > tmp || { test -z "$n" && fail=1; }
> +      id -${o}${n}  $u >> exp ||
> +        { test $? -ne 1 || test -z "$n" && fail=1; }
> +      id -${o}${n}z $u  > tmp ||
> +        { test $? -ne 1 || test -z "$n" && fail=1; }

looks complicated. What about this?

      id -${o}${n}  $u >> exp; test $? -eq 1 && test -z "$n" && fail=1
      id -${o}${n}z $u  > tmp; test $? -eq 1 && test -z "$n" && fail=1

> --- a/tests/init.sh
> +++ b/tests/init.sh
> @@ -93,6 +93,19 @@ skip_ () { warn_ "$ME_: skipped test: $@"; Exit 77; }
>  fatal_ () { warn_ "$ME_: hard error: $@"; Exit 99; }
>  framework_failure_ () { warn_ "$ME_: set-up failure: $@"; Exit 99; }
>  
> +# This is used to simplify checking of the return value
> +# which is useful when ensuring a command fails as desired.
> +# I.E. just doing `command ... &&fail=1` will not catch
> +# a segfault in command for example.  With this helper you
> +# instead check an explicit exit code like
> +#   returns_ 1 command ... || fail
> +returns_ () {
> +  local exp_exit="$1"
> +  shift
> +  "$@"
> +  test $? -eq $exp_exit
> +}

can't shift fail? ... maybe paranoia.

> --- a/tests/misc/chcon-fail.sh
> +++ b/tests/misc/chcon-fail.sh
> @@ -22,16 +22,16 @@ print_ver_ chcon
>  
>  
>  # neither context nor file
> -chcon 2> /dev/null && fail=1
> +returns_ 1 chcon 2> /dev/null || fail=1

just one example for unneeded stderr suppression.
Maybe we should do a cleanup for this later, as this is
another topic.

> --- a/tests/misc/env.sh
> +++ b/tests/misc/env.sh
> @@ -102,7 +102,7 @@ cat <<EOF > unlikely_name/also_unlikely || 
> framework_failure_
>  echo pass
>  EOF
>  chmod +x unlikely_name/also_unlikely || framework_failure_
> -env also_unlikely && fail=1
> +returns_ 127 env also_unlikely || fail=1
>  test x$(PATH=$PATH:unlikely_name env also_unlikely) = xpass || fail=1
>  test x$(env PATH="$PATH":unlikely_name also_unlikely) = xpass || fail=1
>  

nice one, too.  Now, we will notice when the exit code in
the program would change some day.

> --- a/tests/misc/selinux.sh
> +++ b/tests/misc/selinux.sh
> @@ -47,7 +47,7 @@ c=$(ls -l f|cut -c11); test "$c" = . || fail=1
>  # Copy with an invalid context and ensure it fails
>  # Note this may succeed when root and selinux is in permissive mode
>  if test "$(getenforce)" = Enforcing; then
> -  cp --context='invalid-selinux-context' f f.cp && fail=1
> +    returns_ 1 cp --context='invalid-selinux-context' f f.cp || fail=1

_____^^ wrong indentation ?

> --- a/tests/misc/wc-parallel.sh
> +++ b/tests/misc/wc-parallel.sh
> @@ -25,8 +25,9 @@ print_ver_ wc
>  # This will output at least 16KiB per process
>  # and start 3 processes, with 2 running concurrently,
>  # which triggers often on Fedora 11 at least.
> -(find tmp tmp tmp -type f | xargs -n2000 -P2 wc) |
> +(find tmp tmp tmp -type f | xargs -n2000 -P2 wc 2>err) |
>  sed -n '/0 0 0 /!p' |
>  grep . > /dev/null && fail=1
> +compare /dev/null err || fail=1

nice!

> --- a/tests/mv/trailing-slash.sh
> +++ b/tests/mv/trailing-slash.sh
> @@ -48,14 +48,14 @@ done
>  # underlying rename syscall handles the trailing slash.
>  # It does fail, as desired, on recent Linux and Solaris systems.
>  #touch a a2
> -#mv a a2/ && fail=1
> +#returns_ 1 mv a a2/ || fail=1
>  
>  # Test for a cp-specific diagnostic introduced after coreutils-8.7:
>  printf '%s\n' \
>    "cp: cannot create regular file 'no-such/': Not a directory" \
>  > expected-err
>  touch b
> -cp b no-such/ 2> err && fail=1
> +cp b no-such/ 2> err

forgot

   s/^/returns_ 1 /
   s/$/|| fail=1/

here?

> --- a/tests/split/fail.sh
> +++ b/tests/split/fail.sh
> @@ -24,13 +24,13 @@ touch in || framework_failure_
>  
>  
>  split -a 0 in 2> /dev/null || fail=1
> -split -b 0 in 2> /dev/null && fail=1
> -split -C 0 in 2> /dev/null && fail=1
> -split -l 0 in 2> /dev/null && fail=1
> -split -n 0 in 2> /dev/null && fail=1
> -split -n 1/0 in 2> /dev/null && fail=1
> -split -n 0/1 in 2> /dev/null && fail=1
> -split -n 2/1 in 2> /dev/null && fail=1
> +returns_ 1 split -b 0 in 2> /dev/null || fail=1
> +returns_ 1 split -C 0 in 2> /dev/null || fail=1
> +returns_ 1 split -l 0 in 2> /dev/null || fail=1
> +returns_ 1 split -n 0 in 2> /dev/null || fail=1
> +returns_ 1 split -n 1/0 in 2> /dev/null || fail=1
> +returns_ 1 split -n 0/1 in 2> /dev/null || fail=1
> +returns_ 1 split -n 2/1 in 2> /dev/null || fail=1
>  
>  # Make sure -C doesn't create empty files.
>  rm -f x?? || fail=1
> @@ -42,21 +42,21 @@ test -f xac && fail=1
>  split -1 in 2> /dev/null || fail=1
>  
>  # Then make sure that -0 evokes a failure.
> -split -0 in 2> /dev/null && fail=1
> +returns_ 1 split -0 in 2> /dev/null || fail=1
>  
>  split --lines=$UINTMAX_MAX in || fail=1
>  split --bytes=$OFF_T_MAX in || fail=1
> -split --line-bytes=$OFF_T_OFLOW 2> /dev/null in && fail=1
> -split --line-bytes=$SIZE_OFLOW 2> /dev/null in && fail=1
> +returns_ 1 split --line-bytes=$OFF_T_OFLOW 2> /dev/null in || fail=1
> +returns_ 1 split --line-bytes=$SIZE_OFLOW 2> /dev/null in || fail=1
>  if truncate -s$SIZE_OFLOW large; then
>    # Ensure we can split chunks of a large file on 32 bit hosts
>    split --number=$SIZE_OFLOW/$SIZE_OFLOW large >/dev/null || fail=1
>  fi
>  split --number=r/$UINTMAX_MAX/$UINTMAX_MAX </dev/null >/dev/null || fail=1
> -split --number=r/$UINTMAX_OFLOW </dev/null 2>/dev/null && fail=1
> +returns_ 1 split --number=r/$UINTMAX_OFLOW </dev/null 2>/dev/null || fail=1
>  
>  # Make sure that a huge obsolete option evokes the right failure.
> -split -99999999999999999991 2> out && fail=1
> +split -99999999999999999991 2> out

although the content of out is checked here, it wouldn't hurt
to assert $? = 1.

Looks much nicer - although the review was still long enough. ;-)
Nice work, thanks!

Have a nice day,
Berny