git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Jakub Narebski <jnareb@gmail.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH v2 0/3] Make the Git tests emit TAP format
Date: Tue, 15 Jun 2010 15:17:58 +0000	[thread overview]
Message-ID: <AANLkTimxE0mqmitRzlXjSAO6v7IOEg4guUnRlzJUyIF1@mail.gmail.com> (raw)
In-Reply-To: <AANLkTinHrf_ZuuoMXlL8BFqm3UhRYxrV4t2Nmp5QNjrE@mail.gmail.com>

On Mon, Jun 14, 2010 at 22:10, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> On Mon, Jun 14, 2010 at 21:49, Jakub Narebski <jnareb@gmail.com> wrote:
>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>>
>>> All this series does is slightly adjust the raw text output of our
>>> tests so that it conforms. to the TAP standard, i.e. instead of this:
>>>
>>>    $ ./t0005-signals.sh
>>>    *   ok 1: sigchain works
>>>    * passed all 1 test(s)
>>>
>>> We get this:
>>>
>>>    $ ./t0005-signals.sh
>>>    ok 1 - sigchain works
>>>    # passed all 1 test(s)
>>>    1..1
>>>
>>> Changing the output format like this gives us the ability to run the
>>> Git tests with any TAP tool (like prove(1)) at no extra cost. Every
>>> other existing way of running the tests continues to work, it's just
>>> easier for machines to read the output now.
>
> I'll re-submit a version of the patch with a better commit message
> which addresses all of the below.
>
>> This doesn't tell us if the result of running test suite with
>> '--verbose' and/or with '--debug' changes, and if changes how?  Is it
>> compatibile with TAP format so that TAP parsers understand it?
>
> It just changes in that the lines that previously said e.g. "*   ok 1:
> sigchain works" now say "ok 1 - sigchain works".
>
> TAP parsers still understand it, since ignoring unknown garbage is
> part of the TAP standard.

Actually it doesn't work for all the tests. The following crops up on
prove -j 10 ./t[0-9]*.sh :: --verbose:

    ./t1007-hash-object.sh                             (Wstat: 0
Tests: 19 Failed: 0)
      Parse errors: Tests out of sequence.  Found (12) but expected (11)
                    Tests out of sequence.  Found (13) but expected (12)
                    Tests out of sequence.  Found (14) but expected (13)
                    Tests out of sequence.  Found (15) but expected (14)
                    Tests out of sequence.  Found (16) but expected (15)
    Displayed the first 5 of 10 TAP syntax errors.
    Re-run prove with the -p option to see them all.
    ./t3411-rebase-preserve-around-merges.sh           (Wstat: 0
Tests: 1 Failed: 0)
      Parse errors: Bad plan.  You planned 3 tests but ran 1.
    ./t3410-rebase-preserve-dropped-merges.sh          (Wstat: 0
Tests: 1 Failed: 0)
      Parse errors: Bad plan.  You planned 3 tests but ran 1.
    ./t3413-rebase-hook.sh                             (Wstat: 0
Tests: 10 Failed: 0)
      Parse errors: Tests out of sequence.  Found (4) but expected (3)
                    Tests out of sequence.  Found (5) but expected (4)
                    Tests out of sequence.  Found (6) but expected (5)
                    Tests out of sequence.  Found (7) but expected (6)
                    Tests out of sequence.  Found (11) but expected (7)
    Displayed the first 5 of 9 TAP syntax errors.
    Re-run prove with the -p option to see them all.
    ./t3409-rebase-preserve-merges.sh                  (Wstat: 0
Tests: 1 Failed: 0)
      Parse errors: Bad plan.  You planned 3 tests but ran 1.
    ./t3414-rebase-preserve-onto.sh                    (Wstat: 0
Tests: 1 Failed: 0)
      Parse errors: Bad plan.  You planned 4 tests but ran 1.
    ./t3415-rebase-autosquash.sh                       (Wstat: 0
Tests: 1 Failed: 0)
      Parse errors: Bad plan.  You planned 4 tests but ran 1.
    ./t3416-rebase-onto-threedots.sh                   (Wstat: 0
Tests: 5 Failed: 0)
      Parse errors: Tests out of sequence.  Found (7) but expected (5)
                    Bad plan.  You planned 7 tests but ran 5.
    ./t3412-rebase-root.sh                             (Wstat: 0
Tests: 24 Failed: 0)
    ./t3404-rebase-interactive.sh                      (Wstat: 0
Tests: 23 Failed: 0)
      Parse errors: Tests out of sequence.  Found (6) but expected (2)
                    Tests out of sequence.  Found (8) but expected (3)
                    Tests out of sequence.  Found (9) but expected (4)
                    Tests out of sequence.  Found (10) but expected (5)
                    Tests out of sequence.  Found (11) but expected (6)
    Displayed the first 5 of 23 TAP syntax errors.
    Re-run prove with the -p option to see them all.
    ./t5407-post-rewrite-hook.sh                       (Wstat: 0
Tests: 9 Failed: 0)
      Parse errors: Tests out of sequence.  Found (10) but expected (8)
                    Tests out of sequence.  Found (11) but expected (9)
                    Bad plan.  You planned 12 tests but ran 9.
    ./t7402-submodule-rebase.sh                        (Wstat: 0
Tests: 4 Failed: 0)
      Parse errors: Tests out of sequence.  Found (4) but expected (3)
                    Tests out of sequence.  Found (5) but expected (4)
                    Bad plan.  You planned 5 tests but ran 4.
    ./t7003-filter-branch.sh                           (Wstat: 0
Tests: 30 Failed: 0)
      Parse errors: Tests out of sequence.  Found (8) but expected (7)
                    Tests out of sequence.  Found (9) but expected (8)
                    Tests out of sequence.  Found (10) but expected (9)
                    Tests out of sequence.  Found (11) but expected (10)
                    Tests out of sequence.  Found (12) but expected (11)
    Displayed the first 5 of 25 TAP syntax errors.
    Re-run prove with the -p option to see them all.
    ./t9001-send-email.sh                              (Wstat: 0
Tests: 63 Failed: 0)
      Parse errors: Tests out of sequence.  Found (45) but expected (44)
                    Tests out of sequence.  Found (46) but expected (45)
                    Tests out of sequence.  Found (47) but expected (46)
                    Tests out of sequence.  Found (48) but expected (47)
                    Tests out of sequence.  Found (49) but expected (48)

