git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
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
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

  parent reply index

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

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git