(+cc: Ævar in case he is interested)
Hi,
Jeff King wrote:
> So I dunno. Most of it I am fine with, though I question whether it is
> really worth the effort. But I really don't want to be too draconian
> about "everything must go into test_expect_success".
Thanks for the comments; that is very useful. I'm sure you won't
be surprised to hear that that is a part I am more attached to than
the () vs {}, say. I think I did not explain it well.
As you mentioned, it is a big departure from the style of the existing
tests. So why push it? A quick story:
1. Long ago, when I first debugged a test script with -v, I was a bit
confused because the transcript did not tell the whole story
(because some commands are run outside test assertions). No big
deal, but I remembered it.
2. Sometimes the setup commands outside of test scripts produce
output. This is annoying, so people silence it.
3. Sometimes the setup commands outside of test scripts are broken.
Tests do not use "set -e" or check for errors outside of test
assertions, so simple typos can go undetected for a long time.
4. What actually provoked me to care about it: when trying to add a
test to t9301-fast-import.sh, say, I found myself completely lost.
It is really hard to figure out what the state is supposed to be
at a particular point in the test script. Sometimes I am tempted
to write a new test script when adding a new behavior, only
because I do not understand the existing one on a topic. All the
tests can be well-behaved and follow sane invariants, but that
does not matter, because the invariants are not documented anywhere.
How to fix that? I would like to see tests look roughly like this:
test_description='description of purpose
description of state maintained between tests
'
. ./test-lib.sh
test_expect_success 'setup' '
...
'
test_expect_success 'some good thing holds' '
... commands that do not break global state ...
'
test_expect_success 'another good thing holds' '
... more peaceful test commands ...
'
test_expect_success 'setup: update global state somehow' '
...
'
...
test_done
Some of my other puzzling patches (test_might_fail, test_when_finished)
are also meant for this purpose. Of course, it will take a while.
The result would be:
- test commands all shown with "-v", output all suppressed without;
- all commands pass at least the sanity check of exiting with 0
status;
- easy to write a GIT_SKIP_TESTS specification. Would be possible
to add the ability to try a single test (plus all setup tests in
that script that precede it);
- as long as all the setup tests pass, the list of failed tests
from a test failure can be more informative;
- state can be tracked by just reading the setup tests.
> Sure, if you are
> executing commands that might have output, or might be of interest to
> the user, put them there. But I find this a lot more readable:
>
> cat >expect <<'EOF'
> ... some expected output ...
> EOF
> test_expect_success 'frob it' '
I don't know: I think
cat >expect <<-\EOF &&
...
EOF
is pretty readable. The problem with sticking to
cat >expect <<\EOF
...
EOF
is that once someone wants to include a commit id, they change
it to
cat >expect <<EOF
... $(git rev-parse ... )
...
EOF
and we have nontrivial code outside the test now.
On the other hand:
> , but IMO you are
> making at least one of them less readable, because you have to deal with
> the extra quoting layer of test_expect_success. IOW:
>
>> -SECTION="test.q\"s\\sq'sp e.key"
>> test_expect_success 'make sure git config escapes section names properly' '
>> + SECTION="test.q\"s\\sq'\''sp e.key" &&
>
> seems like a net loss to me.
I wholeheartedly agree, and maybe I should have done
apos="'\''" &&
SECTION="test.q\"s\\sq${apos}sp e.key" &&
or something; this detail of quoting has never brought much happiness
to me.
Sorry for the ramble. Thoughts welcome.
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html