Re: test &&-chain lint

From: Jeff King <peff@peff.net>
Date: Mar 20, 2015, 17:24 UTC
Message-ID: <20150320172406.GA15172@peff.net>
In-Reply-To: <xmqq384zha6s.fsf@gitster.dls.corp.google.com>

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

Show 5 quoted lines

> 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