From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>,
Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>,
git@vger.kernel.org
Subject: Re: [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs
Date: Sat, 28 Mar 2020 06:54:18 -0400 [thread overview]
Message-ID: <20200328105418.GA639140@coredump.intra.peff.net> (raw)
In-Reply-To: <xmqqr1xdhev8.fsf@gitster.c.googlers.com>
On Fri, Mar 27, 2020 at 10:44:27AM -0700, Junio C Hamano wrote:
> -- >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.
> + - 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. :)
> 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
next prev parent reply other threads:[~2020-03-28 10:54 UTC|newest]
Thread overview: 55+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-23 13:09 [PATCH 0/2] Enable GPG in the Windows part of the CI/PR builds Johannes Schindelin via GitGitGadget
2020-03-23 13:09 ` [PATCH 1/2] tests(gpg): allow the gpg-agent to start on Windows Johannes Schindelin via GitGitGadget
2020-03-23 17:46 ` Junio C Hamano
2020-03-24 19:55 ` Johannes Schindelin
2020-03-24 20:59 ` Junio C Hamano
2020-03-24 22:26 ` Johannes Schindelin
2020-03-24 23:40 ` Junio C Hamano
2020-03-23 13:09 ` [PATCH 2/2] tests(gpg): increase verbosity to allow debugging Johannes Schindelin via GitGitGadget
2020-03-23 17:32 ` Jeff King
2020-03-23 18:04 ` Jeff King
2020-03-23 19:21 ` Junio C Hamano
2020-03-23 20:15 ` Jeff King
2020-03-23 21:28 ` Junio C Hamano
2020-03-23 21:31 ` Jeff King
2020-03-24 21:41 ` Johannes Schindelin
2020-03-24 22:05 ` Jeff King
2020-03-24 22:25 ` Johannes Schindelin
2020-03-24 22:33 ` Jeff King
2020-03-25 5:41 ` [PATCH v2 0/5] Enable GPG in the Windows part of the CI/PR builds Johannes Schindelin via GitGitGadget
2020-03-25 5:41 ` [PATCH v2 1/5] tests(gpg): allow the gpg-agent to start on Windows Johannes Schindelin via GitGitGadget
2020-03-25 5:41 ` [PATCH v2 2/5] t/lib-gpg.sh: stop pretending to be a stand-alone script Johannes Schindelin via GitGitGadget
2020-03-26 8:21 ` Jeff King
2020-03-26 13:48 ` Johannes Schindelin
2020-03-26 19:31 ` Junio C Hamano
2020-03-25 5:41 ` [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs Johannes Schindelin via GitGitGadget
2020-03-25 17:25 ` Junio C Hamano
2020-03-26 8:35 ` Jeff King
2020-03-26 14:27 ` Johannes Schindelin
2020-03-27 9:10 ` Jeff King
2020-03-27 17:44 ` Junio C Hamano
2020-03-27 20:24 ` Eric Sunshine
2020-03-27 21:37 ` Junio C Hamano
2020-03-28 10:58 ` Jeff King
2020-03-28 10:54 ` Jeff King [this message]
2020-03-28 23:49 ` [PATCH v2] t/README: suggest how to leave test early with failure Junio C Hamano
2020-03-29 7:23 ` Eric Sunshine
2020-03-29 14:33 ` Jeff King
2020-03-30 18:39 ` [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs Johannes Schindelin
2020-03-31 9:34 ` Jeff King
2020-03-25 5:41 ` [PATCH v2 4/5] tests: do not let lazy prereqs inside `test_expect_*` turn off tracing Johannes Schindelin via GitGitGadget
2020-03-25 17:23 ` Junio C Hamano
2020-03-26 13:45 ` Johannes Schindelin
2020-03-26 8:49 ` Jeff King
2020-03-26 14:34 ` Johannes Schindelin
2020-03-25 5:41 ` [PATCH v2 5/5] tests: increase the verbosity of the GPG-related prereqs Johannes Schindelin via GitGitGadget
2020-03-26 8:50 ` Jeff King
2020-03-26 14:36 ` Johannes Schindelin
2020-03-26 15:35 ` [PATCH v3 0/5] Enable GPG in the Windows part of the CI/PR builds Johannes Schindelin via GitGitGadget
2020-03-26 15:35 ` [PATCH v3 1/5] tests(gpg): allow the gpg-agent to start on Windows Johannes Schindelin via GitGitGadget
2020-03-26 15:35 ` [PATCH v3 2/5] t/lib-gpg.sh: stop pretending to be a stand-alone script Johannes Schindelin via GitGitGadget
2020-03-26 15:35 ` [PATCH v3 3/5] tests: do not let lazy prereqs inside `test_expect_*` turn off tracing Johannes Schindelin via GitGitGadget
2020-03-26 15:35 ` [PATCH v3 4/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs Johannes Schindelin via GitGitGadget
2020-03-26 15:35 ` [PATCH v3 5/5] tests: increase the verbosity of the GPG-related prereqs Johannes Schindelin via GitGitGadget
2020-03-27 9:12 ` [PATCH v3 0/5] Enable GPG in the Windows part of the CI/PR builds Jeff King
2020-03-27 17:45 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200328105418.GA639140@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=Johannes.Schindelin@gmx.de \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).