[PATCH v4 0/7] Improvements for t/README

[PATCH v4 0/7] Improvements for t/README

[Ævar Arnfjörð Bjarmason]

Since last time:

 * Dropped the s[sh ./][./] patch
 * Typo fix: gleam -> glean

Here's the diff since v3:
    
    diff --git a/t/README b/t/README
    index 4d0526d..e481657 100644
    --- a/t/README
    +++ b/t/README
    @@ -55 +55 @@ You can also run each test individually from command line, like this:
    -    $ ./t3010-ls-files-killed-modified.sh
    +    $ sh ./t3010-ls-files-killed-modified.sh
    @@ -297 +297 @@ Don't:
    -   You can gleam some further possible issues from the TAP grammar
    +   You can glean some further possible issues from the TAP grammar

Ævar Arnfjörð Bjarmason (7):
  t/README: The trash is in 't/trash directory.$name'
  t/README: Typo: paralell -> parallel
  t/README: Document the prereq functions, and 3-arg test_*
  t/README: Document test_external*
  t/README: Document test_expect_code
  t/README: Add a section about skipping tests
  t/README: Document the do's and don'ts of tests

 t/README |  170 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 163 insertions(+), 7 deletions(-)

[PATCH v4 1/7] t/README: The trash is in 't/trash directory.$name'

[Ævar Arnfjörð Bjarmason]

There's a unique trash directory for each test, not a single directory
as the previous documentation suggested.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/README |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/t/README b/t/README
index aa4ed28..cad6fde 100644
--- a/t/README
+++ b/t/README
@@ -221,9 +221,10 @@ This test harness library does the following things:
  - If the script is invoked with command line argument --help
    (or -h), it shows the test_description and exits.
 
- - Creates an empty test directory with an empty .git/objects
-   database and chdir(2) into it.  This directory is 't/trash directory'
-   if you must know, but I do not think you care.
+ - Creates an empty test directory with an empty .git/objects database
+   and chdir(2) into it.  This directory is 't/trash
+   directory.$test_name_without_dotsh', with t/ subject to change by
+   the --root option documented above.
 
  - Defines standard test helper functions for your scripts to
    use.  These functions are designed to make all scripts behave
-- 
1.7.1.251.g92a7

[PATCH v4 2/7] t/README: Typo: paralell -> parallel

