Re: [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs

From: Jeff King <peff@peff.net>
Date: Mar 28, 2020, 10:54 UTC
Message-ID: <20200328105418.GA639140@coredump.intra.peff.net>
In-Reply-To: <xmqqr1xdhev8.fsf@gitster.c.googlers.com>

On Fri, Mar 27, 2020 at 10:44:27AM -0700, Junio C Hamano wrote:

Show 6 quoted lines

> -- >8 --
> Subject: [PATCH] t/README: suggest how to leave test early with failure
> 
> Over time, we added the support to our test framework to make it
> easy to leave a test early with failure, but it was not clearly
> documented in t/README to help developers writing new tests.

Thanks for getting started on this. Everything you wrote looks correct, but I think we can be more succinct. And I think it's worth being so, since there are a lot of "do's" already, and we don't want to overwhelm the user.

Show 14 quoted lines

> + - Be careful when you loop
> +
> +   You may need to test multiple things in a loop, but the following
> +   does not work correctly:
> +
> +	test_expect_success 'git frotz on various versions' '
> +	    for revision in one two three
> +	    do
> +		    echo "frotz $revision" >expect &&
> +		    git frotz "$revision" >actual &&
> +		    test_cmp expect actual
> +	    done &&
> +	    test something else
> +	'

We could shrink this example down to the bare minimum. Perhaps:

  for i in a b c; do
    do_something "$i"
  done &&
  do_something_else

The key thing is that the "&&" will pick up only the value of "do_something $c" and will ignore "a" and "b". That might be too dense, but it does reduce any cognitive burden from unimportant details.

> +   If the output from the "git frotz" command did not match what is
> +   expected for 'one' and 'two', but it did for 'three', the loop
> +   itself would not fail, and the test goes on happily.  This is not
> +   what you want.

This explanation works fine, though I think you can also explain it as:

  The result code of a shell loop is the result of the final iteration;
  the results of do_something for "a" and "b" are discarded, and we'd
  pass the test even if they fail.

(I'm happy with either, just thinking out loud).

> +   You can work it around in two ways.  You could use a separate
> +   "flag" variable to carry the failed state out of the loop:

IMHO it's not worth giving this alternative. It's perfectly valid, but we promise the "return" version works exactly so we don't have to deal deal with this ugly and error-prone code.

> +    Or you can fail the entire loop immediately when you see the
> +    first failure by using "return 1" from inside the loop, like so:

I think we can jump straight to "in our test suite", like:

  One way around this is to break out of the loop when we see a failure.
  All test_expect_* snippets are executed inside a function, allowing an
  immediate return on failure:

    for i in a b c; do
      do_something "$i" || return 1
    done &&
    do_something_else

Possibly we'd also want to say:

  Note that we still &&-chain the loop to propagate failures from
  earlier commands.

but certainly the &&-chain linter would remind them of that. :)

Show 13 quoted lines

>  And here are the "don'ts:"
>  
>   - Don't exit() within a <script> part.
>  * A tangent.  441ee35d (t/README: reformat Do, Don't, Keep in mind
>    lists, 2018-10-05) added these lines
> 
>     Here are the "do's:"
>     And here are the "don'ts:"
> 
>    Is the placement of the colons on these lines right?  I am
>    tempted to chase them out of the dq pair and move them at the end
>    of their lines, but obviously that is outside of the scope of
>    this patch.

I think it's a matter of taste. Most writing style guides put punctuation inside quotes, but they're also expecting the output to be typeset, where a trailing period ends up more under the quotes than beside. I'm not sure what such style guides would say about colons, as they're a bit taller.

But regardless, I actually prefer putting punctuation outside of the quotes. It looks better to me in a fixed-width terminal setting. Plus I guess as a programmer it feels like a parsing error. ;)

I don't know that it matters too much either way, though.

-Peff