The problem is that these tests all have code outputthat goes
something like this:

    ok 2 - rebase

    expecting success:
            git checkout test &&
            git reset --hard side &&
            EDITOR=true git rebase -i master &&
            test "z$(cat git)" = zworld

    HEAD is now at c847452 side
    Rebasing (1/1)^Mok 3 - rebase -i

    expecting success:

Caused by tests like these that don't end in a newline:

    test_expect_success 'rebase -i' '
    	git checkout test &&
    	git reset --hard side &&
    	EDITOR=true git rebase -i master &&
    	test "z$(cat git)" = zworld'
    '

This was of course also broken before TAP, it just revealed the issue:

    Applying: side
    *   ok 2: rebase

    * expecting success:
            git checkout test &&
            git reset --hard side &&
            EDITOR=true git rebase -i master &&
            test "z$(cat git)" = zworld

    HEAD is now at c847452 side
    Rebasing (1/1)^M*   ok 3: rebase -i

I propose to fix it like this:

    diff --git a/t/test-lib.sh b/t/test-lib.sh
    index 37987d7..86a46bf 100644
    --- a/t/test-lib.sh
    +++ b/t/test-lib.sh
    @@ -369,6 +369,9 @@ test_run_ () {
            eval >&3 2>&4 "$1"
            eval_ret=$?
            eval >&3 2>&4 "$test_cleanup"
    +       if test "$verbose" = "t" && test -n "$HARNESS_ACTIVE"; then
    +               echo ""
    +       fi
            return 0
     }

That makes all tests a bit more verbose when run under --verbose and a
TAP::Harness, but fixes the issue everywhere and in any future code by
ensuring that there's a newline before /^(?:not )?ok/ lines.

