[PATCH v2 0/8] Improvements for t/README

[PATCH v2 0/8] Improvements for t/README

[Ævar Arnfjörð Bjarmason]

This is v2 of the t/README series. It should address the concerns
Junio and Jeff had about it.

Jeff:

  - Keep the ./ way of running tests, but elaborate a bit in the
    commit message

  - Mention --root's effects on the trash directory location

Junio:

  - The bit about ok/not ok moved out of the section on running
    tests.

  - Integrate a section that covers all the points in Junio's "Test
    your stuff" section, but does so as an itemized list, so it's
    easier to maintain it.

Other:

  - Made it diff --check friendly.

There's a diff against v1 of the series after this diffstat:

Ævar Arnfjörð Bjarmason (8):
  t/README: Tests are all +x, ./test, not sh ./test
  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 |  172 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 164 insertions(+), 8 deletions(-)

diff --git a/t/README b/t/README
index 29f795e..4a1cfc9 100644
--- a/t/README
+++ b/t/README
@@ -50,12 +50,6 @@ prove and other harnesses come with a lot of useful options. The
     # Repeat until no more failures
     $ prove -j 15 --state=failed,save ./t[0-9]*.sh
 
-The TAP support is completely incidental. A TAP harness is just a
-program that interprets the "ok"/"not ok" (and some other strings) in
-a special way. The only limitation this imposes is that you shouldn't
-echo "ok" or "not ok" by yourself at the beginning of a line, that'll
-confuse the TAP harness.
-
 You can also run each test individually from command line, like this:
 
     $ ./t3010-ls-files-killed-modified.sh
@@ -229,14 +223,92 @@ This test harness library does the following things:
 
  - 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' if you must know, but I do not
-   think you care.
+   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
    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 gleam 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
 --------------
 

[PATCH v2 1/8] t/README: Tests are all +x, ./test, not sh ./test

[Ævar Arnfjörð Bjarmason]

test-lib.sh runs tests with $SHELL_PATH or the #!/bin/sh shebang when
run under prove. It's simpler to just show ./ as an example.

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 aa4ed28..b378ab1 100644
--- a/t/README
+++ b/t/README
@@ -52,7 +52,7 @@ prove and other harnesses come with a lot of useful options. The
 
 You can also run each test individually from command line, like this:
 
-    $ sh ./t3010-ls-files-killed-modified.sh
+    $ ./t3010-ls-files-killed-modified.sh
     ok 1 - git update-index --add to add various paths.
     ok 2 - git ls-files -k to show killed files.
     ok 3 - validate git ls-files -k output.
-- 
1.7.1.251.g92a7

[PATCH v2 2/8] 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 b378ab1..3cdd744 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 v2 3/8] 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 3cdd744..1d0fd18 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 v2 4/8] 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 1d0fd18..2df56a8 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 v2 5/8] 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 2df56a8..d9b5294 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_without_stderr \
+	    'Perl API' \
+	    "$PERL_PATH" "$TEST_DIRECTORY"/t9700/test.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 v2 6/8] 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 d9b5294..27b9658 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 v2 7/8] 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 27b9658..5a291e1 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 v2 8/8] 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 5a291e1..4a1cfc9 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 gleam 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 v2 5/8] t/README: Document test_external*

[Jakub Narebski]

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

> + - 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_without_stderr \
> +	    'Perl API' \
> +	    "$PERL_PATH" "$TEST_DIRECTORY"/t9700/test.pl

I think this example was to be about 'test_external'; currently it
duplicates example for 'test_external_without_stderr'.

Perhaps:

  +     test_external \
  +         'GitwebCache::*FileCache*' \
  +         "$PERL_PATH" "$TEST_DIRECTORY"/t9503/test_cache_interface.pl

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

-- 
Jakub Narebski
Poland
ShadeHawk on #git

[PATCH v3 5/8] 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>
---

On Thu, Jul 1, 2010 at 20:52, Jakub Narebski <jnareb@gmail.com> wrote:

> I think this example was to be about 'test_external'; currently it
> duplicates example for 'test_external_without_stderr'.

Quite correct. Here's a fixed patch that uses your example. Thanks.

 t/README |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/t/README b/t/README
index 2df56a8..3c31cff 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

Re: [PATCH v2 1/8] t/README: Tests are all +x, ./test, not sh ./test

[Junio C Hamano]

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

> test-lib.sh runs tests with $SHELL_PATH or the #!/bin/sh shebang when
> run under prove. It's simpler to just show ./ as an example.

I don't think this is a good change at all.  If we spell "sh" out, people
with defective shell as the default shell on their system at least would
know that they need to substitute that with "ksh" or "bash" or whatever.

> 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 aa4ed28..b378ab1 100644
> --- a/t/README
> +++ b/t/README
> @@ -52,7 +52,7 @@ prove and other harnesses come with a lot of useful options. The
>  
>  You can also run each test individually from command line, like this:
>  
> -    $ sh ./t3010-ls-files-killed-modified.sh
> +    $ ./t3010-ls-files-killed-modified.sh
>      ok 1 - git update-index --add to add various paths.
>      ok 2 - git ls-files -k to show killed files.
>      ok 3 - validate git ls-files -k output.
> -- 
> 1.7.1.251.g92a7
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/8] t/README: Tests are all +x, ./test, not sh ./test

[Ævar Arnfjörð Bjarmason]

On Thu, Jul 1, 2010 at 23:54, Junio C Hamano <gitster@pobox.com> wrote:
> I don't think this is a good change at all.  If we spell "sh" out, people
> with defective shell as the default shell on their system at least would
> know that they need to substitute that with "ksh" or "bash" or whatever.

Sure, consider this patch dropped from the series then.

Re: [PATCH v2 0/8] Improvements for t/README

[Jeff King]

On Thu, Jul 01, 2010 at 08:17:50PM +0000, Ævar Arnfjörð Bjarmason wrote:

> This is v2 of the t/README series. It should address the concerns
> Junio and Jeff had about it.
> 
> Jeff:
> 
>   - Keep the ./ way of running tests, but elaborate a bit in the
>     commit message
> 
>   - Mention --root's effects on the trash directory location

Thanks. On the first point, I agree with Junio that dropping patch 1 is
probably the best thing. For the second, your changes to patch 2 look
good to me.

The rest look OK from my cursory read. I like the "do's and don'ts"
list, which is a good way of communicating the little bits of wisdom
(btw, I saw your post a few weeks ago about shell portability in git,
and I think a list of known-bad constructs would be a good idea. Making
a list from our previous fixes is on my todo list, but I haven't
actually started on it. :) ).

One nit:

> +   You can gleam 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.

I think you mean "glean".

-Peff