Re: [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs
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_elseThe 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_elsePossibly 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
Johannes Schindelin via GitGitGadget