The alternative would be to change all the tests in question so that
they end with && echo, or to munge the `eval >&3 2>&4 "$1"' part above
(maybe with tempfiles?) so that a newline is only added to the end if
there isn't one already.

Aside from that, making sure that nothing in the test suite itself
says "ok" on an otherwise empty line is also required:

     t/t1020-subdirectory.sh          |   12 ++++++------
     t/t2102-update-index-symlinks.sh |    2 +-
     t/t3700-add.sh                   |   12 ++++++------
     3 files changed, 13 insertions(+), 13 deletions(-)

    diff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh
    index 210e594..5687499 100755
    --- a/t/t1020-subdirectory.sh
    +++ b/t/t1020-subdirectory.sh
    @@ -24,18 +24,18 @@ test_expect_success 'update-index and ls-files' '
     	cd "$HERE" &&
     	git update-index --add one &&
     	case "`git ls-files`" in
    -	one) echo ok one ;;
    +	one) echo pass one ;;
     	*) echo bad one; exit 1 ;;
     	esac &&
     	cd dir &&
     	git update-index --add two &&
     	case "`git ls-files`" in
    -	two) echo ok two ;;
    +	two) echo pass two ;;
     	*) echo bad two; exit 1 ;;
     	esac &&
     	cd .. &&
     	case "`git ls-files`" in
    -	dir/two"$LF"one) echo ok both ;;
    +	dir/two"$LF"one) echo pass both ;;
     	*) echo bad; exit 1 ;;
     	esac
     '
    @@ -58,17 +58,17 @@ test_expect_success 'diff-files' '
     	echo a >>one &&
     	echo d >>dir/two &&
     	case "`git diff-files --name-only`" in
    -	dir/two"$LF"one) echo ok top ;;
    +	dir/two"$LF"one) echo pass top ;;
     	*) echo bad top; exit 1 ;;
     	esac &&
     	# diff should not omit leading paths
     	cd dir &&
     	case "`git diff-files --name-only`" in
    -	dir/two"$LF"one) echo ok subdir ;;
    +	dir/two"$LF"one) echo pass subdir ;;
     	*) echo bad subdir; exit 1 ;;
     	esac &&
     	case "`git diff-files --name-only .`" in
    -	dir/two) echo ok subdir limited ;;
    +	dir/two) echo pass subdir limited ;;
     	*) echo bad subdir limited; exit 1 ;;
     	esac
     '
    diff --git a/t/t2102-update-index-symlinks.sh
b/t/t2102-update-index-symlinks.sh
    index 1ed44ee..4d0d0a3 100755
    --- a/t/t2102-update-index-symlinks.sh
    +++ b/t/t2102-update-index-symlinks.sh
    @@ -24,7 +24,7 @@ git update-index symlink'
     test_expect_success \
     'the index entry must still be a symbolic link' '
     case "`git ls-files --stage --cached symlink`" in
    -120000" "*symlink) echo ok;;
    +120000" "*symlink) echo pass;;
     *) echo fail; git ls-files --stage --cached symlink; (exit 1);;
     esac'

    diff --git a/t/t3700-add.sh b/t/t3700-add.sh
    index 525c9a8..6f031af 100755
    --- a/t/t3700-add.sh
    +++ b/t/t3700-add.sh
    @@ -26,7 +26,7 @@ test_expect_success \
     	 chmod 755 xfoo1 &&
     	 git add xfoo1 &&
     	 case "`git ls-files --stage xfoo1`" in
    -	 100644" "*xfoo1) echo ok;;
    +	 100644" "*xfoo1) echo pass;;
     	 *) echo fail; git ls-files --stage xfoo1; (exit 1);;
     	 esac'

    @@ -35,7 +35,7 @@ test_expect_success SYMLINKS 'git add:
filemode=0 should not get confused by sym
     	ln -s foo xfoo1 &&
     	git add xfoo1 &&
     	case "`git ls-files --stage xfoo1`" in
    -	120000" "*xfoo1) echo ok;;
    +	120000" "*xfoo1) echo pass;;
     	*) echo fail; git ls-files --stage xfoo1; (exit 1);;
     	esac
     '
    @@ -47,7 +47,7 @@ test_expect_success \
     	 chmod 755 xfoo2 &&
     	 git update-index --add xfoo2 &&
     	 case "`git ls-files --stage xfoo2`" in
    -	 100644" "*xfoo2) echo ok;;
    +	 100644" "*xfoo2) echo pass;;
     	 *) echo fail; git ls-files --stage xfoo2; (exit 1);;
     	 esac'

    @@ -56,7 +56,7 @@ test_expect_success SYMLINKS 'git add:
filemode=0 should not get confused by sym
     	ln -s foo xfoo2 &&
     	git update-index --add xfoo2 &&
     	case "`git ls-files --stage xfoo2`" in
    -	120000" "*xfoo2) echo ok;;
    +	120000" "*xfoo2) echo pass;;
     	*) echo fail; git ls-files --stage xfoo2; (exit 1);;
     	esac
     '
    @@ -67,7 +67,7 @@ test_expect_success SYMLINKS \
     	 ln -s xfoo2 xfoo3 &&
     	 git update-index --add xfoo3 &&
     	 case "`git ls-files --stage xfoo3`" in
    -	 120000" "*xfoo3) echo ok;;
    +	 120000" "*xfoo3) echo pass;;
     	 *) echo fail; git ls-files --stage xfoo3; (exit 1);;
     	 esac'

    @@ -172,7 +172,7 @@ test_expect_success 'git add --refresh' '
     	test -z "`git diff-index HEAD -- foo`" &&
     	git read-tree HEAD &&
     	case "`git diff-index HEAD -- foo`" in
    -	:100644" "*"M	foo") echo ok;;
    +	:100644" "*"M	foo") echo pass;;
     	*) echo fail; (exit 1);;
     	esac &&
     	git add --refresh -- foo &&

Another way to solve all of these would be to apply s/^/# / to all the
--verbose output. This is what the Perl tools do:

    $ perl -MTest::More=tests,2 -E 'pass "a test";
diag("hello\nthere\noutput\n"); pass "another test"'
    1..2
    ok 1 - a test
    # hello
    # there
    # output
    ok 2 - another test

Then the verbose output would only be printed under prove(1) if its
--verbose option was supplied (if lines aren't comments they're
printed as-is). That'd also require something like the eval + tempfile
hack suggested above.

  parent reply	other threads:[~2010-06-15 15:18 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-06-09 15:22 [PATCH v2 0/3] Make the Git tests emit TAP format Ævar Arnfjörð Bjarmason
2010-06-09 15:22 ` [PATCH v2 1/3] Make test-lib.sh emit valid " Ævar Arnfjörð Bjarmason
2010-06-14 22:01   ` Jakub Narebski
2010-06-14 22:29     ` Ævar Arnfjörð Bjarmason
2010-06-09 15:22 ` [PATCH v2 2/3] Skip tests in a way that makes sense under TAP Ævar Arnfjörð Bjarmason
2010-06-09 15:24 ` [PATCH v2 3/3] We use TAP so the Perl test can run without scaffolding Ævar Arnfjörð Bjarmason
2010-06-14 21:49 ` [PATCH v2 0/3] Make the Git tests emit TAP format Jakub Narebski
2010-06-14 22:10   ` Ævar Arnfjörð Bjarmason
2010-06-14 23:16     ` Ævar Arnfjörð Bjarmason
2010-06-15  3:08       ` Junio C Hamano
2010-06-15  3:10         ` Ævar Arnfjörð Bjarmason
2010-06-15 15:17     ` Ævar Arnfjörð Bjarmason [this message]
2010-06-15 16:42       ` Andreas Ericsson
2010-06-15 16:49         ` Ævar Arnfjörð Bjarmason
2010-06-15 22:32       ` [PATCH v3 0/5] TAP support for Git Ævar Arnfjörð Bjarmason
2010-06-15 22:32       ` [PATCH v3 1/5] Make test-lib.sh emit valid TAP format Ævar Arnfjörð Bjarmason
2010-06-15 22:32       ` [PATCH v3 2/5] Skip tests in a way that makes sense under TAP Ævar Arnfjörð Bjarmason
2010-06-15 22:32       ` [PATCH v3 3/5] We use TAP so the Perl test can run without scaffolding Ævar Arnfjörð Bjarmason
2010-06-15 22:32       ` [PATCH v3 4/5] TAP: Say "pass" rather than "ok" on an empty line Ævar Arnfjörð Bjarmason
2010-06-15 22:32       ` [PATCH v3 5/5] TAP: Make sure there's a newline before "ok" under harness Ævar Arnfjörð Bjarmason

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=AANLkTimxE0mqmitRzlXjSAO6v7IOEg4guUnRlzJUyIF1@mail.gmail.com \
    --to=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jnareb@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).