Jeff King [off-list ref] writes:
So my perspective was the opposite of yours: "return" is the officially
sanctioned way to exit early from a test snippet, and "exit" only
happens to work because of the undocumented fact that lazy prereqs
happen in a subshell. But it turns out neither was documented. :)
Good miniproject to document them, I presume. A rough draft
attached. I do not mind adding "exit 1 also works in this and that
case, but not in that other case" if the rule can be given succinct
enough, but I opted for simplicity (mostly because I couldn't come
up with such a clear rule for "exit 1").
As long as we are consciouly ensuring that "return 1" consistently
works everywhere, I didn't see much point offering multiple ways to
do the same thing.
Yes. Part of the reason I kept mine as it was is because it matched the
original behavior better (and I was really only using a lazy prereq
because we didn't have a non-lazy version). But there's really no reason
_not_ to be lazy, so as long as it isn't breaking anything I think
that's a better way forward. (And if it is breaking something, that
something should be fixed).
Yup, I too liked that part of Dscho's version better.
-- >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.
Signed-off-by: Junio C Hamano <redacted>
---
* 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.
t/README | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 55 insertions(+)
diff --git a/t/README b/t/README
index 369e3a9ded..08c8d00bb6 100644
--- a/t/README
+++ b/t/README
@@ -546,6 +546,61 @@ Here are the "do's:"
reports "ok" or "not ok" to the end user running the tests. Under
--verbose, they are shown to help debug the tests.
+ - 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
+ '
+
+ 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.
+
+ You can work it around in two ways. You could use a separate
+ "flag" variable to carry the failed state out of the loop:
+
+ test_expect_success 'git frotz on various versions' '
+ failed=
+ for revision in one two three
+ do
+ echo "frotz $revision" >expect &&
+ git frotz "$revision" >actual &&
+ test_cmp expect actual ||
+ failed="$failed${failed:+ }$revision"
+ done &&
+ test -z "$failed" &&
+ test something else
+ '
+
+ Or you can fail the entire loop immediately when you see the
+ first failure by using "return 1" from inside the loop, like so:
+
+ 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 || return 1
+ done &&
+ test something else
+ '
+
+ Note that this is only possible in our test suite, where we
+ arrange to run your test <script> wrapped inside a helper
+ function to ensure that return values matter; in your own script
+ outside any function, this technique may not work.
+
+
And here are the "don'ts:"
- Don't exit() within a <script> part.On Fri, Mar 27, 2020 at 1:44 PM Junio C Hamano [off-list ref] wrote:
Subject: [PATCH] t/README: suggest how to leave test early with failure
+ 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 || return 1
+ done &&
+ test something else
+ '
+
+ Note that this is only possible in our test suite, where we
+ arrange to run your test <script> wrapped inside a helper
+ function to ensure that return values matter; in your own script
+ outside any function, this technique may not work.
+
And here are the "don'ts:"
- Don't exit() within a <script> part.
We use 'exit 1' to terminate subshells[1] inside tests.
[1]: https://lore.kernel.org/git/20150325052952.GE31924@peff.net/
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