[Ævar Arnfjörð Bjarmason]

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/README |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/README b/t/README
index cad6fde..a7312fa 100644
--- a/t/README
+++ b/t/README
@@ -33,7 +33,7 @@ the tests.
     ok 3 - plain bare
 
 Since the tests all output TAP (see http://testanything.org) they can
-be run with any TAP harness. Here's an example of paralell testing
+be run with any TAP harness. Here's an example of parallel testing
 powered by a recent version of prove(1):
 
     $ prove --timer --jobs 15 ./t[0-9]*.sh
-- 
1.7.1.251.g92a7

[PATCH v4 3/7] t/README: Document the prereq functions, and 3-arg test_*

[Ævar Arnfjörð Bjarmason]

There was no documentation for the test_set_prereq and
test_have_prereq functions, or the three-arg form of
test_expect_success and test_expect_failure.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/README |   37 ++++++++++++++++++++++++++++++++++---
 1 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/t/README b/t/README
index a7312fa..c1fd092 100644
--- a/t/README
+++ b/t/README
@@ -246,9 +246,9 @@ Test harness library
 There are a handful helper functions defined in the test harness
 library for your script to use.
 
- - test_expect_success <message> <script>
+ - test_expect_success [<prereq>] <message> <script>
 
-   This takes two strings as parameter, and evaluates the
+   Usually takes two strings as parameter, and evaluates the
    <script>.  If it yields success, test is considered
    successful.  <message> should state what it is testing.
 
@@ -258,7 +258,14 @@ library for your script to use.
 	    'git-write-tree should be able to write an empty tree.' \
 	    'tree=$(git-write-tree)'
 
- - test_expect_failure <message> <script>
+   If you supply three parameters the first will be taken to be a
+   prerequisite, see the test_set_prereq and test_have_prereq
+   documentation below:
+
+	test_expect_success TTY 'git --paginate rev-list uses a pager' \
+	    ' ... '
+
+ - test_expect_failure [<prereq>] <message> <script>
 
    This is NOT the opposite of test_expect_success, but is used
    to mark a test that demonstrates a known breakage.  Unlike
@@ -267,6 +274,9 @@ library for your script to use.
    success and "still broken" on failure.  Failures from these
    tests won't cause -i (immediate) to stop.
 
+   Like test_expect_success this function can optionally use a three
+   argument invocation with a prerequisite as the first argument.
+
  - test_debug <script>
 
    This takes a single argument, <script>, and evaluates it only
@@ -299,6 +309,27 @@ library for your script to use.
    Merges the given rev using the given message.  Like test_commit,
    creates a tag and calls test_tick before committing.
 
+ - test_set_prereq SOME_PREREQ
+
+   Set a test prerequisite to be used later with test_have_prereq. The
+   test-lib will set some prerequisites for you, e.g. PERL and PYTHON
+   which are derived from ./GIT-BUILD-OPTIONS (grep test_set_prereq
+   test-lib.sh for more). Others you can set yourself and use later
+   with either test_have_prereq directly, or the three argument
+   invocation of test_expect_success and test_expect_failure.
+
+ - test_have_prereq SOME PREREQ
+
+   Check if we have a prerequisite previously set with
+   test_set_prereq. The most common use of this directly is to skip
+   all the tests if we don't have some essential prerequisite:
+
+	if ! test_have_prereq PERL
+	then
+	    skip_all='skipping perl interface tests, perl not available'
+	    test_done
+	fi
+
 Tips for Writing Tests
 ----------------------
 
-- 
1.7.1.251.g92a7

[PATCH v4 4/7] t/README: Document test_external*

[Ævar Arnfjörð Bjarmason]

There was do documentation for the test_external_without_stderr and
test_external functions.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/README |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/t/README b/t/README
index c1fd092..b40403d 100644
--- a/t/README
+++ b/t/README
@@ -330,6 +330,33 @@ library for your script to use.
 	    test_done
 	fi
 
+ - test_external [<prereq>] <message> <external> <script>
+
+   Execute a <script> with an <external> interpreter (like perl). This
+   was added for tests like t9700-perl-git.sh which do most of their
+   work in an external test script.
+
+	test_external \
+	    'GitwebCache::*FileCache*' \
+	    "$PERL_PATH" "$TEST_DIRECTORY"/t9503/test_cache_interface.pl
+
+   If the test is outputting its own TAP you should set the
+   test_external_has_tap variable somewhere before calling the first
+   test_external* function. See t9700-perl-git.sh for an example.
+
+	# The external test will outputs its own plan
+	test_external_has_tap=1
+
+ - test_external_without_stderr [<prereq>] <message> <external> <script>
+
+   Like test_external but fail if there's any output on stderr,
+   instead of checking the exit code.
+
+	test_external_without_stderr \
+	    'Perl API' \
+	    "$PERL_PATH" "$TEST_DIRECTORY"/t9700/test.pl
+
+
 Tips for Writing Tests
 ----------------------
 
-- 
1.7.1.251.g92a7

[PATCH v4 5/7] t/README: Document test_expect_code

[Ævar Arnfjörð Bjarmason]

test_expect_code (which was introduced in d3bfdb75) never had any
documentation.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/README |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/t/README b/t/README
index b40403d..04ad927 100644
--- a/t/README
+++ b/t/README
@@ -277,6 +277,13 @@ library for your script to use.
    Like test_expect_success this function can optionally use a three
    argument invocation with a prerequisite as the first argument.
 
+ - test_expect_code [<prereq>] <code> <message> <script>
+
+   Analogous to test_expect_success, but pass the test if it exits
+   with a given exit <code>
+
+ test_expect_code 1 'Merge with d/f conflicts' 'git merge "merge msg" B master'
+
  - test_debug <script>
 
    This takes a single argument, <script>, and evaluates it only
-- 
1.7.1.251.g92a7

[PATCH v4 6/7] t/README: Add a section about skipping tests

[Ævar Arnfjörð Bjarmason]

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/README |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/t/README b/t/README
index 04ad927..4079635 100644
--- a/t/README
+++ b/t/README
@@ -231,6 +231,18 @@ This test harness library does the following things:
    consistently when command line arguments --verbose (or -v),
    --debug (or -d), and --immediate (or -i) is given.
 
+Skipping tests
+--------------
+
+If you need to skip all the remaining tests you should set skip_all
+and immediately call test_done. The string you give to skip_all will
+be used as an explanation for why the test was skipped. for instance:
+
+	if ! test_have_prereq PERL
+	then
+	    skip_all='skipping perl interface tests, perl not available'
+	    test_done
+	fi
 
 End with test_done
 ------------------
-- 
1.7.1.251.g92a7

[PATCH v4 7/7] t/README: Document the do's and don'ts of tests

[Ævar Arnfjörð Bjarmason]

Add a "Do's, don'ts & things to keep in mind" subsection to the
"Writing Tests" documentation. Much of this is based on Junio C
Hamano's "Test your stuff" section in
<7vhbkj2kcr.fsf@alter.siamese.dyndns.org>.

I turned it into a list of do's and don'ts to make it easier to skim
it, and integrated my note that a TAP harness will get confused if you
print "ok" or "not ok" at the beginning of a line.

Thad had to be fixed in 335f87871fe5aa6b3fd55b2b4e80f16fe9681483 when
TAP support was introduced.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/README |   78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 78 insertions(+), 0 deletions(-)

diff --git a/t/README b/t/README
index 4079635..e481657 100644
--- a/t/README
+++ b/t/README
@@ -231,6 +231,84 @@ This test harness library does the following things:
    consistently when command line arguments --verbose (or -v),
    --debug (or -d), and --immediate (or -i) is given.
 
+Do's, don'ts & things to keep in mind
+-------------------------------------
+
+Here's a few examples of things you probably should and shouldn't do
+when writing tests.
+
+Do:
+
+ - Put as much code as possible inside test_expect_success and other
+   assertions.
+
+   Even code that isn't a test per se, but merely some setup code
+   should be inside a test assertion if at all possible. Test scripts
+   should only have trivial code outside of their assertions.
+
+ - Chain your test assertions
+
+   Write test code like this:
+
+	git merge foo &&
+	git push bar &&
+	test ...
+
+   Instead of:
+
+	git merge hla
+	git push gh
+	test ...
+
+   That way all of the commands in your tests will succeed or fail. If
+   you must ignore the return value of something (e.g. the return
+   value of export is unportable) it's best to indicate so explicitly
+   with a semicolon:
+
+	export HLAGH;
+	git merge hla &&
+	git push gh &&
+	test ...
+
+Don't:
+
+ - exit() within a <script> part.
+
+   The harness will catch this as a programming error of the test.
+   Use test_done instead if you need to stop the tests early (see
+   "Skipping tests" below).
+
+ - Break the TAP output
+
+   The raw output from your test might be interpreted by a TAP
+   harness. You usually don't have to worry about that. TAP harnesses
+   will ignore everything they don't know about, but don't step on
+   their toes in these areas:
+
+   - Don't print lines like "$x..$y" where $x and $y are integers.
+
+   - Don't print lines that begin with "ok" or "not ok".
+
+   A TAP harness expect a line that begins with either "ok" and "not
+   ok" to signal a test passed or failed (and our harness already
+   produces such lines), so your script shouldn't emit such lines to
+   their output.
+
+   You can glean some further possible issues from the TAP grammar
+   (see http://search.cpan.org/perldoc?TAP::Parser::Grammar#TAP_Grammar)
+   but the best indication is to just run the tests with prove(1),
+   it'll complain if anything is amiss.
+
+Keep in mind:
+
+ - That what you print to stderr and stdout is usually ignored
+
+   Inside <script> part, the standard output and standard error
+   streams are discarded, and the test harness only reports "ok" or
+   "not ok" to the end user running the tests. Under --verbose, they
+   are shown to help debugging the tests.
+
+
 Skipping tests
 --------------
 
-- 
1.7.1.251.g92a7

Re: [PATCH v4 7/7] t/README: Document the do's and don'ts of tests

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> +Do's, don'ts & things to keep in mind
> +-------------------------------------
> +
> +Here's a few examples of things you probably should and shouldn't do
> +when writing tests.

"Here are" perhaps?

> +Do:
> +
> + - Put as much code as possible inside test_expect_success and other
> +   assertions.
> +
> +   Even code that isn't a test per se, but merely some setup code
> +   should be inside a test assertion if at all possible. Test scripts
> +   should only have trivial code outside of their assertions.

Let's make it even stronger; "should only have trivial" -> "shouldn't have
any ... unless there is a good reason."

> +Don't:
> +
> + - exit() within a <script> part.
> +
> +   The harness will catch this as a programming error of the test.
> +   Use test_done instead if you need to stop the tests early (see
> +   "Skipping tests" below).
> +
> + - Break the TAP output
> +
> +   The raw output from your test might be interpreted by a TAP
> +   harness. You usually don't have to worry about that. TAP harnesses

I'd recommend dropping "You usually...about that"  You do care, but the
limitation may be not so severe.

> +   will ignore everything they don't know about, but don't step on
> +   their toes in these areas:
> +
> +   - Don't print lines like "$x..$y" where $x and $y are integers.
> +
> +   - Don't print lines that begin with "ok" or "not ok".
> +
> +   A TAP harness expect a line that begins with either "ok" and "not
> +   ok" to signal a test passed or failed (and our harness already
> +   produces such lines), so your script shouldn't emit such lines to
> +   their output.
> +
> +   You can glean some further possible issues from the TAP grammar
> +   (see http://search.cpan.org/perldoc?TAP::Parser::Grammar#TAP_Grammar)
> +   but the best indication is to just run the tests with prove(1),
> +   it'll complain if anything is amiss.
> +
> +Keep in mind:
> +
> + - That what you print to stderr and stdout is usually ignored
> +
> +   Inside <script> part, the standard output and standard error

Splitting the above into two sentences (or a header and a body) makes it
unclear that your "usually" comes from the earlier "Do Put as much code
inside test_expect_success...".  I think you can simply drop "That what
you print ... ignored".

Everything else in the series looked good.  Thanks.

Re: [PATCH v4 7/7] t/README: Document the do's and don'ts of tests

[Jakub Narebski]

Junio C Hamano <gitster@pobox.com> writes:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> 
> > +Do:
> > +
> > + - Put as much code as possible inside test_expect_success and other
> > +   assertions.
> > +
> > +   Even code that isn't a test per se, but merely some setup code
> > +   should be inside a test assertion if at all possible. Test scripts
> > +   should only have trivial code outside of their assertions.
> 
> Let's make it even stronger; "should only have trivial" -> "shouldn't have
> any ... unless there is a good reason."

I think that the only thing that can and *should* be put outside
test_expect_* is creating helper file: test vectors ('expect' files),
configuration files, files that are to be arguments to commands, etc.
Is it covered by "there is a good reason"?  Isn't it too severe?

There probably should be description when to put creating such files
in test script, and when to put them as pre-made files in tXXXX/
subdirectory (non US-ASCII is one reason to put it as pre-made file).
 
-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH v4 7/7] t/README: Document the do's and don'ts of tests

