Re: test &&-chain lint

[Jeff King <peff@peff.net>]

On Fri, Mar 20, 2015 at 10:04:43AM -0700, Junio C Hamano wrote:

> One case where this might misdetect a good test would be this one:
> 
>     test_expect_success 'either succeed or fail with status 1' '
> 	git subcmd || case "$?" in 1) : happy ;; *) false failure ;; esac
>     '

Yes. Any use of "||" is going to cause problems. See the conversions in
the series I posted a few hours ago. The solution is to adapt the test
style to encase any "||" inside a block. I don't like having to modify
the tests to match our lint check, but it is a fairly rare case, and the
modification is not complex. And in fact I think it often highlights to
the reader that something funny is going on; a snippet like the one
above is OK to run at the start of a test, but:

  foo &&
  bar ||
  case "$?"
   ...

is potentially dangerous. You do not know if you are checking the $? of
"foo" or "bar" in that case statement.

Your case above is actually better spelled as test_expect_code, but
there are more complex one-off cases that I solved using a {} block.

> I wonder if another valid way to make it harder for us to commit
> "broken && chain" errors in our test may be to make it not an error
> in the first place.  Do we know how buggy various implementations of
> shells are with respect to their handling of "set -e"?

Certainly that is a thought that occurred to me while writing the
series. At GitHub, we have a shell-based test harness for internal
projects that is somewhat like Git's t/test-lib.sh, but uses "-e" to
catch errors.

It's mostly OK, but there are some surprising bits. For instance, try
this sequence of snippets:

     set -e
     false
     echo should not reach

That should print nothing. So far so good. How about in a subshell:

     set -e
     (
       false
       echo 1
     )
     echo 2

That also prints nothing. Good. But "set -e" is suppressed in a
conditional or in the presence of logic operators like "||".  So you'd
expect this:

     set -e
     (
       false
       echo 1
     ) || {
         echo outcome=$?
         false
     }
     echo 2

to break out of the subshell, print the failed outcome, and then exit.
But it doesn't. It prints "1" and "2". The suppression of "set -e"
continues inside the subshell, even though the inner commands are not
part of their own conditionals.

OK, so when we open a subshell we have to re-iterate our desire for "set
-e":

    set -e
    (
      set -e
      false
      echo 1
    ) || {
        echo outcome=$?
	false
    }
    echo 2

Nope, does nothing, at least in bash and dash. The ||-operator
suppression is clearly not "turn off set -e", but "globally make -e
useless inside here".

So you end up having to use manual exits to jump out of the subshell,
like:

     set -e
     (
       false || exit 1
       echo 1
     ) || {
         echo outcome=$?
         false
     }
     echo 2


That snippet produces "outcome=1", which is what I expect.

Of course most tests are not so complicated to have subshells with
conditional operators on the side. But I did not just type out that
example right now from scratch. I cut-and-pasted it from a real-world
commit message.

So I dunno. I think "set -e" is kind of a dangerous lure. It works so
well _most_ of the time that you start to rely on it, but it really does
have some funny corner cases (even on modern shells, and for all I know,
the behavior above is mandated by POSIX).

-Peff