Skip to content

Commit

Permalink
shelltest.od: add more "set -e" tests and add a 'skip' return code.
Browse files Browse the repository at this point in the history
Based on the earlier t/000-set-minus-e bug in minimal/do on some
shells, let's add some extra tests that reveal the weirdness on those
shells.  Unfortunately because they are so popular (including bash and
zsh), we can't reject them outright for failing this one.

While we're here, add a new return code, "skip", which notes that a
test has failed but is not important enough to be considered a warning
or failure.  Previously we just had these commented out, which is not
quite obvious enough.

...and I updated a few comments while reviewing some of the older
tests.
  • Loading branch information
apenwarr committed Dec 17, 2018
1 parent 761b773 commit 1f64cc4
Show file tree
Hide file tree
Showing 5 changed files with 79 additions and 18 deletions.
2 changes: 2 additions & 0 deletions redo/sh.do
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,11 @@ for sh in dash /usr/xpg4/bin/sh ash posh \
#echo "line: '$line'" >&2
stripw=${line#warning: }
stripf=${line#failed: }
strips=${line#skip: }
crash=$line
[ "$line" = "$stripw" ] || msgs="$msgs W$stripw"
[ "$line" = "$stripf" ] || msgs="$msgs F$stripf"
[ "$line" = "$strips" ] || msgs="$msgs s$strips"
done <shelltest.tmp
rm -f shelltest.tmp
msgs=${msgs# }
Expand Down
1 change: 1 addition & 0 deletions t/.gitignore
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/broken
/shellfile
/shellfail
/shelltest.warned
/shelltest.failed
/stress.log
Expand Down
1 change: 1 addition & 0 deletions t/000-set-minus-e/all.do
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
redo-ifchange ../../redo/sh
rm -f log
redo fatal >/dev/null 2>&1 || true

Expand Down
2 changes: 1 addition & 1 deletion t/clean.do
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,6 @@
sed 's/\.do$//' |
xargs redo

rm -f broken shellfile shelltest.warned shelltest.failed \
rm -f broken shellfile shellfail shelltest.warned shelltest.failed \
*~ .*~ stress.log flush-cache
rm -rf 'space home dir'
91 changes: 74 additions & 17 deletions t/shelltest.od
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,12 @@ warn()
: >shelltest.warned
}

# For things that should be a warning, but which are just too common
skip()
{
echo " skip: $1"
}

quiet_stderr()
{
if [ -n "$SHELLTEST_QUIET" ]; then
Expand Down Expand Up @@ -185,17 +191,20 @@ c"
[ "$t2" = "$WANT" ] || fail 45


# Arguably, 'export' and 'local' shouldn't change variable assignment quoting
# rules, but in almost all shells (except bash), they do, and POSIX doesn't say
# anything about it. So let's not bother testing this, other than just letting
# it check our syntax.
#
# Arguably, 'export' and 'local' shouldn't change variable assignment
# quoting rules, but in many shells they do, and POSIX doesn't say anything
# about it. This leads to really confusing whitespace bugs in some shells,
# where
# local x; x=$a
# and
# local x=$a
# mean two different things when $a contains whitespace.
bob="a b *"
bob=$(quiet_stderr eval 'export bob=$bob:hello'; echo "$bob")
#[ "$bob" = "a b *:hello" ] || warn 46
[ "$bob" = "a b *:hello" ] || skip 46
bob="a b *"
nob=$(eval 'f() { local nob=$bob:hello; echo "$nob"; }'; quiet_stderr f)
#[ "$nob" = "a b *:hello" ] || warn 47
[ "$nob" = "a b *:hello" ] || skip 47


# Someone pointed out that temporary variable assignments aren't
Expand Down Expand Up @@ -247,14 +256,14 @@ echo "`printf 'foo\r\n'`"" bar" | diff -q - broken || fail 59


#
# This one is too obnoxious. dash and ash pass the test, but most shells don't,
# and this case is just too dumb to care about. Just don't do that!
# This one is too obnoxious. dash and ash pass the test, but most shells
# don't, and this case is just too dumb to care about. Just don't do that!
#
#t=`echo $(case x in x) echo hello;; esac)`
#[ "$t" = "hello" ] || fail 60
t=`echo $(case x in x) echo hello;; esac)`
[ "$t" = "hello" ] || skip 60
#
# Note that with the little-known optional left-paren, this *does* work. Let's
# try it to make sure that remains true.
# Note that with the little-known optional left-paren, this *does* work.
# Let's try it to make sure that remains true.
t=`echo $(case x in (x) echo hello;; esac)`
[ "$t" = "hello" ] || fail 60

Expand Down Expand Up @@ -328,6 +337,53 @@ rv=$?
[ "$rv" = 0 ] || fail 89


rm -f shellfail
echo "false; true" >shellfail

# 'false' should *not* abort because outer context has "||".
# Then 'true' gives success.
set +e
(
set -e
false
true
) || fail 89a

# 'false' still should not abort even in a subshell.
set +e
(
set -e
( false; true )
[ "$?" = 0 ] || fail 89b1
true
) || fail 89b

# Check whether "." scripts inherit "set -e" setting *and* the outer "||"
# context.
set +e
(
set -e
( . ./shellfail )
# There seems to be widespread disagreement on what should happen
# here. All shells seem to inherit "set -e" into a "." script, but
# they disagree about whether the outer "||" context (which
# effectively disables "set -e") should be inherited. ksh93, bash,
# and zsh inherit it, while ash, dash, and mksh (among others) throw
# it away, causing "set -e" to abort.
#
# Honestly, the latter behaviour seems a lot more like what I'd
# expect (a separate script doesn't want its "set -e" behaviour
# to depend on whether it's deep in a context with an "||" somewhere
# outside it), so let's warn on the former. But I'm willing to hear
# arguments either way. -- apenwarr
[ "$?" = 0 ] && warn 89c
# even if "." aborted and returned nonzero, our own context should
# not have aborted, so we expect to get here and avoid the outer
# fail below.
true
) || fail 89c1


# http://www.gnu.org/software/hello/manual/autoconf/Limitations-of-Builtins.html
. /dev/null || fail 90
(! : | :) && fail 91 || true
Expand All @@ -338,7 +394,7 @@ case frog.c in
(*) t3=all ;;
esac
[ "$t3" = "c" ] || fail 93
t4=$(echo '\n' | wc -l)
t4=$(echo '\n' | wc -l) # echo should *not* be parsing backslash-escapes
[ "$t4" -eq 1 ] || warn 94
f5() {
for arg; do
Expand Down Expand Up @@ -400,9 +456,10 @@ false
# this is actually a bash/kshism, but is allowed by POSIX: the parameters to
# '.' should be passed to the sub-script.
#
# We used to require this, because it's so useful despite being optional in POSIX.
# But unfortunately, too many shells (including dash) can't do it, so we ended up
# always using bash, which leads people to write .do scripts with bashisms.
# We used to require this, because it's so useful despite being optional in
# POSIX. But unfortunately, too many shells (including dash) can't do it,
# so we ended up always using bash, which leads people to write .do scripts
# with bashisms.
set x y z
# dotparams.od might warn 115
. ./dotparams.od a b || fail 117
Expand Down

0 comments on commit 1f64cc4

Please sign in to comment.