[Ævar Arnfjörð Bjarmason]

On Tue, Jul 6, 2010 at 08:35, Jakub Narebski <jnareb@gmail.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>
>> > +Do:
>> > +
>> > + - Put as much code as possible inside test_expect_success and other
>> > +   assertions.
>> > +
>> > +   Even code that isn't a test per se, but merely some setup code
>> > +   should be inside a test assertion if at all possible. Test scripts
>> > +   should only have trivial code outside of their assertions.
>>
>> Let's make it even stronger; "should only have trivial" -> "shouldn't have
>> any ... unless there is a good reason."
>
> I think that the only thing that can and *should* be put outside
> test_expect_* is creating helper file: test vectors ('expect' files),
> configuration files, files that are to be arguments to commands, etc.
> Is it covered by "there is a good reason"?  Isn't it too severe?

Personally I'd put `.. >expect && .. >actual && test_cmp' inside
test_expect_* too if they're only going to be used by that test, but
outside them at the top of the file if they're files that are used by
multiple tests for the duration of the test run.

> There probably should be description when to put creating such files
> in test script, and when to put them as pre-made files in tXXXX/
> subdirectory (non US-ASCII is one reason to put it as pre-made file).

I don't know which one would be preferrable. We have a lot of things
in t/t*/* that could be generated by a test, and vice-versa.

Re: [PATCH v4 7/7] t/README: Document the do's and don'ts of tests

[Jakub Narebski]

On Tue, 6 Jul 2010, Ævar Arnfjörð Bjarmason wrote:
> On Tue, Jul 6, 2010 at 08:35, Jakub Narebski <jnareb@gmail.com> wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>>
>>>> +Do:
>>>> +
>>>> + - Put as much code as possible inside test_expect_success and other
>>>> +   assertions.
>>>> +
>>>> +   Even code that isn't a test per se, but merely some setup code
>>>> +   should be inside a test assertion if at all possible. Test scripts
>>>> +   should only have trivial code outside of their assertions.
>>>
>>> Let's make it even stronger; "should only have trivial" -> "shouldn't have
>>> any ... unless there is a good reason."
>>
>> I think that the only thing that can and *should* be put outside
>> test_expect_* is creating helper file: test vectors ('expect' files),
>> configuration files, files that are to be arguments to commands, etc.
>> Is it covered by "there is a good reason"?  Isn't it too severe?
> 
> Personally I'd put `..>expect && ..>actual && test_cmp' inside
> test_expect_* too if they're only going to be used by that test, but
> outside them at the top of the file if they're files that are used by
> multiple tests for the duration of the test run.

I agree with putting e.g. `echo "sth" >expect` inside test_expect_*.
It is also obvious that `.. >actual` should be inside test_expect_*.

What I was thinking about was generating larger files, by using e.g.

  cat >expected <<\EOF
  
  ...
  EOF

Putting them inside test_expect_* would make it IMHO less clear, less
readable.


Sidenote: we should probably describe <<\EOF vs <<EOF here-docs and
when to use one or another in t/README.

>> There probably should be description when to put creating such files
>> in test script, and when to put them as pre-made files in tXXXX/
>> subdirectory (non US-ASCII is one reason to put it as pre-made file).
> 
> I don't know which one would be preferrable. We have a lot of things
> in t/t*/* that could be generated by a test, and vice-versa.

