guarding everything with test_expect_success (Re: [PATCH 6/7] t1303 (config): style tweaks)

[Jonathan Nieder <jrnieder@gmail.com>]

(+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