Probably because those tests were written by diferent people, and there
were no clear policy / guidelines description in t/README :-)


Thanks a lot for your work!

-- 
Jakub Narebski
Poland

Re: [PATCH v4 7/7] t/README: Document the do's and don'ts of tests

[Ævar Arnfjörð Bjarmason]

On Tue, Jul 6, 2010 at 02:35, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> +Do's, don'ts & things to keep in mind
>> +-------------------------------------
>> +
>> +Here's a few examples of things you probably should and shouldn't do
>> +when writing tests.
>
> "Here are" perhaps?
>
>> +Do:
>> +
>> + - Put as much code as possible inside test_expect_success and other
>> +   assertions.
>> +
>> +   Even code that isn't a test per se, but merely some setup code
>> +   should be inside a test assertion if at all possible. Test scripts
>> +   should only have trivial code outside of their assertions.
>
> Let's make it even stronger; "should only have trivial" -> "shouldn't have
> any ... unless there is a good reason."
>
>> +Don't:
>> +
>> + - exit() within a <script> part.
>> +
>> +   The harness will catch this as a programming error of the test.
>> +   Use test_done instead if you need to stop the tests early (see
>> +   "Skipping tests" below).
>> +
>> + - Break the TAP output
>> +
>> +   The raw output from your test might be interpreted by a TAP
>> +   harness. You usually don't have to worry about that. TAP harnesses
>
> I'd recommend dropping "You usually...about that"  You do care, but the
> limitation may be not so severe.
>
>> +   will ignore everything they don't know about, but don't step on
>> +   their toes in these areas:
>> +
>> +   - Don't print lines like "$x..$y" where $x and $y are integers.
>> +
>> +   - Don't print lines that begin with "ok" or "not ok".
>> +
>> +   A TAP harness expect a line that begins with either "ok" and "not
>> +   ok" to signal a test passed or failed (and our harness already
>> +   produces such lines), so your script shouldn't emit such lines to
>> +   their output.
>> +
>> +   You can glean some further possible issues from the TAP grammar
>> +   (see http://search.cpan.org/perldoc?TAP::Parser::Grammar#TAP_Grammar)
>> +   but the best indication is to just run the tests with prove(1),
>> +   it'll complain if anything is amiss.
>> +
>> +Keep in mind:
>> +
>> + - That what you print to stderr and stdout is usually ignored
>> +
>> +   Inside <script> part, the standard output and standard error
>
> Splitting the above into two sentences (or a header and a body) makes it
> unclear that your "usually" comes from the earlier "Do Put as much code
> inside test_expect_success...".  I think you can simply drop "That what
> you print ... ignored".
>
> Everything else in the series looked good.  Thanks.

All of these rewordings (the "t/README: proposed rewording..." commit
in pu) look good.

    Acked-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>