* [PATCH 0/6 + 1] test-lib: make --verbose output valid TAP
2021-03-01 13:54 ` Ævar Arnfjörð Bjarmason
@ 2021-03-09 16:02 ` Ævar Arnfjörð Bjarmason
2021-03-09 17:52 ` SZEDER Gábor
2021-03-09 16:02 ` [PATCH 1/7] test-lib: remove test_external Ævar Arnfjörð Bjarmason
` (6 subsequent siblings)
7 siblings, 1 reply; 47+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-03-09 16:02 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, SZEDER Gábor, Johannes Schindelin,
Ævar Arnfjörð Bjarmason
This makes the --verbose valid TAP.
This is a non-RFC version of an RFC patch I sent in
https://lore.kernel.org/git/87tupwj5y5.fsf@evledraar.gmail.com/
There's a 7/7 patch that's an RFC at the end. It works for me, but
since I haven't used the JUnit output it's likely subtly broken in
some way (e.g. the new output doesn't include a timestamp attribute),
but it shows how easy it is to add new output targets.
The eventual goal not included in this series is to have multiple
output targets, and e.g. convert ci/print-test-failures.sh to use a
TAP parser.
Machine-readable "TAP --verbose -x" output can bring us a lot of nice
things down the line, I have some local WIP code that's a smarter
version of ci/print-test-failures.sh that knows how to spew out only
the output relevant to the failing test(s).
Future improvements based on that could be showing what specific line
in a &&-chain failed, as well as e.g. running a debug version of a
failing "grep" if we detect that the failing line line was 'grep <pat>
<filename>'.
Ævar Arnfjörð Bjarmason (7):
test-lib: remove test_external
test-lib: add say_color_tap helper to emit TAP format
test-lib: color "ok" TAP directives green under --verbose (or -x)
test-lib: add tee with TAP support to test-tool
test-lib: indent and format GIT_TEST_TEE_OUTPUT_FILE code
test-lib: make --verbose output valid TAP
test-lib: generate JUnit output via TAP
Makefile | 2 +-
.../netrc/t-git-credential-netrc.sh | 7 +-
t/README | 35 +--
t/helper/test-date.c | 12 -
t/helper/test-path-utils.c | 21 --
t/helper/test-tee.c | 85 +++++++
t/helper/test-tool.c | 2 +-
t/helper/test-tool.h | 2 +-
t/helper/test-xml-encode.c | 80 -------
t/t0000-basic.sh | 8 +
t/t0202-gettext-perl.sh | 10 +-
t/t9700-perl-git.sh | 10 +-
t/test-lib-functions.sh | 89 +------
t/test-lib.sh | 226 +++++-------------
14 files changed, 177 insertions(+), 412 deletions(-)
create mode 100644 t/helper/test-tee.c
delete mode 100644 t/helper/test-xml-encode.c
--
2.31.0.rc1.210.g0f8085a843c
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 0/6 + 1] test-lib: make --verbose output valid TAP
2021-03-09 16:02 ` [PATCH 0/6 + 1] test-lib: make --verbose output valid TAP Ævar Arnfjörð Bjarmason
@ 2021-03-09 17:52 ` SZEDER Gábor
2021-03-09 21:03 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 47+ messages in thread
From: SZEDER Gábor @ 2021-03-09 17:52 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Jeff King, Johannes Schindelin
On Tue, Mar 09, 2021 at 05:02:12PM +0100, Ævar Arnfjörð Bjarmason wrote:
> The eventual goal not included in this series is to have multiple
> output targets, and e.g. convert ci/print-test-failures.sh to use a
> TAP parser.
>
> Machine-readable "TAP --verbose -x" output can bring us a lot of nice
> things down the line, I have some local WIP code that's a smarter
> version of ci/print-test-failures.sh that knows how to spew out only
> the output relevant to the failing test(s).
I wonder what you mean by the word "relevant" here, as I can't imagine
how you could possibly identify what is relevant for a failing test
and what isn't. If you didn't at all meant "relevant", but that it
will show only the output of the failing test(s), then this is a bad
example. Our test cases depend too much on previous test cases, and a
failure of one test can be the result of a change in a previous
successful tests.
Therefore, any such change to 'ci/print-test-failures.sh' will have my
firm NACK.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 0/6 + 1] test-lib: make --verbose output valid TAP
2021-03-09 17:52 ` SZEDER Gábor
@ 2021-03-09 21:03 ` Ævar Arnfjörð Bjarmason
2021-03-09 22:07 ` SZEDER Gábor
0 siblings, 1 reply; 47+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-03-09 21:03 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: git, Junio C Hamano, Jeff King, Johannes Schindelin
On Tue, Mar 09 2021, SZEDER Gábor wrote:
> On Tue, Mar 09, 2021 at 05:02:12PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> The eventual goal not included in this series is to have multiple
>> output targets, and e.g. convert ci/print-test-failures.sh to use a
>> TAP parser.
>>
>> Machine-readable "TAP --verbose -x" output can bring us a lot of nice
>> things down the line, I have some local WIP code that's a smarter
>> version of ci/print-test-failures.sh that knows how to spew out only
>> the output relevant to the failing test(s).
>
> I wonder what you mean by the word "relevant" here, as I can't imagine
> how you could possibly identify what is relevant for a failing test
> and what isn't. If you didn't at all meant "relevant", but that it
> will show only the output of the failing test(s), then this is a bad
> example. Our test cases depend too much on previous test cases, and a
> failure of one test can be the result of a change in a previous
> successful tests.
>
> Therefore, any such change to 'ci/print-test-failures.sh' will have my
> firm NACK.
On e.g. the github CI every step in the run is an collapsable button, so
we could have our cake and eat it too here.
It seems to me like a sane default would be to have an equivalent to
"print-test-failures.sh" that only prints the --verbose output for the
failing tests, and a "print-all-output-for-failing-tests.sh" or whatever
which gave you the full output.
Even then, there seem to me to be some low hanging fruit for
abbreviating even that output. E.g. if you we have 100 tests and we
failed only on the 5th, isn't the --verbose -x output up to and
including the 5th going to be enough, or do we need it for the other 95?
There's also ways to unambiguously abbreviate the output, and that's now
easy to implement as shown in my other just-sent reply to you.
E.g. a lot of our test setup is really repetitive, so under -x you're
sometimes looking at the same 5-10+ lines of test setup. We could just
do a couple of passes over the output and change output like:
+ foo
+ bar
+ he
+ foo
+ bar
+ llo
+ foo
+ bar
+ world
to:
$1 = +foo
+bar
+ he
$1
+ llo
$1
+ world
Or whatever, I'm not advocating for the specific syntax there, just for
the output to be nicer.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 0/6 + 1] test-lib: make --verbose output valid TAP
2021-03-09 21:03 ` Ævar Arnfjörð Bjarmason
@ 2021-03-09 22:07 ` SZEDER Gábor
0 siblings, 0 replies; 47+ messages in thread
From: SZEDER Gábor @ 2021-03-09 22:07 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Jeff King, Johannes Schindelin
On Tue, Mar 09, 2021 at 10:03:54PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
> On Tue, Mar 09 2021, SZEDER Gábor wrote:
>
> > On Tue, Mar 09, 2021 at 05:02:12PM +0100, Ævar Arnfjörð Bjarmason wrote:
> >> The eventual goal not included in this series is to have multiple
> >> output targets, and e.g. convert ci/print-test-failures.sh to use a
> >> TAP parser.
> >>
> >> Machine-readable "TAP --verbose -x" output can bring us a lot of nice
> >> things down the line, I have some local WIP code that's a smarter
> >> version of ci/print-test-failures.sh that knows how to spew out only
> >> the output relevant to the failing test(s).
> >
> > I wonder what you mean by the word "relevant" here, as I can't imagine
> > how you could possibly identify what is relevant for a failing test
> > and what isn't. If you didn't at all meant "relevant", but that it
> > will show only the output of the failing test(s), then this is a bad
> > example. Our test cases depend too much on previous test cases, and a
> > failure of one test can be the result of a change in a previous
> > successful tests.
> >
> > Therefore, any such change to 'ci/print-test-failures.sh' will have my
> > firm NACK.
>
> On e.g. the github CI every step in the run is an collapsable button, so
> we could have our cake and eat it too here.
Well, one of the things I didn't like in GitHub CI is that I had to
click a lot to get to the information I wanted...
> It seems to me like a sane default would be to have an equivalent to
> "print-test-failures.sh" that only prints the --verbose output for the
> failing tests, and a "print-all-output-for-failing-tests.sh" or whatever
> which gave you the full output.
>
> Even then, there seem to me to be some low hanging fruit for
> abbreviating even that output. E.g. if you we have 100 tests and we
> failed only on the 5th, isn't the --verbose -x output up to and
> including the 5th going to be enough, or do we need it for the other 95?
I have a test fix waiting to be sent out, where a failing git command
in the 'test_when_finished' block of test #21 went unnoticed because
of the broken && chain, and ultimately caused the failure of test
#91.
In my opinion your proposed changes to 'ci/print-test-failures.sh'
would make things worse.
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH 1/7] test-lib: remove test_external
2021-03-01 13:54 ` Ævar Arnfjörð Bjarmason
2021-03-09 16:02 ` [PATCH 0/6 + 1] test-lib: make --verbose output valid TAP Ævar Arnfjörð Bjarmason
@ 2021-03-09 16:02 ` Ævar Arnfjörð Bjarmason
2021-03-10 1:04 ` Junio C Hamano
2021-03-09 16:02 ` [PATCH 2/7] test-lib: add say_color_tap helper to emit TAP format Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
7 siblings, 1 reply; 47+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-03-09 16:02 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, SZEDER Gábor, Johannes Schindelin,
Ævar Arnfjörð Bjarmason
Remove the test_external function(s) in favor of running the Perl
tests with a test_expect_success.
The only advantage of this wrapper added in
fb32c410087 (t/test-lib.sh: add test_external and
test_external_without_stderr, 2008-06-19) was that we got the
Test::More output as-is from the two wrapped scripts.
Now we'll instead hang until the script is finished, and report on its
exit code. The Test::More framework ensures that we exit with non-zero
exit code on failure.
My motivation for this is to eliminate a special case where things
that aren't test-lib.sh are going to produce TAP, for reasons that'll
be clear in subsequent commits.
This also eliminates special cases I added in d998bd4ab67 (test-lib:
Make the test_external_* functions TAP-aware, 2010-06-24) from
test-lib.sh itself.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
.../netrc/t-git-credential-netrc.sh | 7 +-
t/README | 26 ------
t/t0202-gettext-perl.sh | 10 +--
t/t9700-perl-git.sh | 10 +--
t/test-lib-functions.sh | 89 +------------------
t/test-lib.sh | 42 ++++-----
6 files changed, 28 insertions(+), 156 deletions(-)
diff --git a/contrib/credential/netrc/t-git-credential-netrc.sh b/contrib/credential/netrc/t-git-credential-netrc.sh
index 07227d02287..28118a9e194 100755
--- a/contrib/credential/netrc/t-git-credential-netrc.sh
+++ b/contrib/credential/netrc/t-git-credential-netrc.sh
@@ -20,13 +20,10 @@
'set up test repository' \
'git config --add gpg.program test.git-config-gpg'
- # The external test will outputs its own plan
- test_external_has_tap=1
-
export PERL5LIB="$GITPERLLIB"
- test_external \
- 'git-credential-netrc' \
+ test_expect_success 'git-credential-netrc' '
perl "$GIT_BUILD_DIR"/contrib/credential/netrc/test.pl
+ '
test_done
)
diff --git a/t/README b/t/README
index 593d4a4e270..2cc8cbc7185 100644
--- a/t/README
+++ b/t/README
@@ -844,32 +844,6 @@ 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 "$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 "$TEST_DIRECTORY"/t9700/test.pl
-
- test_expect_code <exit-code> <command>
Run a command and ensure that it exits with the given exit code.
diff --git a/t/t0202-gettext-perl.sh b/t/t0202-gettext-perl.sh
index a29d166e007..06a93b36790 100755
--- a/t/t0202-gettext-perl.sh
+++ b/t/t0202-gettext-perl.sh
@@ -17,11 +17,9 @@ perl -MTest::More -e 0 2>/dev/null || {
test_done
}
-# The external test will outputs its own plan
-test_external_has_tap=1
-
-test_external_without_stderr \
- 'Perl Git::I18N API' \
- perl "$TEST_DIRECTORY"/t0202/test.pl
+test_expect_success 'run t0202/test.pl to test Git::I18N.pm' '
+ perl "$TEST_DIRECTORY"/t0202/test.pl 2>stderr &&
+ test_must_be_empty stderr
+'
test_done
diff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh
index 102c133112c..574c57b17f1 100755
--- a/t/t9700-perl-git.sh
+++ b/t/t9700-perl-git.sh
@@ -50,11 +50,9 @@ test_expect_success \
git config --add test.pathmulti bar
'
-# The external test will outputs its own plan
-test_external_has_tap=1
-
-test_external_without_stderr \
- 'Perl API' \
- perl "$TEST_DIRECTORY"/t9700/test.pl
+test_expect_success 'use t9700/test.pl to test Git.pm' '
+ perl "$TEST_DIRECTORY"/t9700/test.pl 2>stderr &&
+ test_must_be_empty stderr
+'
test_done
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 3dd68091bbf..ead3afb36b3 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -468,7 +468,7 @@ write_script () {
# - Explicitly using test_have_prereq.
#
# - Implicitly by specifying the prerequisite tag in the calls to
-# test_expect_{success,failure} and test_external{,_without_stderr}.
+# test_expect_{success,failure}
#
# The single parameter is the prerequisite tag (a simple word, in all
# capital letters by convention).
@@ -660,93 +660,6 @@ test_expect_success () {
return 1
}
-# test_external runs external test scripts that provide continuous
-# test output about their progress, and succeeds/fails on
-# zero/non-zero exit code. It outputs the test output on stdout even
-# in non-verbose mode, and announces the external script with "# run
-# <n>: ..." before running it. When providing relative paths, keep in
-# mind that all scripts run in "trash directory".
-# Usage: test_external description command arguments...
-# Example: test_external 'Perl API' perl ../path/to/test.pl
-test_external () {
- test "$#" = 4 && { test_prereq=$1; shift; } || test_prereq=
- test "$#" = 3 ||
- BUG "not 3 or 4 parameters to test_external"
- descr="$1"
- shift
- test_verify_prereq
- export test_prereq
- if ! test_skip "$descr" "$@"
- then
- # Announce the script to reduce confusion about the
- # test output that follows.
- say_color "" "# run $test_count: $descr ($*)"
- # Export TEST_DIRECTORY, TRASH_DIRECTORY and GIT_TEST_LONG
- # to be able to use them in script
- export TEST_DIRECTORY TRASH_DIRECTORY GIT_TEST_LONG
- # Run command; redirect its stderr to &4 as in
- # test_run_, but keep its stdout on our stdout even in
- # non-verbose mode.
- "$@" 2>&4
- if test "$?" = 0
- then
- if test $test_external_has_tap -eq 0; then
- test_ok_ "$descr"
- else
- say_color "" "# test_external test $descr was ok"
- test_success=$(($test_success + 1))
- fi
- else
- if test $test_external_has_tap -eq 0; then
- test_failure_ "$descr" "$@"
- else
- say_color error "# test_external test $descr failed: $@"
- test_failure=$(($test_failure + 1))
- fi
- fi
- fi
-}
-
-# Like test_external, but in addition tests that the command generated
-# no output on stderr.
-test_external_without_stderr () {
- # The temporary file has no (and must have no) security
- # implications.
- tmp=${TMPDIR:-/tmp}
- stderr="$tmp/git-external-stderr.$$.tmp"
- test_external "$@" 4> "$stderr"
- test -f "$stderr" || error "Internal error: $stderr disappeared."
- descr="no stderr: $1"
- shift
- say >&3 "# expecting no stderr from previous command"
- if test ! -s "$stderr"
- then
- rm "$stderr"
-
- if test $test_external_has_tap -eq 0; then
- test_ok_ "$descr"
- else
- say_color "" "# test_external_without_stderr test $descr was ok"
- test_success=$(($test_success + 1))
- fi
- else
- if test "$verbose" = t
- then
- output=$(echo; echo "# Stderr is:"; cat "$stderr")
- else
- output=
- fi
- # rm first in case test_failure exits.
- rm "$stderr"
- if test $test_external_has_tap -eq 0; then
- test_failure_ "$descr" "$@" "$output"
- else
- say_color error "# test_external_without_stderr test $descr failed: $@: $output"
- test_failure=$(($test_failure + 1))
- fi
- fi
-}
-
# debugging-friendly alternatives to "test [-f|-d|-e]"
# The commands test the existence or non-existence of $1
test_path_is_file () {
diff --git a/t/test-lib.sh b/t/test-lib.sh
index d3f6af6a654..b0a8bc42510 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -631,8 +631,6 @@ test_fixed=0
test_broken=0
test_success=0
-test_external_has_tap=0
-
die () {
code=$?
# This is responsible for running the atexit commands even when a
@@ -1146,27 +1144,24 @@ test_done () {
fi
case "$test_failure" in
0)
- if test $test_external_has_tap -eq 0
+ if test $test_remaining -gt 0
then
- if test $test_remaining -gt 0
- then
- say_color pass "# passed all $msg"
- fi
-
- # Maybe print SKIP message
- test -z "$skip_all" || skip_all="# SKIP $skip_all"
- case "$test_count" in
- 0)
- say "1..$test_count${skip_all:+ $skip_all}"
- ;;
- *)
- test -z "$skip_all" ||
- say_color warn "$skip_all"
- say "1..$test_count"
- ;;
- esac
+ say_color pass "# passed all $msg"
fi
+ # Maybe print SKIP message
+ test -z "$skip_all" || skip_all="# SKIP $skip_all"
+ case "$test_count" in
+ 0)
+ say "1..$test_count${skip_all:+ $skip_all}"
+ ;;
+ *)
+ test -z "$skip_all" ||
+ say_color warn "$skip_all"
+ say "1..$test_count"
+ ;;
+ esac
+
if test -z "$debug"
then
test -d "$TRASH_DIRECTORY" ||
@@ -1185,11 +1180,8 @@ test_done () {
exit 0 ;;
*)
- if test $test_external_has_tap -eq 0
- then
- say_color error "# failed $test_failure among $msg"
- say "1..$test_count"
- fi
+ say_color error "# failed $test_failure among $msg"
+ say "1..$test_count"
exit 1 ;;
--
2.31.0.rc1.210.g0f8085a843c
^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: [PATCH 1/7] test-lib: remove test_external
2021-03-09 16:02 ` [PATCH 1/7] test-lib: remove test_external Ævar Arnfjörð Bjarmason
@ 2021-03-10 1:04 ` Junio C Hamano
2021-03-10 2:22 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 47+ messages in thread
From: Junio C Hamano @ 2021-03-10 1:04 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jeff King, SZEDER Gábor, Johannes Schindelin
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> Remove the test_external function(s) in favor of running the Perl
> tests with a test_expect_success.
> ...
> My motivation for this is to eliminate a special case where things
> that aren't test-lib.sh are going to produce TAP, for reasons that'll
> be clear in subsequent commits.
Puzzled.
> .../netrc/t-git-credential-netrc.sh | 7 +-
> t/README | 26 ------
> t/t0202-gettext-perl.sh | 10 +--
> t/t9700-perl-git.sh | 10 +--
> t/test-lib-functions.sh | 89 +------------------
> t/test-lib.sh | 42 ++++-----
> 6 files changed, 28 insertions(+), 156 deletions(-)
Reducing the number of lines is always good, but is this essentially
losing what the commit that added test_external wanted to add?
> diff --git a/contrib/credential/netrc/t-git-credential-netrc.sh b/contrib/credential/netrc/t-git-credential-netrc.sh
> index 07227d02287..28118a9e194 100755
> --- a/contrib/credential/netrc/t-git-credential-netrc.sh
> +++ b/contrib/credential/netrc/t-git-credential-netrc.sh
> @@ -20,13 +20,10 @@
> 'set up test repository' \
> 'git config --add gpg.program test.git-config-gpg'
>
> - # The external test will outputs its own plan
> - test_external_has_tap=1
> -
> export PERL5LIB="$GITPERLLIB"
> - test_external \
> - 'git-credential-netrc' \
> + test_expect_success 'git-credential-netrc' '
> perl "$GIT_BUILD_DIR"/contrib/credential/netrc/test.pl
> + '
>
> test_done
> )
This is valid because we expect nobody runs this under tap?
> diff --git a/t/t0202-gettext-perl.sh b/t/t0202-gettext-perl.sh
> index a29d166e007..06a93b36790 100755
> --- a/t/t0202-gettext-perl.sh
> +++ b/t/t0202-gettext-perl.sh
> @@ -17,11 +17,9 @@ perl -MTest::More -e 0 2>/dev/null || {
> test_done
> }
>
> -# The external test will outputs its own plan
> -test_external_has_tap=1
> -
> -test_external_without_stderr \
> - 'Perl Git::I18N API' \
> - perl "$TEST_DIRECTORY"/t0202/test.pl
> +test_expect_success 'run t0202/test.pl to test Git::I18N.pm' '
> + perl "$TEST_DIRECTORY"/t0202/test.pl 2>stderr &&
> + test_must_be_empty stderr
> +'
So t0202/test.pl would still produce output that would confuse
whoever is reading our output as TAP, and it is OK? If the
redirection discards its standard output to /dev/null [*], I would
sort-of understand how this may work (we would have let the perl
script to emit 13 "ok" or "not ok" to our output, but now we discard
that and write just one our own "ok" or "not ok", depending on what
comes out to the standard error stream (e.g. "# Looks like you
failed...").
But that is not what is going on. We'll let these 13 "ok" or "not ok"
come out from the perl script and then add another on our own.
> diff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh
> index 102c133112c..574c57b17f1 100755
> --- a/t/t9700-perl-git.sh
> +++ b/t/t9700-perl-git.sh
> @@ -50,11 +50,9 @@ test_expect_success \
> git config --add test.pathmulti bar
> '
>
> -# The external test will outputs its own plan
> -test_external_has_tap=1
> -
> -test_external_without_stderr \
> - 'Perl API' \
> - perl "$TEST_DIRECTORY"/t9700/test.pl
> +test_expect_success 'use t9700/test.pl to test Git.pm' '
> + perl "$TEST_DIRECTORY"/t9700/test.pl 2>stderr &&
> + test_must_be_empty stderr
> +'
Ditto. It seems that we are still letting the script, i.e. one of
the "things that aren't test-lib.sh" to produce TAP anyway.
[Footnote]
* If we are truly somehow discarding these output that would be TAP
(13 tests in 0202 and uncounted in 9700) from being shown (by e.g.
redirecting output to /dev/null), it would be a regression for those
who debug breakage found by these tests. They used to be told which
one failed and how but now they don't. So I do not think that is a
useful way to go, either.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 1/7] test-lib: remove test_external
2021-03-10 1:04 ` Junio C Hamano
@ 2021-03-10 2:22 ` Ævar Arnfjörð Bjarmason
0 siblings, 0 replies; 47+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-03-10 2:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, SZEDER Gábor, Johannes Schindelin
On Wed, Mar 10 2021, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> Remove the test_external function(s) in favor of running the Perl
>> tests with a test_expect_success.
>> ...
>> My motivation for this is to eliminate a special case where things
>> that aren't test-lib.sh are going to produce TAP, for reasons that'll
>> be clear in subsequent commits.
>
> Puzzled.
>
>> .../netrc/t-git-credential-netrc.sh | 7 +-
>> t/README | 26 ------
>> t/t0202-gettext-perl.sh | 10 +--
>> t/t9700-perl-git.sh | 10 +--
>> t/test-lib-functions.sh | 89 +------------------
>> t/test-lib.sh | 42 ++++-----
>> 6 files changed, 28 insertions(+), 156 deletions(-)
>
> Reducing the number of lines is always good, but is this essentially
> losing what the commit that added test_external wanted to add?
Yes, I don't think streaming TAP from external sources is a problem
worth solving in the first place.
>> diff --git a/contrib/credential/netrc/t-git-credential-netrc.sh b/contrib/credential/netrc/t-git-credential-netrc.sh
>> index 07227d02287..28118a9e194 100755
>> --- a/contrib/credential/netrc/t-git-credential-netrc.sh
>> +++ b/contrib/credential/netrc/t-git-credential-netrc.sh
>> @@ -20,13 +20,10 @@
>> 'set up test repository' \
>> 'git config --add gpg.program test.git-config-gpg'
>>
>> - # The external test will outputs its own plan
>> - test_external_has_tap=1
>> -
>> export PERL5LIB="$GITPERLLIB"
>> - test_external \
>> - 'git-credential-netrc' \
>> + test_expect_success 'git-credential-netrc' '
>> perl "$GIT_BUILD_DIR"/contrib/credential/netrc/test.pl
>> + '
>>
>> test_done
>> )
>
> This is valid because we expect nobody runs this under tap?
No, because at this point we suppress the --verbose output under
HARNESS_ACTIVE (but see 6/7 later), and under non-verbose we now emit:
./t9700-perl-git.sh
ok 1 - set up test repository
ok 2 - use t9700/test.pl to test Git.pm
# passed all 2 test(s)
1..2
So that the stdout from t9700/test.pl doesn't screw up the test output
anymore.
>> diff --git a/t/t0202-gettext-perl.sh b/t/t0202-gettext-perl.sh
>> index a29d166e007..06a93b36790 100755
>> --- a/t/t0202-gettext-perl.sh
>> +++ b/t/t0202-gettext-perl.sh
>> @@ -17,11 +17,9 @@ perl -MTest::More -e 0 2>/dev/null || {
>> test_done
>> }
>>
>> -# The external test will outputs its own plan
>> -test_external_has_tap=1
>> -
>> -test_external_without_stderr \
>> - 'Perl Git::I18N API' \
>> - perl "$TEST_DIRECTORY"/t0202/test.pl
>> +test_expect_success 'run t0202/test.pl to test Git::I18N.pm' '
>> + perl "$TEST_DIRECTORY"/t0202/test.pl 2>stderr &&
>> + test_must_be_empty stderr
>> +'
>
> So t0202/test.pl would still produce output that would confuse
> whoever is reading our output as TAP, and it is OK? If the
> redirection discards its standard output to /dev/null [*], I would
> sort-of understand how this may work (we would have let the perl
> script to emit 13 "ok" or "not ok" to our output, but now we discard
> that and write just one our own "ok" or "not ok", depending on what
> comes out to the standard error stream (e.g. "# Looks like you
> failed...").
>
> But that is not what is going on. We'll let these 13 "ok" or "not ok"
> come out from the perl script and then add another on our own.
Yes, but just like we don't have stdout from any other program appear
directly in the output of ./tXXXX*.sh the TAP doesn't get to our stderr
either, i.e. our whole juggling around with FDs 1-4.
>> diff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh
>> index 102c133112c..574c57b17f1 100755
>> --- a/t/t9700-perl-git.sh
>> +++ b/t/t9700-perl-git.sh
>> @@ -50,11 +50,9 @@ test_expect_success \
> > git config --add test.pathmulti bar
>> '
>>
>> -# The external test will outputs its own plan
>> -test_external_has_tap=1
>> -
>> -test_external_without_stderr \
>> - 'Perl API' \
>> - perl "$TEST_DIRECTORY"/t9700/test.pl
>> +test_expect_success 'use t9700/test.pl to test Git.pm' '
>> + perl "$TEST_DIRECTORY"/t9700/test.pl 2>stderr &&
>> + test_must_be_empty stderr
>> +'
>
> Ditto. It seems that we are still letting the script, i.e. one of
> the "things that aren't test-lib.sh" to produce TAP anyway.
>
>
> [Footnote]
>
> * If we are truly somehow discarding these output that would be TAP
> (13 tests in 0202 and uncounted in 9700) from being shown (by e.g.
> redirecting output to /dev/null), it would be a regression for those
> who debug breakage found by these tests. They used to be told which
> one failed and how but now they don't. So I do not think that is a
> useful way to go, either.
You still get the full TAP output under --verbose for debugging, we just
don't consume it as TAP anymore by "prove" etc, i.e. once we get to 6/7:
$ GIT_TEST_TEE_STARTED=true HARNESS_ACTIVE=t ./t9700-perl-git.sh --verbose --tee
[...]
ok 1 - set up test repository
## expecting success of 9700.2 'use t9700/test.pl to test Git.pm':
perl "$TEST_DIRECTORY"/t9700/test.pl 2>stderr &&
test_must_be_empty stderr
\ok 2 - use Git;
\ok 3 - open repository
\ok 4 - config scalar: string
\ok 5 - config array: string
\ok 6 - config scalar: nonexistent
\ok 7 - config array: nonexistent
[...]
\ok 44 - cat_blob(outside): size
\ok 45 - unquote unquoted path
\ok 46 - unquote simple quoted path
\ok 47 - unquote escape sequences
\1..47
ok 2 - use t9700/test.pl to test Git.pm
# passed all 2 test(s)
1..2
Notice how it's escaped now.
So yes, we don't get the benefit of having e.g. prove/CI say that our
6th test failed anymore in this case.
I think that's a very small price to pay for the benefit of not having
to monkeypatch Perl's TAP emitting in this case to do the back & forth
munging to safe the TAP output I'm doing in 6/7, and only need to do in
one place because I'd gotten rid of this special-case earlier.
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH 2/7] test-lib: add say_color_tap helper to emit TAP format
2021-03-01 13:54 ` Ævar Arnfjörð Bjarmason
2021-03-09 16:02 ` [PATCH 0/6 + 1] test-lib: make --verbose output valid TAP Ævar Arnfjörð Bjarmason
2021-03-09 16:02 ` [PATCH 1/7] test-lib: remove test_external Ævar Arnfjörð Bjarmason
@ 2021-03-09 16:02 ` Ævar Arnfjörð Bjarmason
2021-03-10 0:39 ` Junio C Hamano
2021-03-09 16:02 ` [PATCH 3/7] test-lib: color "ok" TAP directives green under --verbose (or -x) Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
7 siblings, 1 reply; 47+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-03-09 16:02 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, SZEDER Gábor, Johannes Schindelin,
Ævar Arnfjörð Bjarmason
Follow-up 5099b99d25 (test-lib: Adjust output to be valid TAP format,
2010-06-24) and make it easy to distinguishing when we're emitting TAP
directives (with the new "say_color_tap"), and when we just want to
say something with colored text "say_color".
This also makes it easier to do any follow-up changes where we'd like
to do something special with the TAP directives, as they're what
drives the test when we run under the prove(1) utility.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/test-lib.sh | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index b0a8bc42510..9a3a3d8d16d 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -562,6 +562,13 @@ else
}
fi
+say_color_tap() {
+ test "$#" = 2 ||
+ BUG "not 2 parameters to say_color_tap"
+
+ say_color "$1" "$2"
+}
+
TERM=dumb
export TERM
@@ -666,7 +673,7 @@ test_ok_ () {
write_junit_xml_testcase "$*"
fi
test_success=$(($test_success + 1))
- say_color "" "ok $test_count - $@"
+ say_color_tap "" "ok $test_count - $@"
}
test_failure_ () {
@@ -691,7 +698,7 @@ test_failure_ () {
write_junit_xml_testcase "$1" " $junit_insert"
fi
test_failure=$(($test_failure + 1))
- say_color error "not ok $test_count - $1"
+ say_color_tap error "not ok $test_count - $1"
shift
printf '%s\n' "$*" | sed -e 's/^/# /'
test "$immediate" = "" || { finalize_junit_xml; GIT_EXIT_OK=t; exit 1; }
@@ -712,7 +719,7 @@ test_known_broken_failure_ () {
write_junit_xml_testcase "$* (known breakage)"
fi
test_broken=$(($test_broken+1))
- say_color warn "not ok $test_count - $@ # TODO known breakage"
+ say_color_tap warn "not ok $test_count - $@ # TODO known breakage"
}
test_debug () {
@@ -1027,7 +1034,7 @@ test_skip () {
" <skipped message=\"$message\" />"
fi
- say_color skip "ok $test_count # skip $1 ($skipped_reason)"
+ say_color_tap skip "ok $test_count # skip $1 ($skipped_reason)"
: true
;;
*)
@@ -1153,12 +1160,12 @@ test_done () {
test -z "$skip_all" || skip_all="# SKIP $skip_all"
case "$test_count" in
0)
- say "1..$test_count${skip_all:+ $skip_all}"
+ say_color_tap "" "1..$test_count${skip_all:+ $skip_all}"
;;
*)
test -z "$skip_all" ||
say_color warn "$skip_all"
- say "1..$test_count"
+ say_color_tap "" "1..$test_count"
;;
esac
@@ -1181,7 +1188,7 @@ test_done () {
*)
say_color error "# failed $test_failure among $msg"
- say "1..$test_count"
+ say_color_tap "" "1..$test_count"
exit 1 ;;
--
2.31.0.rc1.210.g0f8085a843c
^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: [PATCH 2/7] test-lib: add say_color_tap helper to emit TAP format
2021-03-09 16:02 ` [PATCH 2/7] test-lib: add say_color_tap helper to emit TAP format Ævar Arnfjörð Bjarmason
@ 2021-03-10 0:39 ` Junio C Hamano
0 siblings, 0 replies; 47+ messages in thread
From: Junio C Hamano @ 2021-03-10 0:39 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jeff King, SZEDER Gábor, Johannes Schindelin
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> Follow-up 5099b99d25 (test-lib: Adjust output to be valid TAP format,
> 2010-06-24) and make it easy to distinguishing when we're emitting TAP
> directives (with the new "say_color_tap"), and when we just want to
> say something with colored text "say_color".
OK.
> @@ -666,7 +673,7 @@ test_ok_ () {
> write_junit_xml_testcase "$*"
> fi
> test_success=$(($test_success + 1))
> - say_color "" "ok $test_count - $@"
> + say_color_tap "" "ok $test_count - $@"
Not your fault, but let's not forget that we should clean this up
not to use "$@" here (and instead use $*).
> - say_color warn "not ok $test_count - $@ # TODO known breakage"
> + say_color_tap warn "not ok $test_count - $@ # TODO known breakage"
Likewise.
Yes, I know say_color (and hence its heir say_color_tap) accepts
multiple arguments and eventually pastes them into a single string
with "$*", but there is no point in producing multiple arguments out
of "$@" here in this case.
fn0 () {
cnt=0
for arg
do
echo "$cnt: <$arg>"
cnt=$(( cnt+1 ))
done
}
fn1 () {
fn0 "not - $@ # TODO"
}
fn1 1 "2 3"
--->
0: <not - 1>
1: <2 3 # TODO>
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH 3/7] test-lib: color "ok" TAP directives green under --verbose (or -x)
2021-03-01 13:54 ` Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2021-03-09 16:02 ` [PATCH 2/7] test-lib: add say_color_tap helper to emit TAP format Ævar Arnfjörð Bjarmason
@ 2021-03-09 16:02 ` Ævar Arnfjörð Bjarmason
2021-03-09 16:02 ` [PATCH 4/7] test-lib: add tee with TAP support to test-tool Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
7 siblings, 0 replies; 47+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-03-09 16:02 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, SZEDER Gábor, Johannes Schindelin,
Ævar Arnfjörð Bjarmason
Change the color of "ok" lines to be green when we run under --verbose
or -x.
When a test fails we color the "not ok" line red, but the "ok" lines
are not colored.
I think that's the right thing to do in the default output, which is
e.g.:
$ ./t0000-basic.sh
ok 1 - verify that the running shell supports "local"
ok 2 - .git/objects should be empty after git init in an empty repo
ok 3 - .git/objects should have 3 subdirectories
[...]
Having almost every line colored green in that output would be
distracting.
However, under --verbose we'll typically might emit 10-20 lines of
non-colored stderr and "info" colored output describing the test
itself.
When paging through that output always having the boundaries between
tests highlighted makes the output more readable.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/test-lib.sh | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 9a3a3d8d16d..8562b11b817 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -545,6 +545,7 @@ then
say_color_skip=$(tput setaf 4) # blue
say_color_warn=$(tput setaf 3) # brown/yellow
say_color_pass=$(tput setaf 2) # green
+ say_color_pass_all=$(tput bold; tput setaf 2) # bold green
say_color_info=$(tput setaf 6) # cyan
say_color_reset=$(tput sgr0)
say_color_="" # no formatting for normal text
@@ -673,7 +674,7 @@ test_ok_ () {
write_junit_xml_testcase "$*"
fi
test_success=$(($test_success + 1))
- say_color_tap "" "ok $test_count - $@"
+ say_color_tap "${verbose:+pass}" "ok $test_count - $@"
}
test_failure_ () {
--
2.31.0.rc1.210.g0f8085a843c
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH 4/7] test-lib: add tee with TAP support to test-tool
2021-03-01 13:54 ` Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2021-03-09 16:02 ` [PATCH 3/7] test-lib: color "ok" TAP directives green under --verbose (or -x) Ævar Arnfjörð Bjarmason
@ 2021-03-09 16:02 ` Ævar Arnfjörð Bjarmason
2021-03-09 16:02 ` [PATCH 5/7] test-lib: indent and format GIT_TEST_TEE_OUTPUT_FILE code Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
7 siblings, 0 replies; 47+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-03-09 16:02 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, SZEDER Gábor, Johannes Schindelin,
Ævar Arnfjörð Bjarmason
Add a 'test-tool tee' that behaves similarly to 'tee(1)', except that
it supports sanitizing the TAP emitted on stdout.
This is done by assuming that the TAP is emitted by a friendly library
that promises to prefix valid TAP directives with what we're going to
strip with --strip-prefix="".
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
Makefile | 1 +
t/helper/test-tee.c | 85 ++++++++++++++++++++++++++++++++++++++++++++
t/helper/test-tool.c | 1 +
t/helper/test-tool.h | 1 +
4 files changed, 88 insertions(+)
create mode 100644 t/helper/test-tee.c
diff --git a/Makefile b/Makefile
index dfb0f1000fa..d26b9d62ee9 100644
--- a/Makefile
+++ b/Makefile
@@ -742,6 +742,7 @@ TEST_BUILTINS_OBJS += test-string-list.o
TEST_BUILTINS_OBJS += test-submodule-config.o
TEST_BUILTINS_OBJS += test-submodule-nested-repo-config.o
TEST_BUILTINS_OBJS += test-subprocess.o
+TEST_BUILTINS_OBJS += test-tee.o
TEST_BUILTINS_OBJS += test-trace2.o
TEST_BUILTINS_OBJS += test-urlmatch-normalization.o
TEST_BUILTINS_OBJS += test-wildmatch.o
diff --git a/t/helper/test-tee.c b/t/helper/test-tee.c
new file mode 100644
index 00000000000..4ed34e9db93
--- /dev/null
+++ b/t/helper/test-tee.c
@@ -0,0 +1,85 @@
+#include "test-tool.h"
+#include "parse-options.h"
+#include "strbuf.h"
+
+static int line_has_tap(struct strbuf *line)
+{
+ /*
+ * This is a less permissive version of
+ * https://metacpan.org/release/Test-Harness/source/lib/TAP/Parser/Grammar.pm
+ */
+ if (starts_with(line->buf, "ok") ||
+ starts_with(line->buf, "not ok") ||
+ starts_with(line->buf, "1..") ||
+ starts_with(line->buf, "Bail out!") ||
+ starts_with(line->buf, "TAP version") ||
+ starts_with(line->buf, "pragma"))
+ return 1;
+ if (starts_with(line->buf, "#"))
+ /*
+ * We're ignoring comments from now, but might treat them
+ * specially in the future for sanctioned messaging from the
+ * test-lib.sh.
+ */
+ return 0;
+ return 0;
+}
+
+int cmd__tee(int argc, const char **argv)
+{
+ int tap = 0;
+ int escape_stdout = 0, escape_file = 0;
+ char *prefix = NULL;
+ size_t prefix_len = 0;
+ const char *tee_usage[] = {
+ "test-tool tee [<options>] <FILE>",
+ NULL
+ };
+ struct option options[] = {
+ OPT_BOOL(0, "escape-stdout", &escape_stdout,
+ "escape output on stdout"),
+ OPT_BOOL(0, "escape-file", &escape_file,
+ "escape output written to file"),
+ OPT_BOOL(0, "tap", &tap,
+ "output is TAP"),
+ OPT_STRING(0, "prefix", &prefix, "str",
+ "prefix to strip from the output"),
+ OPT_END()
+ };
+ struct strbuf line = STRBUF_INIT;
+ FILE *logfp = NULL;
+
+ argc = parse_options(argc, argv, NULL, options,
+ tee_usage, PARSE_OPT_STOP_AT_NON_OPTION);
+ if (argc > 1) {
+ fprintf(stderr, "got bad option: %s\n", argv[0]);
+ usage_with_options(tee_usage, options);
+ }
+ if (prefix)
+ prefix_len = strlen(prefix);
+
+ if (argc)
+ logfp = xfopen(argv[0], "w");
+
+ while (strbuf_getline(&line, stdin) != EOF) {
+ size_t offs = 0;
+ if (prefix && starts_with(line.buf, prefix))
+ offs = prefix_len;
+
+ if (!offs && tap && line_has_tap(&line)) {
+ if (escape_stdout)
+ fprintf(stdout, "\\");
+ if (logfp && escape_file)
+ fprintf(logfp, "\\");
+ }
+
+ fprintf(stdout, "%s\n", line.buf + offs);
+ if (logfp)
+ fprintf(logfp, "%s\n", line.buf + offs);
+ }
+ strbuf_release(&line);
+ if (logfp)
+ fclose(logfp);
+
+ return 0;
+}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index f97cd9f48a6..1876bad8f42 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -70,6 +70,7 @@ static struct test_cmd cmds[] = {
{ "submodule-config", cmd__submodule_config },
{ "submodule-nested-repo-config", cmd__submodule_nested_repo_config },
{ "subprocess", cmd__subprocess },
+ { "tee", cmd__tee },
{ "trace2", cmd__trace2 },
{ "urlmatch-normalization", cmd__urlmatch_normalization },
{ "xml-encode", cmd__xml_encode },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 28072c0ad5a..9b3c1f75267 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -60,6 +60,7 @@ int cmd__string_list(int argc, const char **argv);
int cmd__submodule_config(int argc, const char **argv);
int cmd__submodule_nested_repo_config(int argc, const char **argv);
int cmd__subprocess(int argc, const char **argv);
+int cmd__tee(int argc, const char **argv);
int cmd__trace2(int argc, const char **argv);
int cmd__urlmatch_normalization(int argc, const char **argv);
int cmd__xml_encode(int argc, const char **argv);
--
2.31.0.rc1.210.g0f8085a843c
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH 5/7] test-lib: indent and format GIT_TEST_TEE_OUTPUT_FILE code
2021-03-01 13:54 ` Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2021-03-09 16:02 ` [PATCH 4/7] test-lib: add tee with TAP support to test-tool Ævar Arnfjörð Bjarmason
@ 2021-03-09 16:02 ` Ævar Arnfjörð Bjarmason
2021-03-09 16:02 ` [PATCH 6/7] test-lib: make --verbose output valid TAP Ævar Arnfjörð Bjarmason
2021-03-09 16:02 ` [RFC/PATCH 7/7] test-lib: generate JUnit output via TAP Ævar Arnfjörð Bjarmason
7 siblings, 0 replies; 47+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-03-09 16:02 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, SZEDER Gábor, Johannes Schindelin,
Ævar Arnfjörð Bjarmason
Change the indentation and formatting of the GIT_TEST_TEE_OUTPUT_FILE
code block added in 44138559e8 (test-lib.sh: optionally output to
test-results/$TEST.out, too, 2009-02-04).
This brings it in line with the general style in our test library, and
makes it easier to read modifications of it in subsequent commits.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/test-lib.sh | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 8562b11b817..aa7068b06b6 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -361,8 +361,10 @@ then
# from any previous runs.
>"$GIT_TEST_TEE_OUTPUT_FILE"
- (GIT_TEST_TEE_STARTED=done ${TEST_SHELL_PATH} "$0" "$@" 2>&1;
- echo $? >"$TEST_RESULTS_BASE.exit") | tee -a "$GIT_TEST_TEE_OUTPUT_FILE"
+ (
+ GIT_TEST_TEE_STARTED=done ${TEST_SHELL_PATH} "$0" "$@" 2>&1
+ echo $? >"$TEST_RESULTS_BASE.exit"
+ ) | tee -a "$GIT_TEST_TEE_OUTPUT_FILE"
test "$(cat "$TEST_RESULTS_BASE.exit")" = 0
exit
fi
--
2.31.0.rc1.210.g0f8085a843c
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH 6/7] test-lib: make --verbose output valid TAP
2021-03-01 13:54 ` Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2021-03-09 16:02 ` [PATCH 5/7] test-lib: indent and format GIT_TEST_TEE_OUTPUT_FILE code Ævar Arnfjörð Bjarmason
@ 2021-03-09 16:02 ` Ævar Arnfjörð Bjarmason
2021-03-09 18:59 ` SZEDER Gábor
` (3 more replies)
2021-03-09 16:02 ` [RFC/PATCH 7/7] test-lib: generate JUnit output via TAP Ævar Arnfjörð Bjarmason
7 siblings, 4 replies; 47+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-03-09 16:02 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, SZEDER Gábor, Johannes Schindelin,
Ævar Arnfjörð Bjarmason
Make the --verbose output be valid TAP, making it machine-readable for
TAP parsers again.
Both the verbose and non-verbose test outputs were valid TAP back when
I added support for TAP in 5099b99d25f (test-lib: Adjust output to be
valid TAP format, 2010-06-24).
Sometime after that the --verbose output broke due to some tests
emitting their own lines starting "ok" (or otherwise invalidate the
TAP). That was noticed and fixed in 452320f1f5 (test-lib: add
--verbose-log option, 2016-10-21) and "fixed" by simply turning off
the verbose mode when we were running under TAP::Harness (e.g. under
"prove").
That solution worked for running under Travis CI. After that fix it
was made to use the --verbose-log option in 041c72de109 (travis: use
--verbose-log test option, 2016-10-21), see 522354d70f4 (Add Travis CI
support, 2015-11-27) for the "cat t/test-results/*.out" code that was
aimed at.
But that solution and others discussed in 452320f1f5 closed the door
on us having reliable machine-readable TAP output.
Let's instead revert the work done in 452320f1f5 and, as well as the
follow-up commits 88c6e9d31c (test-lib: --valgrind should not override
--verbose-log, 2017-09-05) and f5ba2de6bc (test-lib: make "-x" work
with "--verbose-log", 2017-12-08), which were only needed to work
around bugs in the the previous --verbose-log implementation.
Replace it with a simple method for ensuring that we have valid TAP
both on stdout, and in any verbose output we write. When we detect
that we're running under "prove" we prefix all legitimate TAP
directives with "GIT_TEST_TEE_STARTED":
$ GIT_TEST_TEE_STARTED=1 ./t5547-push-quarantine.sh
GIT_TEST_TEE_STARTED ok 1 - create picky dest repo
GIT_TEST_TEE_STARTED ok 2 - accepted objects work
[...]
GIT_TEST_TEE_STARTED 1..6
Then, instead of piping the output to "tee -a" we pipe it to a helper
which first converts "ok" and other TAP syntax to e.g. "\ok", and then
strips that "GIT_TEST_TEE_STARTED " prefix from the start of the line.
The end result is that we're guaranteed to have valid TAP syntax on
stdout.
We can thus get rid of the --verbose-log special-case. Since that
option was meant to get around the TAP issue let's simply make it an
alias for "--verbose --tee".
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/README | 9 ++++++---
t/t0000-basic.sh | 8 ++++++++
t/test-lib.sh | 43 +++++++++++++++++++++----------------------
3 files changed, 35 insertions(+), 25 deletions(-)
diff --git a/t/README b/t/README
index 2cc8cbc7185..f09d94e754e 100644
--- a/t/README
+++ b/t/README
@@ -157,10 +157,13 @@ appropriately before running "make". Short options can be bundled, i.e.
-V::
--verbose-log::
- Write verbose output to the same logfile as `--tee`, but do
- _not_ write it to stdout. Unlike `--tee --verbose`, this option
+ An alias for `--verbose --tee`. This option
is safe to use when stdout is being consumed by a TAP parser
- like `prove`. Implies `--tee` and `--verbose`.
+ like `prove`.
+ Historically this option was different from `--verbose --tee`
+ and would not write any verbose output to stdout to ensure the
+ TAP-correctness of the output. The TAP-correctness of the
+ output is now sanity checked by the test library,
--with-dashes::
By default tests are run without dashed forms of
diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 705d62cc27a..2a95a612fb0 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -76,6 +76,14 @@ _run_sub_test_lib_test_common () {
# this variable, so we need a stable setting under which to run
# the sub-test.
sane_unset HARNESS_ACTIVE &&
+
+ # When under --tee pretend we're not as far as
+ # emitting the "GIT_TEST_TEE_STARTED " TAP sanity
+ # prefix goes. This means we promise not to emit
+ # invalid TAP, least this test fails under --tee under
+ # prove(1).
+ sane_unset GIT_TEST_TEE_STARTED &&
+
cd "$name" &&
write_script "$name.sh" "$TEST_SHELL_PATH" <<-EOF &&
test_description='$descr (run in sub test-lib)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index aa7068b06b6..0070d05234b 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -146,9 +146,11 @@ parse_option () {
--no-chain-lint)
GIT_TEST_CHAIN_LINT=0 ;;
-x)
- trace=t ;;
+ trace=t
+ verbose=t
+ ;;
-V|--verbose-log)
- verbose_log=t
+ verbose=t
tee=t
;;
--write-junit-xml)
@@ -233,7 +235,7 @@ then
test -z "$verbose" && verbose_only="$valgrind_only"
elif test -n "$valgrind"
then
- test -z "$verbose_log" && verbose=t
+ verbose=t
fi
if test -n "$stress"
@@ -364,9 +366,22 @@ then
(
GIT_TEST_TEE_STARTED=done ${TEST_SHELL_PATH} "$0" "$@" 2>&1
echo $? >"$TEST_RESULTS_BASE.exit"
- ) | tee -a "$GIT_TEST_TEE_OUTPUT_FILE"
+ ) | "$GIT_BUILD_DIR"/t/helper/test-tool tee \
+ --tap --prefix="GIT_TEST_TEE_STARTED " \
+ --escape-stdout ${HARNESS_ACTIVE+--escape-file} \
+ "$GIT_TEST_TEE_OUTPUT_FILE"
test "$(cat "$TEST_RESULTS_BASE.exit")" = 0
exit
+elif test -n "$verbose" -a -n "$HARNESS_ACTIVE"
+then
+ ret=
+ (
+ GIT_TEST_TEE_STARTED=done ${TEST_SHELL_PATH} "$0" "$@" 2>&1
+ ret=$?
+ ) | "$GIT_BUILD_DIR"/t/helper/test-tool tee \
+ --tap --prefix="GIT_TEST_TEE_STARTED " \
+ --escape-stdout
+ exit $ret
fi
if test -n "$trace" && test -n "$test_untraceable"
@@ -392,10 +407,6 @@ then
trace=
fi
fi
-if test -n "$trace" && test -z "$verbose_log"
-then
- verbose=t
-fi
# For repeatability, reset the environment to known value.
# TERM is sanitized below, after saving color control sequences.
@@ -569,7 +580,7 @@ say_color_tap() {
test "$#" = 2 ||
BUG "not 2 parameters to say_color_tap"
- say_color "$1" "$2"
+ say_color "$1" "${GIT_TEST_TEE_STARTED:+GIT_TEST_TEE_STARTED }$2"
}
TERM=dumb
@@ -590,16 +601,6 @@ say () {
say_color info "$*"
}
-if test -n "$HARNESS_ACTIVE"
-then
- if test "$verbose" = t || test -n "$verbose_only"
- then
- printf 'Bail out! %s\n' \
- 'verbose mode forbidden under TAP harness; try --verbose-log'
- exit 1
- fi
-fi
-
test "${test_description}" != "" ||
error "Test script did not set test_description."
@@ -887,9 +888,7 @@ maybe_setup_valgrind () {
trace_level_=0
want_trace () {
- test "$trace" = t && {
- test "$verbose" = t || test "$verbose_log" = t
- }
+ test "$trace" = t && test "$verbose" = t
}
# This is a separate function because some tests use
--
2.31.0.rc1.210.g0f8085a843c
^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: [PATCH 6/7] test-lib: make --verbose output valid TAP
2021-03-09 16:02 ` [PATCH 6/7] test-lib: make --verbose output valid TAP Ævar Arnfjörð Bjarmason
@ 2021-03-09 18:59 ` SZEDER Gábor
2021-03-09 20:57 ` Ævar Arnfjörð Bjarmason
2021-03-09 19:12 ` SZEDER Gábor
` (2 subsequent siblings)
3 siblings, 1 reply; 47+ messages in thread
From: SZEDER Gábor @ 2021-03-09 18:59 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Jeff King, Johannes Schindelin
On Tue, Mar 09, 2021 at 05:02:18PM +0100, Ævar Arnfjörð Bjarmason wrote:
> Make the --verbose output be valid TAP, making it machine-readable for
> TAP parsers again.
>
> Both the verbose and non-verbose test outputs were valid TAP back when
> I added support for TAP in 5099b99d25f (test-lib: Adjust output to be
> valid TAP format, 2010-06-24).
>
> Sometime after that the --verbose output broke due to some tests
> emitting their own lines starting "ok" (or otherwise invalidate the
> TAP). That was noticed and fixed in 452320f1f5 (test-lib: add
> --verbose-log option, 2016-10-21) and "fixed" by simply turning off
> the verbose mode when we were running under TAP::Harness (e.g. under
> "prove").
>
> That solution worked for running under Travis CI. After that fix it
> was made to use the --verbose-log option in 041c72de109 (travis: use
> --verbose-log test option, 2016-10-21), see 522354d70f4 (Add Travis CI
> support, 2015-11-27) for the "cat t/test-results/*.out" code that was
> aimed at.
>
> But that solution and others discussed in 452320f1f5 closed the door
> on us having reliable machine-readable TAP output.
>
> Let's instead revert the work done in 452320f1f5 and, as well as the
> follow-up commits 88c6e9d31c (test-lib: --valgrind should not override
> --verbose-log, 2017-09-05) and f5ba2de6bc (test-lib: make "-x" work
> with "--verbose-log", 2017-12-08), which were only needed to work
> around bugs in the the previous --verbose-log implementation.
>
> Replace it with a simple method for ensuring that we have valid TAP
> both on stdout, and in any verbose output we write. When we detect
> that we're running under "prove" we prefix all legitimate TAP
> directives with "GIT_TEST_TEE_STARTED":
>
> $ GIT_TEST_TEE_STARTED=1 ./t5547-push-quarantine.sh
> GIT_TEST_TEE_STARTED ok 1 - create picky dest repo
> GIT_TEST_TEE_STARTED ok 2 - accepted objects work
> [...]
> GIT_TEST_TEE_STARTED 1..6
>
> Then, instead of piping the output to "tee -a" we pipe it to a helper
> which first converts "ok" and other TAP syntax to e.g. "\ok", and then
> strips that "GIT_TEST_TEE_STARTED " prefix from the start of the line.
>
> The end result is that we're guaranteed to have valid TAP syntax on
> stdout.
>
> We can thus get rid of the --verbose-log special-case. Since that
> option was meant to get around the TAP issue let's simply make it an
> alias for "--verbose --tee".
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
After applying this patch series there is still one place where we
look at $verbose_log:
$ git grep -C4 verbose_log -- test-lib.sh
test-lib.sh-
test-lib.sh-exec 5>&1
test-lib.sh-exec 6<&0
test-lib.sh-exec 7>&2
test-lib.sh:if test "$verbose_log" = "t"
test-lib.sh-then
test-lib.sh- exec 3>>"$GIT_TEST_TEE_OUTPUT_FILE" 4>&3
test-lib.sh-elif test "$verbose" = "t"
test-lib.sh-then
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 6/7] test-lib: make --verbose output valid TAP
2021-03-09 18:59 ` SZEDER Gábor
@ 2021-03-09 20:57 ` Ævar Arnfjörð Bjarmason
2021-03-09 21:31 ` SZEDER Gábor
0 siblings, 1 reply; 47+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-03-09 20:57 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: git, Junio C Hamano, Jeff King, Johannes Schindelin
On Tue, Mar 09 2021, SZEDER Gábor wrote:
> On Tue, Mar 09, 2021 at 05:02:18PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> Make the --verbose output be valid TAP, making it machine-readable for
>> TAP parsers again.
>>
>> Both the verbose and non-verbose test outputs were valid TAP back when
>> I added support for TAP in 5099b99d25f (test-lib: Adjust output to be
>> valid TAP format, 2010-06-24).
>>
>> Sometime after that the --verbose output broke due to some tests
>> emitting their own lines starting "ok" (or otherwise invalidate the
>> TAP). That was noticed and fixed in 452320f1f5 (test-lib: add
>> --verbose-log option, 2016-10-21) and "fixed" by simply turning off
>> the verbose mode when we were running under TAP::Harness (e.g. under
>> "prove").
>>
>> That solution worked for running under Travis CI. After that fix it
>> was made to use the --verbose-log option in 041c72de109 (travis: use
>> --verbose-log test option, 2016-10-21), see 522354d70f4 (Add Travis CI
>> support, 2015-11-27) for the "cat t/test-results/*.out" code that was
>> aimed at.
>>
>> But that solution and others discussed in 452320f1f5 closed the door
>> on us having reliable machine-readable TAP output.
>>
>> Let's instead revert the work done in 452320f1f5 and, as well as the
>> follow-up commits 88c6e9d31c (test-lib: --valgrind should not override
>> --verbose-log, 2017-09-05) and f5ba2de6bc (test-lib: make "-x" work
>> with "--verbose-log", 2017-12-08), which were only needed to work
>> around bugs in the the previous --verbose-log implementation.
>>
>> Replace it with a simple method for ensuring that we have valid TAP
>> both on stdout, and in any verbose output we write. When we detect
>> that we're running under "prove" we prefix all legitimate TAP
>> directives with "GIT_TEST_TEE_STARTED":
>>
>> $ GIT_TEST_TEE_STARTED=1 ./t5547-push-quarantine.sh
>> GIT_TEST_TEE_STARTED ok 1 - create picky dest repo
>> GIT_TEST_TEE_STARTED ok 2 - accepted objects work
>> [...]
>> GIT_TEST_TEE_STARTED 1..6
>>
>> Then, instead of piping the output to "tee -a" we pipe it to a helper
>> which first converts "ok" and other TAP syntax to e.g. "\ok", and then
>> strips that "GIT_TEST_TEE_STARTED " prefix from the start of the line.
>>
>> The end result is that we're guaranteed to have valid TAP syntax on
>> stdout.
>>
>> We can thus get rid of the --verbose-log special-case. Since that
>> option was meant to get around the TAP issue let's simply make it an
>> alias for "--verbose --tee".
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
[Relpying to both replies in this sub-thread]
> After applying this patch series there is still one place where we
> look at $verbose_log:
>
> $ git grep -C4 verbose_log -- test-lib.sh
> test-lib.sh-
> test-lib.sh-exec 5>&1
> test-lib.sh-exec 6<&0
> test-lib.sh-exec 7>&2
> test-lib.sh:if test "$verbose_log" = "t"
> test-lib.sh-then
> test-lib.sh- exec 3>>"$GIT_TEST_TEE_OUTPUT_FILE" 4>&3
> test-lib.sh-elif test "$verbose" = "t"
> test-lib.sh-then
Yes, well spotted. The way this patch is implemented that exec branch
should have been deleted. I just missed it.
[From your <20210309191230.GG3590451@szeder.dev>]:
>> diff --git a/t/README b/t/README
>> index 2cc8cbc7185..f09d94e754e 100644
>> --- a/t/README
>> +++ b/t/README
>> @@ -157,10 +157,13 @@ appropriately before running "make". Short options can be bundled, i.e.
>>
>> -V::
>> --verbose-log::
>> - Write verbose output to the same logfile as `--tee`, but do
>> - _not_ write it to stdout. Unlike `--tee --verbose`, this option
>> + An alias for `--verbose --tee`. This option
>> is safe to use when stdout is being consumed by a TAP parser
>> - like `prove`. Implies `--tee` and `--verbose`.
>> + like `prove`.
>> + Historically this option was different from `--verbose --tee`
>> + and would not write any verbose output to stdout to ensure the
>> + TAP-correctness of the output. The TAP-correctness of the
>> + output is now sanity checked by the test library,
>
> Not everyone is using a TAP harness to run the tests, and, therefore,
> '--verbose-log' should not spew out verbose output to the terminal.
IOW even though --verbose-log was meant as a hack to make prove happy,
you've since come to like the "verbose in log, but not stdout" mode and
want that kept?
Yes, this patch takes that mode away.
Yes, I can change it.
Would your use-case for this be satisfied by having prove(1) just emit
different output for you in this scenario, so you'd need to invoke this
as something like:
prove <test> :: --verbose --tee # or --verbose-log
Becuse the advantage of this series is that that sort of thing becomes
really trivial without everything needing to be hardcoded in
test-lib.sh, observe (this is with my series):
0 $ PERL5LIB=. prove -v --formatter=SZEDERVerboseLog ./t0201-gettext-fallbacks.sh :: --verbose-log
# lib-gettext: No is_IS UTF-8 locale available
# lib-gettext: No is_IS ISO-8859-1 locale available
ok 1 - sanity: $GIT_INTERNAL_GETTEXT_SH_SCHEME is set (to fallthrough)
ok 2 - sanity: $GIT_INTERNAL_GETTEXT_TEST_FALLBACKS is set
ok 3 - sanity: $GIT_INTERNAL_GETTEXT_SH_SCHEME" is fallthrough
ok 4 - gettext: our gettext() fallback has pass-through semantics
ok 5 - eval_gettext: our eval_gettext() fallback has pass-through semantics
ok 6 - eval_gettext: our eval_gettext() fallback can interpolate variables
ok 7 - eval_gettext: our eval_gettext() fallback can interpolate variables with spaces
ok 8 - eval_gettext: our eval_gettext() fallback can interpolate variables with spaces and quotes
# passed all 8 test(s)
1..8
ok
All tests successful.
Files=1, Tests=8, 1 wallclock secs ( 0.01 usr 0.01 sys + 0.07 cusr 0.01 csys = 0.10 CPU)
Result: PASS
$ wc -l test-results/t0201-gettext-fallbacks.out
75 test-results/t0201-gettext-fallbacks.out
All without any patching on top to the test-lib.sh, just:
$ cat SZEDERVerboseLog.pm
package SZEDERVerboseLog::Session;
use base 'TAP::Formatter::Console::Session';
sub result {
my ($self, $result) = @_;
return if $result->is_unknown;
return $self->SUPER::result($result);
}
package SZEDERVerboseLog;
use strict;
use warnings;
use base 'TAP::Formatter::Console';
sub open_test {
my ($self, $test, $parser) = @_;
my $session = SZEDERVerboseLog::Session->new( {
name => $test,
formatter => $self,
parser => $parser,
} );
return $session;
}
1;
The "is_unknown" is everything that's not TAP syntax.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 6/7] test-lib: make --verbose output valid TAP
2021-03-09 20:57 ` Ævar Arnfjörð Bjarmason
@ 2021-03-09 21:31 ` SZEDER Gábor
2021-03-10 2:35 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 47+ messages in thread
From: SZEDER Gábor @ 2021-03-09 21:31 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Jeff King, Johannes Schindelin
On Tue, Mar 09, 2021 at 09:57:03PM +0100, Ævar Arnfjörð Bjarmason wrote:
> >> diff --git a/t/README b/t/README
> >> index 2cc8cbc7185..f09d94e754e 100644
> >> --- a/t/README
> >> +++ b/t/README
> >> @@ -157,10 +157,13 @@ appropriately before running "make". Short options can be bundled, i.e.
> >>
> >> -V::
> >> --verbose-log::
> >> - Write verbose output to the same logfile as `--tee`, but do
> >> - _not_ write it to stdout. Unlike `--tee --verbose`, this option
> >> + An alias for `--verbose --tee`. This option
> >> is safe to use when stdout is being consumed by a TAP parser
> >> - like `prove`. Implies `--tee` and `--verbose`.
> >> + like `prove`.
> >> + Historically this option was different from `--verbose --tee`
> >> + and would not write any verbose output to stdout to ensure the
> >> + TAP-correctness of the output. The TAP-correctness of the
> >> + output is now sanity checked by the test library,
> >
> > Not everyone is using a TAP harness to run the tests, and, therefore,
> > '--verbose-log' should not spew out verbose output to the terminal.
>
> IOW even though --verbose-log was meant as a hack to make prove happy,
> you've since come to like the "verbose in log, but not stdout" mode and
> want that kept?
Yes, '--verbose-log' proved to be really convenient, even if it was
meant to solve a different issue.
> Yes, this patch takes that mode away.
>
> Yes, I can change it.
>
> Would your use-case for this be satisfied by having prove(1) just emit
> different output for you in this scenario, so you'd need to invoke this
> as something like:
>
> prove <test> :: --verbose --tee # or --verbose-log
I use prove to run the test suite, but I don't and won't use prove to
run a single test.
The behavior of './t1234-foo.sh -V' with or without '-x' should not
change without _very_ convincing reasons. "We now output valid TAP
even with --verbose, so we don't need it for the TAP harness" is
definitely not convincing.
> Becuse the advantage of this series is that that sort of thing becomes
> really trivial without everything needing to be hardcoded in
> test-lib.sh, observe (this is with my series):
>
>
> 0 $ PERL5LIB=. prove -v --formatter=SZEDERVerboseLog ./t0201-gettext-fallbacks.sh :: --verbose-log
Well, this doesn't look trivial at all, does it? In fact, I consider
this unusably convoluted.
> # lib-gettext: No is_IS UTF-8 locale available
> # lib-gettext: No is_IS ISO-8859-1 locale available
> ok 1 - sanity: $GIT_INTERNAL_GETTEXT_SH_SCHEME is set (to fallthrough)
> ok 2 - sanity: $GIT_INTERNAL_GETTEXT_TEST_FALLBACKS is set
> ok 3 - sanity: $GIT_INTERNAL_GETTEXT_SH_SCHEME" is fallthrough
> ok 4 - gettext: our gettext() fallback has pass-through semantics
> ok 5 - eval_gettext: our eval_gettext() fallback has pass-through semantics
> ok 6 - eval_gettext: our eval_gettext() fallback can interpolate variables
> ok 7 - eval_gettext: our eval_gettext() fallback can interpolate variables with spaces
> ok 8 - eval_gettext: our eval_gettext() fallback can interpolate variables with spaces and quotes
> # passed all 8 test(s)
> 1..8
> ok
> All tests successful.
> Files=1, Tests=8, 1 wallclock secs ( 0.01 usr 0.01 sys + 0.07 cusr 0.01 csys = 0.10 CPU)
> Result: PASS
> $ wc -l test-results/t0201-gettext-fallbacks.out
> 75 test-results/t0201-gettext-fallbacks.out
>
> All without any patching on top to the test-lib.sh, just:
>
> $ cat SZEDERVerboseLog.pm
> package SZEDERVerboseLog::Session;
> use base 'TAP::Formatter::Console::Session';
>
> sub result {
> my ($self, $result) = @_;
> return if $result->is_unknown;
> return $self->SUPER::result($result);
> }
>
> package SZEDERVerboseLog;
> use strict;
> use warnings;
> use base 'TAP::Formatter::Console';
>
> sub open_test {
> my ($self, $test, $parser) = @_;
> my $session = SZEDERVerboseLog::Session->new( {
> name => $test,
> formatter => $self,
> parser => $parser,
> } );
> return $session;
> }
>
> 1;
>
> The "is_unknown" is everything that's not TAP syntax.
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 6/7] test-lib: make --verbose output valid TAP
2021-03-09 21:31 ` SZEDER Gábor
@ 2021-03-10 2:35 ` Ævar Arnfjörð Bjarmason
2021-03-16 9:10 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 47+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-03-10 2:35 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: git, Junio C Hamano, Jeff King, Johannes Schindelin
On Tue, Mar 09 2021, SZEDER Gábor wrote:
> On Tue, Mar 09, 2021 at 09:57:03PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> >> diff --git a/t/README b/t/README
>> >> index 2cc8cbc7185..f09d94e754e 100644
>> >> --- a/t/README
>> >> +++ b/t/README
>> >> @@ -157,10 +157,13 @@ appropriately before running "make". Short options can be bundled, i.e.
>> >>
>> >> -V::
>> >> --verbose-log::
>> >> - Write verbose output to the same logfile as `--tee`, but do
>> >> - _not_ write it to stdout. Unlike `--tee --verbose`, this option
>> >> + An alias for `--verbose --tee`. This option
>> >> is safe to use when stdout is being consumed by a TAP parser
>> >> - like `prove`. Implies `--tee` and `--verbose`.
>> >> + like `prove`.
>> >> + Historically this option was different from `--verbose --tee`
>> >> + and would not write any verbose output to stdout to ensure the
>> >> + TAP-correctness of the output. The TAP-correctness of the
>> >> + output is now sanity checked by the test library,
>> >
>> > Not everyone is using a TAP harness to run the tests, and, therefore,
>> > '--verbose-log' should not spew out verbose output to the terminal.
>>
>> IOW even though --verbose-log was meant as a hack to make prove happy,
>> you've since come to like the "verbose in log, but not stdout" mode and
>> want that kept?
>
> Yes, '--verbose-log' proved to be really convenient, even if it was
> meant to solve a different issue.
Sure, let's keep it then.
>> Yes, this patch takes that mode away.
>>
>> Yes, I can change it.
>>
>> Would your use-case for this be satisfied by having prove(1) just emit
>> different output for you in this scenario, so you'd need to invoke this
>> as something like:
>>
>> prove <test> :: --verbose --tee # or --verbose-log
>
> I use prove to run the test suite, but I don't and won't use prove to
> run a single test.
>
> The behavior of './t1234-foo.sh -V' with or without '-x' should not
> change without _very_ convincing reasons.
I think I can make it the same, except for emulating the sheared write
issue 452320f1f5 mentions. I.e. moving to this method inherently
requires us to squash together stdout/stderr into one, which isn't what
we're doing now.
> "We now output valid TAP even with --verbose, so we don't need it for
> the TAP harness" is definitely not convincing.
It's more like "we produce one machine-readable output format, so if you
need junit, or an abbreviated format (--verbose-log) or a nyancat
progress bar or whatever, it makes more sense to pipe it to another
process that'll munge that for you... :)
>> Becuse the advantage of this series is that that sort of thing becomes
>> really trivial without everything needing to be hardcoded in
>> test-lib.sh, observe (this is with my series):
>>
>>
>> 0 $ PERL5LIB=. prove -v --formatter=SZEDERVerboseLog ./t0201-gettext-fallbacks.sh :: --verbose-log
>
> Well, this doesn't look trivial at all, does it? In fact, I consider
> this unusably convoluted.
... I meant to say something closer to "does that output look
ok?". Obviously we'd then make the --verbose-log run something like that
under the hood.
But in any case, I found it easier to just add this feature to my "tee"
program than doing it with Perl's TAP libraries, i.e. something like
this on top (will integrate it in an eventual re-roll):
diff --git a/t/helper/test-tee.c b/t/helper/test-tee.c
index 4ed34e9db9..063b9277e0 100644
--- a/t/helper/test-tee.c
+++ b/t/helper/test-tee.c
@@ -29,2 +29,3 @@ int cmd__tee(int argc, const char **argv)
{
+ int only_tap_on_stdout = 0;
int tap = 0;
@@ -38,2 +39,4 @@ int cmd__tee(int argc, const char **argv)
struct option options[] = {
+ OPT_BOOL(0, "only-tap-on-stdout", &only_tap_on_stdout,
+ "only emit TAP on stdout, not 'unknown' etc."),
OPT_BOOL(0, "escape-stdout", &escape_stdout,
@@ -75,3 +78,7 @@ int cmd__tee(int argc, const char **argv)
- fprintf(stdout, "%s\n", line.buf + offs);
+ if (!only_tap_on_stdout)
+ fprintf(stdout, "%s\n", line.buf + offs);
+ else if (offs)
+ fprintf(stdout, "%s\n", line.buf + offs);
+
if (logfp)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 479ad4fb38..731ecc36bb 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -153,2 +153,3 @@ parse_option () {
verbose=t
+ verbose_log=t
tee=t
@@ -371,2 +372,3 @@ then
--escape-stdout ${HARNESS_ACTIVE+--escape-file} \
+ ${verbose_log+--only-tap-on-stdout} \
"$GIT_TEST_TEE_OUTPUT_FILE"
>> # lib-gettext: No is_IS UTF-8 locale available
>> # lib-gettext: No is_IS ISO-8859-1 locale available
>> ok 1 - sanity: $GIT_INTERNAL_GETTEXT_SH_SCHEME is set (to fallthrough)
>> ok 2 - sanity: $GIT_INTERNAL_GETTEXT_TEST_FALLBACKS is set
>> ok 3 - sanity: $GIT_INTERNAL_GETTEXT_SH_SCHEME" is fallthrough
>> ok 4 - gettext: our gettext() fallback has pass-through semantics
>> ok 5 - eval_gettext: our eval_gettext() fallback has pass-through semantics
>> ok 6 - eval_gettext: our eval_gettext() fallback can interpolate variables
>> ok 7 - eval_gettext: our eval_gettext() fallback can interpolate variables with spaces
>> ok 8 - eval_gettext: our eval_gettext() fallback can interpolate variables with spaces and quotes
>> # passed all 8 test(s)
>> 1..8
>> ok
>> All tests successful.
>> Files=1, Tests=8, 1 wallclock secs ( 0.01 usr 0.01 sys + 0.07 cusr 0.01 csys = 0.10 CPU)
>> Result: PASS
>> $ wc -l test-results/t0201-gettext-fallbacks.out
>> 75 test-results/t0201-gettext-fallbacks.out
>>
>> All without any patching on top to the test-lib.sh, just:
>>
>> $ cat SZEDERVerboseLog.pm
>> package SZEDERVerboseLog::Session;
>> use base 'TAP::Formatter::Console::Session';
>>
>> sub result {
>> my ($self, $result) = @_;
>> return if $result->is_unknown;
>> return $self->SUPER::result($result);
>> }
>>
>> package SZEDERVerboseLog;
>> use strict;
>> use warnings;
>> use base 'TAP::Formatter::Console';
>>
>> sub open_test {
>> my ($self, $test, $parser) = @_;
>> my $session = SZEDERVerboseLog::Session->new( {
>> name => $test,
>> formatter => $self,
>> parser => $parser,
>> } );
>> return $session;
>> }
>>
>> 1;
>>
>> The "is_unknown" is everything that's not TAP syntax.
>>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 6/7] test-lib: make --verbose output valid TAP
2021-03-10 2:35 ` Ævar Arnfjörð Bjarmason
@ 2021-03-16 9:10 ` Ævar Arnfjörð Bjarmason
0 siblings, 0 replies; 47+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-03-16 9:10 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: git, Junio C Hamano, Jeff King, Johannes Schindelin
On Wed, Mar 10 2021, Ævar Arnfjörð Bjarmason wrote:
> On Tue, Mar 09 2021, SZEDER Gábor wrote:
>
>> On Tue, Mar 09, 2021 at 09:57:03PM +0100, Ævar Arnfjörð Bjarmason wrote:
[...]
>>> Becuse the advantage of this series is that that sort of thing becomes
>>> really trivial without everything needing to be hardcoded in
>>> test-lib.sh, observe (this is with my series):
>>>
>>>
>>> 0 $ PERL5LIB=. prove -v --formatter=SZEDERVerboseLog ./t0201-gettext-fallbacks.sh :: --verbose-log
>>
>> Well, this doesn't look trivial at all, does it? In fact, I consider
>> this unusably convoluted.
>
> ... I meant to say something closer to "does that output look
> ok?". Obviously we'd then make the --verbose-log run something like that
> under the hood.
>
> But in any case, I found it easier to just add this feature to my "tee"
> program than doing it with Perl's TAP libraries, i.e. something like
> this on top (will integrate it in an eventual re-roll):
FWIW I implemented this. I've got it at https://github.com/avar.git's
avar/support-test-verbose-under-prove-2 if you want to take an early
peek at it. It turned into a 50-some patch slog through refactoring
various test-lib.sh code, so I'm seeing about how I submit it.
The end result is that -V now powered by a proper TAP-parsing tap-tee
program. So you'll have e.g. this working as before:
$ ./t9004-example.sh -V
ok 1 - decorate
# passed all 1 test(s)
1..1
But you can also tweak levels of verbosity to stdout (while retaining
full logging) by suppling -V N to get N-level comments, as now indicated
by N number of prefixed "#"'s:
# -V0 is a special-case to get 0th level logging (couldn't think of a
# better name), --verbose-log=0 also works
$ ./t9004-example.sh -V0
ok 1 - decorate
1..1
At level 3 you start getting the source of passing tests (failing tests
are level 1, unexpected TODO tests level 2, trace level 4):
$ ./t9004-example.sh -VVV
ok 1 - decorate
###
### test-tool example-decorate
###
# passed all 1 test(s)
1..1
So obviously the devil's in the details, but it would be useful to know
what you think about the approach before I try to roll this into
submitted patches sooner than later.
I'm mainly interesetd in this for this providing building blocks for
more advanced features down the road.
E.g. it would now be pretty easy to do something like run -x behind the
scenes even if the user doesn't ask for it, and then when a test fail
annotate the specific line that failed using fuzzy-matching on the -x
output.
We could do that now, sort of, but once we've got unambiguous TAP
start/end markers that can't be fooled it becomes trivial to write a
function like get_trace_lines_for_nth_test(N-1) to get your own trace
output for test N.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 6/7] test-lib: make --verbose output valid TAP
2021-03-09 16:02 ` [PATCH 6/7] test-lib: make --verbose output valid TAP Ævar Arnfjörð Bjarmason
2021-03-09 18:59 ` SZEDER Gábor
@ 2021-03-09 19:12 ` SZEDER Gábor
2021-03-10 1:11 ` Junio C Hamano
2021-03-10 7:42 ` Chris Torek
3 siblings, 0 replies; 47+ messages in thread
From: SZEDER Gábor @ 2021-03-09 19:12 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Jeff King, Johannes Schindelin
On Tue, Mar 09, 2021 at 05:02:18PM +0100, Ævar Arnfjörð Bjarmason wrote:
> Make the --verbose output be valid TAP, making it machine-readable for
> TAP parsers again.
>
> Both the verbose and non-verbose test outputs were valid TAP back when
> I added support for TAP in 5099b99d25f (test-lib: Adjust output to be
> valid TAP format, 2010-06-24).
>
> Sometime after that the --verbose output broke due to some tests
> emitting their own lines starting "ok" (or otherwise invalidate the
> TAP). That was noticed and fixed in 452320f1f5 (test-lib: add
> --verbose-log option, 2016-10-21) and "fixed" by simply turning off
> the verbose mode when we were running under TAP::Harness (e.g. under
> "prove").
>
> That solution worked for running under Travis CI. After that fix it
> was made to use the --verbose-log option in 041c72de109 (travis: use
> --verbose-log test option, 2016-10-21), see 522354d70f4 (Add Travis CI
> support, 2015-11-27) for the "cat t/test-results/*.out" code that was
> aimed at.
>
> But that solution and others discussed in 452320f1f5 closed the door
> on us having reliable machine-readable TAP output.
>
> Let's instead revert the work done in 452320f1f5 and, as well as the
> follow-up commits 88c6e9d31c (test-lib: --valgrind should not override
> --verbose-log, 2017-09-05) and f5ba2de6bc (test-lib: make "-x" work
> with "--verbose-log", 2017-12-08), which were only needed to work
> around bugs in the the previous --verbose-log implementation.
>
> Replace it with a simple method for ensuring that we have valid TAP
> both on stdout, and in any verbose output we write. When we detect
> that we're running under "prove" we prefix all legitimate TAP
> directives with "GIT_TEST_TEE_STARTED":
>
> $ GIT_TEST_TEE_STARTED=1 ./t5547-push-quarantine.sh
> GIT_TEST_TEE_STARTED ok 1 - create picky dest repo
> GIT_TEST_TEE_STARTED ok 2 - accepted objects work
> [...]
> GIT_TEST_TEE_STARTED 1..6
>
> Then, instead of piping the output to "tee -a" we pipe it to a helper
> which first converts "ok" and other TAP syntax to e.g. "\ok", and then
> strips that "GIT_TEST_TEE_STARTED " prefix from the start of the line.
>
> The end result is that we're guaranteed to have valid TAP syntax on
> stdout.
>
> We can thus get rid of the --verbose-log special-case. Since that
> option was meant to get around the TAP issue let's simply make it an
> alias for "--verbose --tee".
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> t/README | 9 ++++++---
> t/t0000-basic.sh | 8 ++++++++
> t/test-lib.sh | 43 +++++++++++++++++++++----------------------
> 3 files changed, 35 insertions(+), 25 deletions(-)
>
> diff --git a/t/README b/t/README
> index 2cc8cbc7185..f09d94e754e 100644
> --- a/t/README
> +++ b/t/README
> @@ -157,10 +157,13 @@ appropriately before running "make". Short options can be bundled, i.e.
>
> -V::
> --verbose-log::
> - Write verbose output to the same logfile as `--tee`, but do
> - _not_ write it to stdout. Unlike `--tee --verbose`, this option
> + An alias for `--verbose --tee`. This option
> is safe to use when stdout is being consumed by a TAP parser
> - like `prove`. Implies `--tee` and `--verbose`.
> + like `prove`.
> + Historically this option was different from `--verbose --tee`
> + and would not write any verbose output to stdout to ensure the
> + TAP-correctness of the output. The TAP-correctness of the
> + output is now sanity checked by the test library,
Not everyone is using a TAP harness to run the tests, and, therefore,
'--verbose-log' should not spew out verbose output to the terminal.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 6/7] test-lib: make --verbose output valid TAP
2021-03-09 16:02 ` [PATCH 6/7] test-lib: make --verbose output valid TAP Ævar Arnfjörð Bjarmason
2021-03-09 18:59 ` SZEDER Gábor
2021-03-09 19:12 ` SZEDER Gábor
@ 2021-03-10 1:11 ` Junio C Hamano
2021-03-10 7:42 ` Chris Torek
3 siblings, 0 replies; 47+ messages in thread
From: Junio C Hamano @ 2021-03-10 1:11 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jeff King, SZEDER Gábor, Johannes Schindelin
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> Replace it with a simple method for ensuring that we have valid TAP
> both on stdout, and in any verbose output we write. When we detect
> that we're running under "prove" we prefix all legitimate TAP
> directives with "GIT_TEST_TEE_STARTED":
>
> $ GIT_TEST_TEE_STARTED=1 ./t5547-push-quarantine.sh
> GIT_TEST_TEE_STARTED ok 1 - create picky dest repo
> GIT_TEST_TEE_STARTED ok 2 - accepted objects work
> [...]
> GIT_TEST_TEE_STARTED 1..6
Ah, OK, so these perl scripts that use Test::More think they are
emitting TAP, but because the prefix is not added to any of their
output, they are not considered to be a tap output at all.
So did the "external" tests lose a lot of value with this change?
They used to point out individual breakage, but now it is just
"passed as a whole (or not)" boolean?
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 6/7] test-lib: make --verbose output valid TAP
2021-03-09 16:02 ` [PATCH 6/7] test-lib: make --verbose output valid TAP Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2021-03-10 1:11 ` Junio C Hamano
@ 2021-03-10 7:42 ` Chris Torek
3 siblings, 0 replies; 47+ messages in thread
From: Chris Torek @ 2021-03-10 7:42 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git List, Junio C Hamano, Jeff King, SZEDER Gábor,
Johannes Schindelin
There is a lot of historical stuff I can't really comment on
here, so I'll just point out this one tiny bit of shell oddity:
On Tue, Mar 9, 2021 at 8:04 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> Make the --verbose output be valid TAP, making it machine-readable for
> TAP parsers again.
[snippage]
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index aa7068b06b6..0070d05234b 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
[snippage]
> @@ -364,9 +366,22 @@ then
> (
> GIT_TEST_TEE_STARTED=done ${TEST_SHELL_PATH} "$0" "$@" 2>&1
> echo $? >"$TEST_RESULTS_BASE.exit"
> - ) | tee -a "$GIT_TEST_TEE_OUTPUT_FILE"
> + ) | "$GIT_BUILD_DIR"/t/helper/test-tool tee \
> + --tap --prefix="GIT_TEST_TEE_STARTED " \
> + --escape-stdout ${HARNESS_ACTIVE+--escape-file} \
> + "$GIT_TEST_TEE_OUTPUT_FILE"
> test "$(cat "$TEST_RESULTS_BASE.exit")" = 0
> exit
> +elif test -n "$verbose" -a -n "$HARNESS_ACTIVE"
> +then
> + ret=
> + (
> + GIT_TEST_TEE_STARTED=done ${TEST_SHELL_PATH} "$0" "$@" 2>&1
> + ret=$?
> + ) | "$GIT_BUILD_DIR"/t/helper/test-tool tee \
> + --tap --prefix="GIT_TEST_TEE_STARTED " \
> + --escape-stdout
> + exit $ret
> fi
>
> if test -n "$trace" && test -n "$test_untraceable"
[snippage]
In the block beginning with:
ret=
we have a subshell:
( ... ) | "$GIT_BUILD_DIR"/t/helper/test-tool tee ...
In the subshell itself, we set `ret=$?`. But this is inside a
subshell, which then exits, so the setting of `ret` will get lost.
Did you perhaps want `{ ...; }` instead here? Unfortunately the
pipe means that the whole left side may run in a subshell
anyway, so even that doesn't fix the problem. We need a
temp file, a la the code above that dumps $? into
"$TEST_RESULTS_BASE.exit".
Chris
^ permalink raw reply [flat|nested] 47+ messages in thread
* [RFC/PATCH 7/7] test-lib: generate JUnit output via TAP
2021-03-01 13:54 ` Ævar Arnfjörð Bjarmason
` (6 preceding siblings ...)
2021-03-09 16:02 ` [PATCH 6/7] test-lib: make --verbose output valid TAP Ævar Arnfjörð Bjarmason
@ 2021-03-09 16:02 ` Ævar Arnfjörð Bjarmason
2021-03-19 14:14 ` Johannes Schindelin
7 siblings, 1 reply; 47+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-03-09 16:02 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, SZEDER Gábor, Johannes Schindelin,
Ævar Arnfjörð Bjarmason
Rewrite the home-brew JUnit output generation added in
22231908151 (tests: optionally write results as JUnit-style .xml,
2019-01-29) and other subsequent commits to be generated via the TAP
output.
This is now possible that the TAP output emitted with --tee is
guaranteed to be valid TAP, see the preceding commit.
The JUnit output is not bug-for-bug compatible with the previous
output, but it looks better to me.
This also requires installing TAP::Formatter::JUnit[1], perhaps that's
not palatable to the users of the --write-junit-xml option.
In any case, it'll be easy to whip up our own TAP emitter with a
TAP::Parser and TAP::Formatter, both of whom come with perl itself,
which we already rely on for tests.
It should also be significantly faster on Windows, as we can
e.g. write all the *.out files, and only do that conversion at the end
for all files in a batch, as opposed to the current implementation of
shelling out to test-tool in a loop for each test.
1. https://metacpan.org/pod/distribution/TAP-Formatter-JUnit/script/tap2junit
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
Makefile | 1 -
t/helper/test-date.c | 12 ----
t/helper/test-path-utils.c | 21 -------
t/helper/test-tool.c | 1 -
t/helper/test-tool.h | 1 -
t/helper/test-xml-encode.c | 80 ------------------------
t/test-lib.sh | 123 ++++---------------------------------
7 files changed, 12 insertions(+), 227 deletions(-)
delete mode 100644 t/helper/test-xml-encode.c
diff --git a/Makefile b/Makefile
index d26b9d62ee9..fa7c52f7a42 100644
--- a/Makefile
+++ b/Makefile
@@ -748,7 +748,6 @@ TEST_BUILTINS_OBJS += test-urlmatch-normalization.o
TEST_BUILTINS_OBJS += test-wildmatch.o
TEST_BUILTINS_OBJS += test-windows-named-pipe.o
TEST_BUILTINS_OBJS += test-write-cache.o
-TEST_BUILTINS_OBJS += test-xml-encode.o
# Do not add more tests here unless they have extra dependencies. Add
# them in TEST_BUILTINS_OBJS above.
diff --git a/t/helper/test-date.c b/t/helper/test-date.c
index 099eff4f0fc..ebf133943b3 100644
--- a/t/helper/test-date.c
+++ b/t/helper/test-date.c
@@ -8,7 +8,6 @@ static const char *usage_msg = "\n"
" test-tool date parse [date]...\n"
" test-tool date approxidate [date]...\n"
" test-tool date timestamp [date]...\n"
-" test-tool date getnanos [start-nanos]\n"
" test-tool date is64bit\n"
" test-tool date time_t-is64bit\n";
@@ -92,15 +91,6 @@ static void parse_approx_timestamp(const char **argv)
}
}
-static void getnanos(const char **argv)
-{
- double seconds = getnanotime() / 1.0e9;
-
- if (*argv)
- seconds -= strtod(*argv, NULL);
- printf("%lf\n", seconds);
-}
-
int cmd__date(int argc, const char **argv)
{
const char *x;
@@ -120,8 +110,6 @@ int cmd__date(int argc, const char **argv)
parse_approxidate(argv+1);
else if (!strcmp(*argv, "timestamp"))
parse_approx_timestamp(argv+1);
- else if (!strcmp(*argv, "getnanos"))
- getnanos(argv+1);
else if (!strcmp(*argv, "is64bit"))
return sizeof(timestamp_t) == 8 ? 0 : 1;
else if (!strcmp(*argv, "time_t-is64bit"))
diff --git a/t/helper/test-path-utils.c b/t/helper/test-path-utils.c
index 313a153209c..090596ac492 100644
--- a/t/helper/test-path-utils.c
+++ b/t/helper/test-path-utils.c
@@ -407,27 +407,6 @@ int cmd__path_utils(int argc, const char **argv)
return !!res;
}
- if (argc == 4 && !strcmp(argv[1], "skip-n-bytes")) {
- int fd = open(argv[2], O_RDONLY), offset = atoi(argv[3]);
- char buffer[65536];
-
- if (fd < 0)
- die_errno("could not open '%s'", argv[2]);
- if (lseek(fd, offset, SEEK_SET) < 0)
- die_errno("could not skip %d bytes", offset);
- for (;;) {
- ssize_t count = read(fd, buffer, sizeof(buffer));
- if (count < 0)
- die_errno("could not read '%s'", argv[2]);
- if (!count)
- break;
- if (write(1, buffer, count) < 0)
- die_errno("could not write to stdout");
- }
- close(fd);
- return 0;
- }
-
if (argc > 5 && !strcmp(argv[1], "slice-tests")) {
int res = 0;
long offset, stride, i;
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 1876bad8f42..3fa02011b61 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -73,7 +73,6 @@ static struct test_cmd cmds[] = {
{ "tee", cmd__tee },
{ "trace2", cmd__trace2 },
{ "urlmatch-normalization", cmd__urlmatch_normalization },
- { "xml-encode", cmd__xml_encode },
{ "wildmatch", cmd__wildmatch },
#ifdef GIT_WINDOWS_NATIVE
{ "windows-named-pipe", cmd__windows_named_pipe },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 9b3c1f75267..5028730807e 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -63,7 +63,6 @@ int cmd__subprocess(int argc, const char **argv);
int cmd__tee(int argc, const char **argv);
int cmd__trace2(int argc, const char **argv);
int cmd__urlmatch_normalization(int argc, const char **argv);
-int cmd__xml_encode(int argc, const char **argv);
int cmd__wildmatch(int argc, const char **argv);
#ifdef GIT_WINDOWS_NATIVE
int cmd__windows_named_pipe(int argc, const char **argv);
diff --git a/t/helper/test-xml-encode.c b/t/helper/test-xml-encode.c
deleted file mode 100644
index a648bbd961c..00000000000
--- a/t/helper/test-xml-encode.c
+++ /dev/null
@@ -1,80 +0,0 @@
-#include "test-tool.h"
-
-static const char *utf8_replace_character = "�";
-
-/*
- * Encodes (possibly incorrect) UTF-8 on <stdin> to <stdout>, to be embedded
- * in an XML file.
- */
-int cmd__xml_encode(int argc, const char **argv)
-{
- unsigned char buf[1024], tmp[4], *tmp2 = NULL;
- ssize_t cur = 0, len = 1, remaining = 0;
- unsigned char ch;
-
- for (;;) {
- if (++cur == len) {
- len = xread(0, buf, sizeof(buf));
- if (!len)
- return 0;
- if (len < 0)
- die_errno("Could not read <stdin>");
- cur = 0;
- }
- ch = buf[cur];
-
- if (tmp2) {
- if ((ch & 0xc0) != 0x80) {
- fputs(utf8_replace_character, stdout);
- tmp2 = NULL;
- cur--;
- continue;
- }
- *tmp2 = ch;
- tmp2++;
- if (--remaining == 0) {
- fwrite(tmp, tmp2 - tmp, 1, stdout);
- tmp2 = NULL;
- }
- continue;
- }
-
- if (!(ch & 0x80)) {
- /* 0xxxxxxx */
- if (ch == '&')
- fputs("&", stdout);
- else if (ch == '\'')
- fputs("'", stdout);
- else if (ch == '"')
- fputs(""", stdout);
- else if (ch == '<')
- fputs("<", stdout);
- else if (ch == '>')
- fputs(">", stdout);
- else if (ch >= 0x20)
- fputc(ch, stdout);
- else if (ch == 0x09 || ch == 0x0a || ch == 0x0d)
- fprintf(stdout, "&#x%02x;", ch);
- else
- fputs(utf8_replace_character, stdout);
- } else if ((ch & 0xe0) == 0xc0) {
- /* 110XXXXx 10xxxxxx */
- tmp[0] = ch;
- remaining = 1;
- tmp2 = tmp + 1;
- } else if ((ch & 0xf0) == 0xe0) {
- /* 1110XXXX 10Xxxxxx 10xxxxxx */
- tmp[0] = ch;
- remaining = 2;
- tmp2 = tmp + 1;
- } else if ((ch & 0xf8) == 0xf0) {
- /* 11110XXX 10XXxxxx 10xxxxxx 10xxxxxx */
- tmp[0] = ch;
- remaining = 3;
- tmp2 = tmp + 1;
- } else
- fputs(utf8_replace_character, stdout);
- }
-
- return 0;
-}
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 0070d05234b..4dc41eeccc2 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -371,6 +371,17 @@ then
--escape-stdout ${HARNESS_ACTIVE+--escape-file} \
"$GIT_TEST_TEE_OUTPUT_FILE"
test "$(cat "$TEST_RESULTS_BASE.exit")" = 0
+ if test -n "$write_junit_xml"
+ then
+ junit_xml_dir="$TEST_OUTPUT_DIRECTORY/out"
+ mkdir -p "$junit_xml_dir"
+ junit_xml_base=${0##*/}
+ junit_xml_path="$junit_xml_dir/TEST-${junit_xml_base%.sh}.xml"
+ junit_attrs="timestamp=\"$(TZ=UTC date +%Y-%m-%dT%H:%M:%S)\""
+ tap2junit --name="$TEST_NAME" - \
+ <"$GIT_TEST_TEE_OUTPUT_FILE" \
+ >"$junit_xml_path"
+ fi
exit
elif test -n "$verbose" -a -n "$HARNESS_ACTIVE"
then
@@ -588,7 +599,6 @@ export TERM
error () {
say_color error "error: $*"
- finalize_junit_xml
GIT_EXIT_OK=t
exit 1
}
@@ -672,56 +682,24 @@ trap '{ code=$?; set +x; } 2>/dev/null; exit $code' INT TERM HUP
# the test_expect_* functions instead.
test_ok_ () {
- if test -n "$write_junit_xml"
- then
- write_junit_xml_testcase "$*"
- fi
test_success=$(($test_success + 1))
say_color_tap "${verbose:+pass}" "ok $test_count - $@"
}
test_failure_ () {
- if test -n "$write_junit_xml"
- then
- junit_insert="<failure message=\"not ok $test_count -"
- junit_insert="$junit_insert $(xml_attr_encode "$1")\">"
- junit_insert="$junit_insert $(xml_attr_encode \
- "$(if test -n "$GIT_TEST_TEE_OUTPUT_FILE"
- then
- test-tool path-utils skip-n-bytes \
- "$GIT_TEST_TEE_OUTPUT_FILE" $GIT_TEST_TEE_OFFSET
- else
- printf '%s\n' "$@" | sed 1d
- fi)")"
- junit_insert="$junit_insert</failure>"
- if test -n "$GIT_TEST_TEE_OUTPUT_FILE"
- then
- junit_insert="$junit_insert<system-err>$(xml_attr_encode \
- "$(cat "$GIT_TEST_TEE_OUTPUT_FILE")")</system-err>"
- fi
- write_junit_xml_testcase "$1" " $junit_insert"
- fi
test_failure=$(($test_failure + 1))
say_color_tap error "not ok $test_count - $1"
shift
printf '%s\n' "$*" | sed -e 's/^/# /'
- test "$immediate" = "" || { finalize_junit_xml; GIT_EXIT_OK=t; exit 1; }
+ test "$immediate" = "" || { GIT_EXIT_OK=t; exit 1; }
}
test_known_broken_ok_ () {
- if test -n "$write_junit_xml"
- then
- write_junit_xml_testcase "$* (breakage fixed)"
- fi
test_fixed=$(($test_fixed+1))
say_color error "ok $test_count - $@ # TODO known breakage vanished"
}
test_known_broken_failure_ () {
- if test -n "$write_junit_xml"
- then
- write_junit_xml_testcase "$* (known breakage)"
- fi
test_broken=$(($test_broken+1))
say_color_tap warn "not ok $test_count - $@ # TODO known breakage"
}
@@ -983,10 +961,6 @@ test_start_ () {
test_count=$(($test_count+1))
maybe_setup_verbose
maybe_setup_valgrind
- if test -n "$write_junit_xml"
- then
- junit_start=$(test-tool date getnanos)
- fi
}
test_finish_ () {
@@ -1029,13 +1003,6 @@ test_skip () {
case "$to_skip" in
t)
- if test -n "$write_junit_xml"
- then
- message="$(xml_attr_encode "$skipped_reason")"
- write_junit_xml_testcase "$1" \
- " <skipped message=\"$message\" />"
- fi
-
say_color_tap skip "ok $test_count # skip $1 ($skipped_reason)"
: true
;;
@@ -1050,53 +1017,6 @@ test_at_end_hook_ () {
:
}
-write_junit_xml () {
- case "$1" in
- --truncate)
- >"$junit_xml_path"
- junit_have_testcase=
- shift
- ;;
- esac
- printf '%s\n' "$@" >>"$junit_xml_path"
-}
-
-xml_attr_encode () {
- printf '%s\n' "$@" | test-tool xml-encode
-}
-
-write_junit_xml_testcase () {
- junit_attrs="name=\"$(xml_attr_encode "$this_test.$test_count $1")\""
- shift
- junit_attrs="$junit_attrs classname=\"$this_test\""
- junit_attrs="$junit_attrs time=\"$(test-tool \
- date getnanos $junit_start)\""
- write_junit_xml "$(printf '%s\n' \
- " <testcase $junit_attrs>" "$@" " </testcase>")"
- junit_have_testcase=t
-}
-
-finalize_junit_xml () {
- if test -n "$write_junit_xml" && test -n "$junit_xml_path"
- then
- test -n "$junit_have_testcase" || {
- junit_start=$(test-tool date getnanos)
- write_junit_xml_testcase "all tests skipped"
- }
-
- # adjust the overall time
- junit_time=$(test-tool date getnanos $junit_suite_start)
- sed -e "s/\(<testsuite.*\) time=\"[^\"]*\"/\1/" \
- -e "s/<testsuite [^>]*/& time=\"$junit_time\"/" \
- -e '/^ *<\/testsuite/d' \
- <"$junit_xml_path" >"$junit_xml_path.new"
- mv "$junit_xml_path.new" "$junit_xml_path"
-
- write_junit_xml " </testsuite>" "</testsuites>"
- write_junit_xml=
- fi
-}
-
test_atexit_cleanup=:
test_atexit_handler () {
# In a succeeding test script 'test_atexit_handler' is invoked
@@ -1119,8 +1039,6 @@ test_done () {
# removed, so the commands can access pidfiles and socket files.
test_atexit_handler
- finalize_junit_xml
-
if test -z "$HARNESS_ACTIVE"
then
mkdir -p "$TEST_RESULTS_DIR"
@@ -1364,23 +1282,6 @@ then
test_done
fi
-if test -n "$write_junit_xml"
-then
- junit_xml_dir="$TEST_OUTPUT_DIRECTORY/out"
- mkdir -p "$junit_xml_dir"
- junit_xml_base=${0##*/}
- junit_xml_path="$junit_xml_dir/TEST-${junit_xml_base%.sh}.xml"
- junit_attrs="name=\"${junit_xml_base%.sh}\""
- junit_attrs="$junit_attrs timestamp=\"$(TZ=UTC \
- date +%Y-%m-%dT%H:%M:%S)\""
- write_junit_xml --truncate "<testsuites>" " <testsuite $junit_attrs>"
- junit_suite_start=$(test-tool date getnanos)
- if test -n "$GIT_TEST_TEE_OUTPUT_FILE"
- then
- GIT_TEST_TEE_OFFSET=0
- fi
-fi
-
# Convenience
# A regexp to match 5, 35 and 40 hexdigits
_x05='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'
--
2.31.0.rc1.210.g0f8085a843c
^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: [RFC/PATCH 7/7] test-lib: generate JUnit output via TAP
2021-03-09 16:02 ` [RFC/PATCH 7/7] test-lib: generate JUnit output via TAP Ævar Arnfjörð Bjarmason
@ 2021-03-19 14:14 ` Johannes Schindelin
2021-03-21 0:28 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 47+ messages in thread
From: Johannes Schindelin @ 2021-03-19 14:14 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Jeff King, SZEDER Gábor
[-- Attachment #1: Type: text/plain, Size: 15187 bytes --]
Hi Ævar,
On Tue, 9 Mar 2021, Ævar Arnfjörð Bjarmason wrote:
> Rewrite the home-brew JUnit output generation added in
> 22231908151 (tests: optionally write results as JUnit-style .xml,
> 2019-01-29) and other subsequent commits to be generated via the TAP
> output.
>
> This is now possible that the TAP output emitted with --tee is
> guaranteed to be valid TAP, see the preceding commit.
>
> The JUnit output is not bug-for-bug compatible with the previous
> output, but it looks better to me.
>
> This also requires installing TAP::Formatter::JUnit[1], perhaps that's
> not palatable to the users of the --write-junit-xml option.
Indeed. I am trying to keep the dependencies required for the Windows jobs
of our CI/PR builds to a minimum.
Note, also, that the JUnit output was mostly relevant for when we used
Azure Pipelines: it has a specific UI to study test results, figure out
flaky tests, performance, etc.
Now that we use GitHub Actions, we do not have such a nice test aggregator
anymore, but we might get one again in the future, who knows.
> In any case, it'll be easy to whip up our own TAP emitter with a
> TAP::Parser and TAP::Formatter, both of whom come with perl itself,
> which we already rely on for tests.
>
> It should also be significantly faster on Windows,
I really hate to have to harp on this when talking to you, but... well,
how can I say it? Perl is _slooooooooooooow_ on Windows.
Like, _really_ slow. Ridiculously slow.
I know, you recently got riled up when Jeff suggested that the Perl hook
calling FSMonior might be slow, but the truth is: it is super slow. It's
not even funny how slow it is. And it fills me with no joy having to
repeat it every time the question comes up whether running any part of Git
that is written in Perl might affect performance on Windows. I really
dislike having to be that messenger.
I doubt that you will ever be able to replace my (admittedly slightly
hacky) C helper with anything written in Perl that does even come close to
being faster.
In other words, I fear that your work here might not have the outcome you
hoped for.
Ciao,
Johannes
> as we can e.g. write all the *.out files, and only do that conversion at
> the end for all files in a batch, as opposed to the current
> implementation of shelling out to test-tool in a loop for each test.
>
> 1. https://metacpan.org/pod/distribution/TAP-Formatter-JUnit/script/tap2junit
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> Makefile | 1 -
> t/helper/test-date.c | 12 ----
> t/helper/test-path-utils.c | 21 -------
> t/helper/test-tool.c | 1 -
> t/helper/test-tool.h | 1 -
> t/helper/test-xml-encode.c | 80 ------------------------
> t/test-lib.sh | 123 ++++---------------------------------
> 7 files changed, 12 insertions(+), 227 deletions(-)
> delete mode 100644 t/helper/test-xml-encode.c
>
> diff --git a/Makefile b/Makefile
> index d26b9d62ee9..fa7c52f7a42 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -748,7 +748,6 @@ TEST_BUILTINS_OBJS += test-urlmatch-normalization.o
> TEST_BUILTINS_OBJS += test-wildmatch.o
> TEST_BUILTINS_OBJS += test-windows-named-pipe.o
> TEST_BUILTINS_OBJS += test-write-cache.o
> -TEST_BUILTINS_OBJS += test-xml-encode.o
>
> # Do not add more tests here unless they have extra dependencies. Add
> # them in TEST_BUILTINS_OBJS above.
> diff --git a/t/helper/test-date.c b/t/helper/test-date.c
> index 099eff4f0fc..ebf133943b3 100644
> --- a/t/helper/test-date.c
> +++ b/t/helper/test-date.c
> @@ -8,7 +8,6 @@ static const char *usage_msg = "\n"
> " test-tool date parse [date]...\n"
> " test-tool date approxidate [date]...\n"
> " test-tool date timestamp [date]...\n"
> -" test-tool date getnanos [start-nanos]\n"
> " test-tool date is64bit\n"
> " test-tool date time_t-is64bit\n";
>
> @@ -92,15 +91,6 @@ static void parse_approx_timestamp(const char **argv)
> }
> }
>
> -static void getnanos(const char **argv)
> -{
> - double seconds = getnanotime() / 1.0e9;
> -
> - if (*argv)
> - seconds -= strtod(*argv, NULL);
> - printf("%lf\n", seconds);
> -}
> -
> int cmd__date(int argc, const char **argv)
> {
> const char *x;
> @@ -120,8 +110,6 @@ int cmd__date(int argc, const char **argv)
> parse_approxidate(argv+1);
> else if (!strcmp(*argv, "timestamp"))
> parse_approx_timestamp(argv+1);
> - else if (!strcmp(*argv, "getnanos"))
> - getnanos(argv+1);
> else if (!strcmp(*argv, "is64bit"))
> return sizeof(timestamp_t) == 8 ? 0 : 1;
> else if (!strcmp(*argv, "time_t-is64bit"))
> diff --git a/t/helper/test-path-utils.c b/t/helper/test-path-utils.c
> index 313a153209c..090596ac492 100644
> --- a/t/helper/test-path-utils.c
> +++ b/t/helper/test-path-utils.c
> @@ -407,27 +407,6 @@ int cmd__path_utils(int argc, const char **argv)
> return !!res;
> }
>
> - if (argc == 4 && !strcmp(argv[1], "skip-n-bytes")) {
> - int fd = open(argv[2], O_RDONLY), offset = atoi(argv[3]);
> - char buffer[65536];
> -
> - if (fd < 0)
> - die_errno("could not open '%s'", argv[2]);
> - if (lseek(fd, offset, SEEK_SET) < 0)
> - die_errno("could not skip %d bytes", offset);
> - for (;;) {
> - ssize_t count = read(fd, buffer, sizeof(buffer));
> - if (count < 0)
> - die_errno("could not read '%s'", argv[2]);
> - if (!count)
> - break;
> - if (write(1, buffer, count) < 0)
> - die_errno("could not write to stdout");
> - }
> - close(fd);
> - return 0;
> - }
> -
> if (argc > 5 && !strcmp(argv[1], "slice-tests")) {
> int res = 0;
> long offset, stride, i;
> diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
> index 1876bad8f42..3fa02011b61 100644
> --- a/t/helper/test-tool.c
> +++ b/t/helper/test-tool.c
> @@ -73,7 +73,6 @@ static struct test_cmd cmds[] = {
> { "tee", cmd__tee },
> { "trace2", cmd__trace2 },
> { "urlmatch-normalization", cmd__urlmatch_normalization },
> - { "xml-encode", cmd__xml_encode },
> { "wildmatch", cmd__wildmatch },
> #ifdef GIT_WINDOWS_NATIVE
> { "windows-named-pipe", cmd__windows_named_pipe },
> diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
> index 9b3c1f75267..5028730807e 100644
> --- a/t/helper/test-tool.h
> +++ b/t/helper/test-tool.h
> @@ -63,7 +63,6 @@ int cmd__subprocess(int argc, const char **argv);
> int cmd__tee(int argc, const char **argv);
> int cmd__trace2(int argc, const char **argv);
> int cmd__urlmatch_normalization(int argc, const char **argv);
> -int cmd__xml_encode(int argc, const char **argv);
> int cmd__wildmatch(int argc, const char **argv);
> #ifdef GIT_WINDOWS_NATIVE
> int cmd__windows_named_pipe(int argc, const char **argv);
> diff --git a/t/helper/test-xml-encode.c b/t/helper/test-xml-encode.c
> deleted file mode 100644
> index a648bbd961c..00000000000
> --- a/t/helper/test-xml-encode.c
> +++ /dev/null
> @@ -1,80 +0,0 @@
> -#include "test-tool.h"
> -
> -static const char *utf8_replace_character = "�";
> -
> -/*
> - * Encodes (possibly incorrect) UTF-8 on <stdin> to <stdout>, to be embedded
> - * in an XML file.
> - */
> -int cmd__xml_encode(int argc, const char **argv)
> -{
> - unsigned char buf[1024], tmp[4], *tmp2 = NULL;
> - ssize_t cur = 0, len = 1, remaining = 0;
> - unsigned char ch;
> -
> - for (;;) {
> - if (++cur == len) {
> - len = xread(0, buf, sizeof(buf));
> - if (!len)
> - return 0;
> - if (len < 0)
> - die_errno("Could not read <stdin>");
> - cur = 0;
> - }
> - ch = buf[cur];
> -
> - if (tmp2) {
> - if ((ch & 0xc0) != 0x80) {
> - fputs(utf8_replace_character, stdout);
> - tmp2 = NULL;
> - cur--;
> - continue;
> - }
> - *tmp2 = ch;
> - tmp2++;
> - if (--remaining == 0) {
> - fwrite(tmp, tmp2 - tmp, 1, stdout);
> - tmp2 = NULL;
> - }
> - continue;
> - }
> -
> - if (!(ch & 0x80)) {
> - /* 0xxxxxxx */
> - if (ch == '&')
> - fputs("&", stdout);
> - else if (ch == '\'')
> - fputs("'", stdout);
> - else if (ch == '"')
> - fputs(""", stdout);
> - else if (ch == '<')
> - fputs("<", stdout);
> - else if (ch == '>')
> - fputs(">", stdout);
> - else if (ch >= 0x20)
> - fputc(ch, stdout);
> - else if (ch == 0x09 || ch == 0x0a || ch == 0x0d)
> - fprintf(stdout, "&#x%02x;", ch);
> - else
> - fputs(utf8_replace_character, stdout);
> - } else if ((ch & 0xe0) == 0xc0) {
> - /* 110XXXXx 10xxxxxx */
> - tmp[0] = ch;
> - remaining = 1;
> - tmp2 = tmp + 1;
> - } else if ((ch & 0xf0) == 0xe0) {
> - /* 1110XXXX 10Xxxxxx 10xxxxxx */
> - tmp[0] = ch;
> - remaining = 2;
> - tmp2 = tmp + 1;
> - } else if ((ch & 0xf8) == 0xf0) {
> - /* 11110XXX 10XXxxxx 10xxxxxx 10xxxxxx */
> - tmp[0] = ch;
> - remaining = 3;
> - tmp2 = tmp + 1;
> - } else
> - fputs(utf8_replace_character, stdout);
> - }
> -
> - return 0;
> -}
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 0070d05234b..4dc41eeccc2 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -371,6 +371,17 @@ then
> --escape-stdout ${HARNESS_ACTIVE+--escape-file} \
> "$GIT_TEST_TEE_OUTPUT_FILE"
> test "$(cat "$TEST_RESULTS_BASE.exit")" = 0
> + if test -n "$write_junit_xml"
> + then
> + junit_xml_dir="$TEST_OUTPUT_DIRECTORY/out"
> + mkdir -p "$junit_xml_dir"
> + junit_xml_base=${0##*/}
> + junit_xml_path="$junit_xml_dir/TEST-${junit_xml_base%.sh}.xml"
> + junit_attrs="timestamp=\"$(TZ=UTC date +%Y-%m-%dT%H:%M:%S)\""
> + tap2junit --name="$TEST_NAME" - \
> + <"$GIT_TEST_TEE_OUTPUT_FILE" \
> + >"$junit_xml_path"
> + fi
> exit
> elif test -n "$verbose" -a -n "$HARNESS_ACTIVE"
> then
> @@ -588,7 +599,6 @@ export TERM
>
> error () {
> say_color error "error: $*"
> - finalize_junit_xml
> GIT_EXIT_OK=t
> exit 1
> }
> @@ -672,56 +682,24 @@ trap '{ code=$?; set +x; } 2>/dev/null; exit $code' INT TERM HUP
> # the test_expect_* functions instead.
>
> test_ok_ () {
> - if test -n "$write_junit_xml"
> - then
> - write_junit_xml_testcase "$*"
> - fi
> test_success=$(($test_success + 1))
> say_color_tap "${verbose:+pass}" "ok $test_count - $@"
> }
>
> test_failure_ () {
> - if test -n "$write_junit_xml"
> - then
> - junit_insert="<failure message=\"not ok $test_count -"
> - junit_insert="$junit_insert $(xml_attr_encode "$1")\">"
> - junit_insert="$junit_insert $(xml_attr_encode \
> - "$(if test -n "$GIT_TEST_TEE_OUTPUT_FILE"
> - then
> - test-tool path-utils skip-n-bytes \
> - "$GIT_TEST_TEE_OUTPUT_FILE" $GIT_TEST_TEE_OFFSET
> - else
> - printf '%s\n' "$@" | sed 1d
> - fi)")"
> - junit_insert="$junit_insert</failure>"
> - if test -n "$GIT_TEST_TEE_OUTPUT_FILE"
> - then
> - junit_insert="$junit_insert<system-err>$(xml_attr_encode \
> - "$(cat "$GIT_TEST_TEE_OUTPUT_FILE")")</system-err>"
> - fi
> - write_junit_xml_testcase "$1" " $junit_insert"
> - fi
> test_failure=$(($test_failure + 1))
> say_color_tap error "not ok $test_count - $1"
> shift
> printf '%s\n' "$*" | sed -e 's/^/# /'
> - test "$immediate" = "" || { finalize_junit_xml; GIT_EXIT_OK=t; exit 1; }
> + test "$immediate" = "" || { GIT_EXIT_OK=t; exit 1; }
> }
>
> test_known_broken_ok_ () {
> - if test -n "$write_junit_xml"
> - then
> - write_junit_xml_testcase "$* (breakage fixed)"
> - fi
> test_fixed=$(($test_fixed+1))
> say_color error "ok $test_count - $@ # TODO known breakage vanished"
> }
>
> test_known_broken_failure_ () {
> - if test -n "$write_junit_xml"
> - then
> - write_junit_xml_testcase "$* (known breakage)"
> - fi
> test_broken=$(($test_broken+1))
> say_color_tap warn "not ok $test_count - $@ # TODO known breakage"
> }
> @@ -983,10 +961,6 @@ test_start_ () {
> test_count=$(($test_count+1))
> maybe_setup_verbose
> maybe_setup_valgrind
> - if test -n "$write_junit_xml"
> - then
> - junit_start=$(test-tool date getnanos)
> - fi
> }
>
> test_finish_ () {
> @@ -1029,13 +1003,6 @@ test_skip () {
>
> case "$to_skip" in
> t)
> - if test -n "$write_junit_xml"
> - then
> - message="$(xml_attr_encode "$skipped_reason")"
> - write_junit_xml_testcase "$1" \
> - " <skipped message=\"$message\" />"
> - fi
> -
> say_color_tap skip "ok $test_count # skip $1 ($skipped_reason)"
> : true
> ;;
> @@ -1050,53 +1017,6 @@ test_at_end_hook_ () {
> :
> }
>
> -write_junit_xml () {
> - case "$1" in
> - --truncate)
> - >"$junit_xml_path"
> - junit_have_testcase=
> - shift
> - ;;
> - esac
> - printf '%s\n' "$@" >>"$junit_xml_path"
> -}
> -
> -xml_attr_encode () {
> - printf '%s\n' "$@" | test-tool xml-encode
> -}
> -
> -write_junit_xml_testcase () {
> - junit_attrs="name=\"$(xml_attr_encode "$this_test.$test_count $1")\""
> - shift
> - junit_attrs="$junit_attrs classname=\"$this_test\""
> - junit_attrs="$junit_attrs time=\"$(test-tool \
> - date getnanos $junit_start)\""
> - write_junit_xml "$(printf '%s\n' \
> - " <testcase $junit_attrs>" "$@" " </testcase>")"
> - junit_have_testcase=t
> -}
> -
> -finalize_junit_xml () {
> - if test -n "$write_junit_xml" && test -n "$junit_xml_path"
> - then
> - test -n "$junit_have_testcase" || {
> - junit_start=$(test-tool date getnanos)
> - write_junit_xml_testcase "all tests skipped"
> - }
> -
> - # adjust the overall time
> - junit_time=$(test-tool date getnanos $junit_suite_start)
> - sed -e "s/\(<testsuite.*\) time=\"[^\"]*\"/\1/" \
> - -e "s/<testsuite [^>]*/& time=\"$junit_time\"/" \
> - -e '/^ *<\/testsuite/d' \
> - <"$junit_xml_path" >"$junit_xml_path.new"
> - mv "$junit_xml_path.new" "$junit_xml_path"
> -
> - write_junit_xml " </testsuite>" "</testsuites>"
> - write_junit_xml=
> - fi
> -}
> -
> test_atexit_cleanup=:
> test_atexit_handler () {
> # In a succeeding test script 'test_atexit_handler' is invoked
> @@ -1119,8 +1039,6 @@ test_done () {
> # removed, so the commands can access pidfiles and socket files.
> test_atexit_handler
>
> - finalize_junit_xml
> -
> if test -z "$HARNESS_ACTIVE"
> then
> mkdir -p "$TEST_RESULTS_DIR"
> @@ -1364,23 +1282,6 @@ then
> test_done
> fi
>
> -if test -n "$write_junit_xml"
> -then
> - junit_xml_dir="$TEST_OUTPUT_DIRECTORY/out"
> - mkdir -p "$junit_xml_dir"
> - junit_xml_base=${0##*/}
> - junit_xml_path="$junit_xml_dir/TEST-${junit_xml_base%.sh}.xml"
> - junit_attrs="name=\"${junit_xml_base%.sh}\""
> - junit_attrs="$junit_attrs timestamp=\"$(TZ=UTC \
> - date +%Y-%m-%dT%H:%M:%S)\""
> - write_junit_xml --truncate "<testsuites>" " <testsuite $junit_attrs>"
> - junit_suite_start=$(test-tool date getnanos)
> - if test -n "$GIT_TEST_TEE_OUTPUT_FILE"
> - then
> - GIT_TEST_TEE_OFFSET=0
> - fi
> -fi
> -
> # Convenience
> # A regexp to match 5, 35 and 40 hexdigits
> _x05='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'
> --
> 2.31.0.rc1.210.g0f8085a843c
>
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC/PATCH 7/7] test-lib: generate JUnit output via TAP
2021-03-19 14:14 ` Johannes Schindelin
@ 2021-03-21 0:28 ` Ævar Arnfjörð Bjarmason
2021-03-22 13:46 ` Johannes Schindelin
0 siblings, 1 reply; 47+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-03-21 0:28 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Junio C Hamano, Jeff King, SZEDER Gábor, Jeff Hostetler
On Fri, Mar 19 2021, Johannes Schindelin wrote:
> Hi Ævar,
>
> On Tue, 9 Mar 2021, Ævar Arnfjörð Bjarmason wrote:
>
>> Rewrite the home-brew JUnit output generation added in
>> 22231908151 (tests: optionally write results as JUnit-style .xml,
>> 2019-01-29) and other subsequent commits to be generated via the TAP
>> output.
>>
>> This is now possible that the TAP output emitted with --tee is
>> guaranteed to be valid TAP, see the preceding commit.
>>
>> The JUnit output is not bug-for-bug compatible with the previous
>> output, but it looks better to me.
>>
>> This also requires installing TAP::Formatter::JUnit[1], perhaps that's
>> not palatable to the users of the --write-junit-xml option.
>
> Indeed. I am trying to keep the dependencies required for the Windows jobs
> of our CI/PR builds to a minimum.
I'm taking this to mean the dependency of the TAP::Formatter::JUnit CPAN
module. Since the tests already depend on Perl, and presumably that Perl
isn't so broken as to not have the built in TAP modules
(e.g. TAP::Parser).
> Note, also, that the JUnit output was mostly relevant for when we used
> Azure Pipelines: it has a specific UI to study test results, figure out
> flaky tests, performance, etc.
>
> Now that we use GitHub Actions, we do not have such a nice test aggregator
> anymore, but we might get one again in the future, who knows.
So there's no known current active user of this JUnit output target, or
at least if such a user exists it's not you anymore?
I'm guessing we'd be unlikely to miss someone targeting JUnit from
git.git's tests who's not on this list to chime in. So is it not used
currently & could be removed?
>> In any case, it'll be easy to whip up our own TAP emitter with a
>> TAP::Parser and TAP::Formatter, both of whom come with perl itself,
>> which we already rely on for tests.
>>
>> It should also be significantly faster on Windows,
>
> I really hate to have to harp on this when talking to you, but... well,
> how can I say it? Perl is _slooooooooooooow_ on Windows.
>
> Like, _really_ slow. Ridiculously slow.
>
> I know, you recently got riled up when Jeff suggested that the Perl hook
> calling FSMonior might be slow, but the truth is: it is super slow. It's
> not even funny how slow it is. And it fills me with no joy having to
> repeat it every time the question comes up whether running any part of Git
> that is written in Perl might affect performance on Windows. I really
> dislike having to be that messenger.
Riled up? No, perplexed: yes. You're referring to
https://lore.kernel.org/git/87h7lgfchm.fsf@evledraar.gmail.com/
So as an aside about that particular issue / slowness I haven't been
able to reproduce:
If it's really a reference to some Windows-specific issue with Perl on
Windows in particular that would certainly be helpful for the rest of us
trying to follow along wondering where these wildly different
performance numbers come from.
So not that we don't want to more to some other fsmonitor implementation
for other reasons, but the Perl part of that hook would be rather easy
to replace with some C JSON encoder or whatever.
> I doubt that you will ever be able to replace my (admittedly slightly
> hacky) C helper with anything written in Perl that does even come close to
> being faster.
>
> In other words, I fear that your work here might not have the outcome you
> hoped for.
We shell out to your C helper at least once for every test we run. By
converting the TAP after the full run we're only invoking Perl once
per-test.
If that one Perl invocation is still much more expensive than doing that
~20-50 times per-test we can easily move the loop to the Perl script and
invoke Perl once per test suite run. At that point you've got 1 run
v.s. >20k runs for the current helper being called in a loop by
test-lib.sh
But I don't have access to a Windows test box. So maybe it's slower than
I'm imagining. How long do the equivalent of these take on Windows (not
sure if the time built-in is there)?:
$ time perl -MTAP::Harness -wE 'say "hello world"'
hello world
real 0m0.018s
user 0m0.013s
sys 0m0.005s
$ time (echo hi | helper/test-tool xml-encode)
hi

real 0m0.002s
user 0m0.001s
sys 0m0.002s
In any case, I regret focusing on the TAP::Formatter::JUnit part in this
RFC. The real meaty part is being able to have stable TAP output, and
thus not having to add N number of output formats into test-lib.sh
itself.
A tool that consumes that can then be e.g. Perl's TAP tooling, or just
some C program or shellscript that calls the code that the below quoted
diff is removing (aside from the test-lib.sh part, which would go away).
But it would be easier to write such tools using Perl's tooling, as it
saves one from writing a parser for TAP.
I'd be surprised if Perl was adding much overhead in that context, and I
see that the existing Windows CI jobs are running through "prove", so
presumably even on Windows e.g. this:
time make T=t000*.sh DEFAULT_TEST_TARGET=prove
Is not much slower than:
time make T=t000*.sh DEFAULT_TEST_TARGET=test
I think this is the Nth time we've had some variant of this "Perl in
git's tests" discussion.
I'm all for finding solutions to whatever issues you have, but I find it
confusing that in these discussions you seem to conflate and jump
between some or all of (just the ones I'm remembering):
1. Some imagined future state where there's no Perl whatsoever in the
test suite or git.git's build toolchain, which isn't the case now,
or anywhere in the forseeable future.
2. A future state where GFW etc. don't need to ship a Perl to users (I
believe this is either close or already there, I haven't kept up..)
3. The slowness of Perl for: tight loops, a few per test, once per test
suite run.
4. How #3 e.g. in a tight loop compares to calling say sed or awk in a
tight shell loop. Is perl uniquely bad on Windows, or is it really a
gripe about our use of shellscripting in general?
6. Not wanting to (understandably) package any more non-core Perl
stuff, understandable, but we always have the option of extending
perl/FromCPAN/ if we find something truly useful (in this case just
having a custom JUnit XML emitter, if it's actually needed by anyone
anymore, should be rather easy).
So e.g. in this case I can guess at some of the gaps and *think* that
some code in t/ that uses TAP::Parser (a perl core module) and was
invoked either once per test-lib.sh invocation, or better (but perhaps
not needed, as it would make it a bit more complex) once per test-suite
run should be fine.
That code could even be implemented as a prove(1) plugin, at which point
we'd be invoking Perl exactly as many times as we are now, but I'd
rather not paint myself into that particular corner without good reason.
>> as we can e.g. write all the *.out files, and only do that conversion at
>> the end for all files in a batch, as opposed to the current
>> implementation of shelling out to test-tool in a loop for each test.
>>
>> 1. https://metacpan.org/pod/distribution/TAP-Formatter-JUnit/script/tap2junit
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>> Makefile | 1 -
>> t/helper/test-date.c | 12 ----
>> t/helper/test-path-utils.c | 21 -------
>> t/helper/test-tool.c | 1 -
>> t/helper/test-tool.h | 1 -
>> t/helper/test-xml-encode.c | 80 ------------------------
>> t/test-lib.sh | 123 ++++---------------------------------
>> 7 files changed, 12 insertions(+), 227 deletions(-)
>> delete mode 100644 t/helper/test-xml-encode.c
>>
>> diff --git a/Makefile b/Makefile
>> index d26b9d62ee9..fa7c52f7a42 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -748,7 +748,6 @@ TEST_BUILTINS_OBJS += test-urlmatch-normalization.o
>> TEST_BUILTINS_OBJS += test-wildmatch.o
>> TEST_BUILTINS_OBJS += test-windows-named-pipe.o
>> TEST_BUILTINS_OBJS += test-write-cache.o
>> -TEST_BUILTINS_OBJS += test-xml-encode.o
>>
>> # Do not add more tests here unless they have extra dependencies. Add
>> # them in TEST_BUILTINS_OBJS above.
>> diff --git a/t/helper/test-date.c b/t/helper/test-date.c
>> index 099eff4f0fc..ebf133943b3 100644
>> --- a/t/helper/test-date.c
>> +++ b/t/helper/test-date.c
>> @@ -8,7 +8,6 @@ static const char *usage_msg = "\n"
>> " test-tool date parse [date]...\n"
>> " test-tool date approxidate [date]...\n"
>> " test-tool date timestamp [date]...\n"
>> -" test-tool date getnanos [start-nanos]\n"
>> " test-tool date is64bit\n"
>> " test-tool date time_t-is64bit\n";
>>
>> @@ -92,15 +91,6 @@ static void parse_approx_timestamp(const char **argv)
>> }
>> }
>>
>> -static void getnanos(const char **argv)
>> -{
>> - double seconds = getnanotime() / 1.0e9;
>> -
>> - if (*argv)
>> - seconds -= strtod(*argv, NULL);
>> - printf("%lf\n", seconds);
>> -}
>> -
>> int cmd__date(int argc, const char **argv)
>> {
>> const char *x;
>> @@ -120,8 +110,6 @@ int cmd__date(int argc, const char **argv)
>> parse_approxidate(argv+1);
>> else if (!strcmp(*argv, "timestamp"))
>> parse_approx_timestamp(argv+1);
>> - else if (!strcmp(*argv, "getnanos"))
>> - getnanos(argv+1);
>> else if (!strcmp(*argv, "is64bit"))
>> return sizeof(timestamp_t) == 8 ? 0 : 1;
>> else if (!strcmp(*argv, "time_t-is64bit"))
>> diff --git a/t/helper/test-path-utils.c b/t/helper/test-path-utils.c
>> index 313a153209c..090596ac492 100644
>> --- a/t/helper/test-path-utils.c
>> +++ b/t/helper/test-path-utils.c
>> @@ -407,27 +407,6 @@ int cmd__path_utils(int argc, const char **argv)
>> return !!res;
>> }
>>
>> - if (argc == 4 && !strcmp(argv[1], "skip-n-bytes")) {
>> - int fd = open(argv[2], O_RDONLY), offset = atoi(argv[3]);
>> - char buffer[65536];
>> -
>> - if (fd < 0)
>> - die_errno("could not open '%s'", argv[2]);
>> - if (lseek(fd, offset, SEEK_SET) < 0)
>> - die_errno("could not skip %d bytes", offset);
>> - for (;;) {
>> - ssize_t count = read(fd, buffer, sizeof(buffer));
>> - if (count < 0)
>> - die_errno("could not read '%s'", argv[2]);
>> - if (!count)
>> - break;
>> - if (write(1, buffer, count) < 0)
>> - die_errno("could not write to stdout");
>> - }
>> - close(fd);
>> - return 0;
>> - }
>> -
>> if (argc > 5 && !strcmp(argv[1], "slice-tests")) {
>> int res = 0;
>> long offset, stride, i;
>> diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
>> index 1876bad8f42..3fa02011b61 100644
>> --- a/t/helper/test-tool.c
>> +++ b/t/helper/test-tool.c
>> @@ -73,7 +73,6 @@ static struct test_cmd cmds[] = {
>> { "tee", cmd__tee },
>> { "trace2", cmd__trace2 },
>> { "urlmatch-normalization", cmd__urlmatch_normalization },
>> - { "xml-encode", cmd__xml_encode },
>> { "wildmatch", cmd__wildmatch },
>> #ifdef GIT_WINDOWS_NATIVE
>> { "windows-named-pipe", cmd__windows_named_pipe },
>> diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
>> index 9b3c1f75267..5028730807e 100644
>> --- a/t/helper/test-tool.h
>> +++ b/t/helper/test-tool.h
>> @@ -63,7 +63,6 @@ int cmd__subprocess(int argc, const char **argv);
>> int cmd__tee(int argc, const char **argv);
>> int cmd__trace2(int argc, const char **argv);
>> int cmd__urlmatch_normalization(int argc, const char **argv);
>> -int cmd__xml_encode(int argc, const char **argv);
>> int cmd__wildmatch(int argc, const char **argv);
>> #ifdef GIT_WINDOWS_NATIVE
>> int cmd__windows_named_pipe(int argc, const char **argv);
>> diff --git a/t/helper/test-xml-encode.c b/t/helper/test-xml-encode.c
>> deleted file mode 100644
>> index a648bbd961c..00000000000
>> --- a/t/helper/test-xml-encode.c
>> +++ /dev/null
>> @@ -1,80 +0,0 @@
>> -#include "test-tool.h"
>> -
>> -static const char *utf8_replace_character = "�";
>> -
>> -/*
>> - * Encodes (possibly incorrect) UTF-8 on <stdin> to <stdout>, to be embedded
>> - * in an XML file.
>> - */
>> -int cmd__xml_encode(int argc, const char **argv)
>> -{
>> - unsigned char buf[1024], tmp[4], *tmp2 = NULL;
>> - ssize_t cur = 0, len = 1, remaining = 0;
>> - unsigned char ch;
>> -
>> - for (;;) {
>> - if (++cur == len) {
>> - len = xread(0, buf, sizeof(buf));
>> - if (!len)
>> - return 0;
>> - if (len < 0)
>> - die_errno("Could not read <stdin>");
>> - cur = 0;
>> - }
>> - ch = buf[cur];
>> -
>> - if (tmp2) {
>> - if ((ch & 0xc0) != 0x80) {
>> - fputs(utf8_replace_character, stdout);
>> - tmp2 = NULL;
>> - cur--;
>> - continue;
>> - }
>> - *tmp2 = ch;
>> - tmp2++;
>> - if (--remaining == 0) {
>> - fwrite(tmp, tmp2 - tmp, 1, stdout);
>> - tmp2 = NULL;
>> - }
>> - continue;
>> - }
>> -
>> - if (!(ch & 0x80)) {
>> - /* 0xxxxxxx */
>> - if (ch == '&')
>> - fputs("&", stdout);
>> - else if (ch == '\'')
>> - fputs("'", stdout);
>> - else if (ch == '"')
>> - fputs(""", stdout);
>> - else if (ch == '<')
>> - fputs("<", stdout);
>> - else if (ch == '>')
>> - fputs(">", stdout);
>> - else if (ch >= 0x20)
>> - fputc(ch, stdout);
>> - else if (ch == 0x09 || ch == 0x0a || ch == 0x0d)
>> - fprintf(stdout, "&#x%02x;", ch);
>> - else
>> - fputs(utf8_replace_character, stdout);
>> - } else if ((ch & 0xe0) == 0xc0) {
>> - /* 110XXXXx 10xxxxxx */
>> - tmp[0] = ch;
>> - remaining = 1;
>> - tmp2 = tmp + 1;
>> - } else if ((ch & 0xf0) == 0xe0) {
>> - /* 1110XXXX 10Xxxxxx 10xxxxxx */
>> - tmp[0] = ch;
>> - remaining = 2;
>> - tmp2 = tmp + 1;
>> - } else if ((ch & 0xf8) == 0xf0) {
>> - /* 11110XXX 10XXxxxx 10xxxxxx 10xxxxxx */
>> - tmp[0] = ch;
>> - remaining = 3;
>> - tmp2 = tmp + 1;
>> - } else
>> - fputs(utf8_replace_character, stdout);
>> - }
>> -
>> - return 0;
>> -}
>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>> index 0070d05234b..4dc41eeccc2 100644
>> --- a/t/test-lib.sh
>> +++ b/t/test-lib.sh
>> @@ -371,6 +371,17 @@ then
>> --escape-stdout ${HARNESS_ACTIVE+--escape-file} \
>> "$GIT_TEST_TEE_OUTPUT_FILE"
>> test "$(cat "$TEST_RESULTS_BASE.exit")" = 0
>> + if test -n "$write_junit_xml"
>> + then
>> + junit_xml_dir="$TEST_OUTPUT_DIRECTORY/out"
>> + mkdir -p "$junit_xml_dir"
>> + junit_xml_base=${0##*/}
>> + junit_xml_path="$junit_xml_dir/TEST-${junit_xml_base%.sh}.xml"
>> + junit_attrs="timestamp=\"$(TZ=UTC date +%Y-%m-%dT%H:%M:%S)\""
>> + tap2junit --name="$TEST_NAME" - \
>> + <"$GIT_TEST_TEE_OUTPUT_FILE" \
>> + >"$junit_xml_path"
>> + fi
>> exit
>> elif test -n "$verbose" -a -n "$HARNESS_ACTIVE"
>> then
>> @@ -588,7 +599,6 @@ export TERM
>>
>> error () {
>> say_color error "error: $*"
>> - finalize_junit_xml
>> GIT_EXIT_OK=t
>> exit 1
>> }
>> @@ -672,56 +682,24 @@ trap '{ code=$?; set +x; } 2>/dev/null; exit $code' INT TERM HUP
>> # the test_expect_* functions instead.
>>
>> test_ok_ () {
>> - if test -n "$write_junit_xml"
>> - then
>> - write_junit_xml_testcase "$*"
>> - fi
>> test_success=$(($test_success + 1))
>> say_color_tap "${verbose:+pass}" "ok $test_count - $@"
>> }
>>
>> test_failure_ () {
>> - if test -n "$write_junit_xml"
>> - then
>> - junit_insert="<failure message=\"not ok $test_count -"
>> - junit_insert="$junit_insert $(xml_attr_encode "$1")\">"
>> - junit_insert="$junit_insert $(xml_attr_encode \
>> - "$(if test -n "$GIT_TEST_TEE_OUTPUT_FILE"
>> - then
>> - test-tool path-utils skip-n-bytes \
>> - "$GIT_TEST_TEE_OUTPUT_FILE" $GIT_TEST_TEE_OFFSET
>> - else
>> - printf '%s\n' "$@" | sed 1d
>> - fi)")"
>> - junit_insert="$junit_insert</failure>"
>> - if test -n "$GIT_TEST_TEE_OUTPUT_FILE"
>> - then
>> - junit_insert="$junit_insert<system-err>$(xml_attr_encode \
>> - "$(cat "$GIT_TEST_TEE_OUTPUT_FILE")")</system-err>"
>> - fi
>> - write_junit_xml_testcase "$1" " $junit_insert"
>> - fi
>> test_failure=$(($test_failure + 1))
>> say_color_tap error "not ok $test_count - $1"
>> shift
>> printf '%s\n' "$*" | sed -e 's/^/# /'
>> - test "$immediate" = "" || { finalize_junit_xml; GIT_EXIT_OK=t; exit 1; }
>> + test "$immediate" = "" || { GIT_EXIT_OK=t; exit 1; }
>> }
>>
>> test_known_broken_ok_ () {
>> - if test -n "$write_junit_xml"
>> - then
>> - write_junit_xml_testcase "$* (breakage fixed)"
>> - fi
>> test_fixed=$(($test_fixed+1))
>> say_color error "ok $test_count - $@ # TODO known breakage vanished"
>> }
>>
>> test_known_broken_failure_ () {
>> - if test -n "$write_junit_xml"
>> - then
>> - write_junit_xml_testcase "$* (known breakage)"
>> - fi
>> test_broken=$(($test_broken+1))
>> say_color_tap warn "not ok $test_count - $@ # TODO known breakage"
>> }
>> @@ -983,10 +961,6 @@ test_start_ () {
>> test_count=$(($test_count+1))
>> maybe_setup_verbose
>> maybe_setup_valgrind
>> - if test -n "$write_junit_xml"
>> - then
>> - junit_start=$(test-tool date getnanos)
>> - fi
>> }
>>
>> test_finish_ () {
>> @@ -1029,13 +1003,6 @@ test_skip () {
>>
>> case "$to_skip" in
>> t)
>> - if test -n "$write_junit_xml"
>> - then
>> - message="$(xml_attr_encode "$skipped_reason")"
>> - write_junit_xml_testcase "$1" \
>> - " <skipped message=\"$message\" />"
>> - fi
>> -
>> say_color_tap skip "ok $test_count # skip $1 ($skipped_reason)"
>> : true
>> ;;
>> @@ -1050,53 +1017,6 @@ test_at_end_hook_ () {
>> :
>> }
>>
>> -write_junit_xml () {
>> - case "$1" in
>> - --truncate)
>> - >"$junit_xml_path"
>> - junit_have_testcase=
>> - shift
>> - ;;
>> - esac
>> - printf '%s\n' "$@" >>"$junit_xml_path"
>> -}
>> -
>> -xml_attr_encode () {
>> - printf '%s\n' "$@" | test-tool xml-encode
>> -}
>> -
>> -write_junit_xml_testcase () {
>> - junit_attrs="name=\"$(xml_attr_encode "$this_test.$test_count $1")\""
>> - shift
>> - junit_attrs="$junit_attrs classname=\"$this_test\""
>> - junit_attrs="$junit_attrs time=\"$(test-tool \
>> - date getnanos $junit_start)\""
>> - write_junit_xml "$(printf '%s\n' \
>> - " <testcase $junit_attrs>" "$@" " </testcase>")"
>> - junit_have_testcase=t
>> -}
>> -
>> -finalize_junit_xml () {
>> - if test -n "$write_junit_xml" && test -n "$junit_xml_path"
>> - then
>> - test -n "$junit_have_testcase" || {
>> - junit_start=$(test-tool date getnanos)
>> - write_junit_xml_testcase "all tests skipped"
>> - }
>> -
>> - # adjust the overall time
>> - junit_time=$(test-tool date getnanos $junit_suite_start)
>> - sed -e "s/\(<testsuite.*\) time=\"[^\"]*\"/\1/" \
>> - -e "s/<testsuite [^>]*/& time=\"$junit_time\"/" \
>> - -e '/^ *<\/testsuite/d' \
>> - <"$junit_xml_path" >"$junit_xml_path.new"
>> - mv "$junit_xml_path.new" "$junit_xml_path"
>> -
>> - write_junit_xml " </testsuite>" "</testsuites>"
>> - write_junit_xml=
>> - fi
>> -}
>> -
>> test_atexit_cleanup=:
>> test_atexit_handler () {
>> # In a succeeding test script 'test_atexit_handler' is invoked
>> @@ -1119,8 +1039,6 @@ test_done () {
>> # removed, so the commands can access pidfiles and socket files.
>> test_atexit_handler
>>
>> - finalize_junit_xml
>> -
>> if test -z "$HARNESS_ACTIVE"
>> then
>> mkdir -p "$TEST_RESULTS_DIR"
>> @@ -1364,23 +1282,6 @@ then
>> test_done
>> fi
>>
>> -if test -n "$write_junit_xml"
>> -then
>> - junit_xml_dir="$TEST_OUTPUT_DIRECTORY/out"
>> - mkdir -p "$junit_xml_dir"
>> - junit_xml_base=${0##*/}
>> - junit_xml_path="$junit_xml_dir/TEST-${junit_xml_base%.sh}.xml"
>> - junit_attrs="name=\"${junit_xml_base%.sh}\""
>> - junit_attrs="$junit_attrs timestamp=\"$(TZ=UTC \
>> - date +%Y-%m-%dT%H:%M:%S)\""
>> - write_junit_xml --truncate "<testsuites>" " <testsuite $junit_attrs>"
>> - junit_suite_start=$(test-tool date getnanos)
>> - if test -n "$GIT_TEST_TEE_OUTPUT_FILE"
>> - then
>> - GIT_TEST_TEE_OFFSET=0
>> - fi
>> -fi
>> -
>> # Convenience
>> # A regexp to match 5, 35 and 40 hexdigits
>> _x05='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'
>> --
>> 2.31.0.rc1.210.g0f8085a843c
>>
>>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC/PATCH 7/7] test-lib: generate JUnit output via TAP
2021-03-21 0:28 ` Ævar Arnfjörð Bjarmason
@ 2021-03-22 13:46 ` Johannes Schindelin
0 siblings, 0 replies; 47+ messages in thread
From: Johannes Schindelin @ 2021-03-22 13:46 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Jeff King, SZEDER Gábor, Jeff Hostetler
[-- Attachment #1: Type: text/plain, Size: 9815 bytes --]
Hi Ævar,
On Sun, 21 Mar 2021, Ævar Arnfjörð Bjarmason wrote:
> On Fri, Mar 19 2021, Johannes Schindelin wrote:
>
> > On Tue, 9 Mar 2021, Ævar Arnfjörð Bjarmason wrote:
> >
> >> Rewrite the home-brew JUnit output generation added in
> >> 22231908151 (tests: optionally write results as JUnit-style .xml,
> >> 2019-01-29) and other subsequent commits to be generated via the TAP
> >> output.
> >>
> >> This is now possible that the TAP output emitted with --tee is
> >> guaranteed to be valid TAP, see the preceding commit.
> >>
> >> The JUnit output is not bug-for-bug compatible with the previous
> >> output, but it looks better to me.
> >>
> >> This also requires installing TAP::Formatter::JUnit[1], perhaps that's
> >> not palatable to the users of the --write-junit-xml option.
> >
> > Indeed. I am trying to keep the dependencies required for the Windows jobs
> > of our CI/PR builds to a minimum.
>
> I'm taking this to mean the dependency of the TAP::Formatter::JUnit CPAN
> module. Since the tests already depend on Perl, and presumably that Perl
> isn't so broken as to not have the built in TAP modules
> (e.g. TAP::Parser).
I have no idea why you insist on modifying something that works quite
well, but yeah, if you want to depend on the implementation detail that we
_currently_ run even the Visual Studio tests (that do _not_ depend on
`prove`) in an environment that has that dependency of `prove`, well, I
guess I'll have to work on maintaining that state of affairs. Doesn't mean
that it makes me happy. It really looks to me like a whole lot of work,
without any benefit.
> > Note, also, that the JUnit output was mostly relevant for when we used
> > Azure Pipelines: it has a specific UI to study test results, figure out
> > flaky tests, performance, etc.
> >
> > Now that we use GitHub Actions, we do not have such a nice test aggregator
> > anymore, but we might get one again in the future, who knows.
>
> So there's no known current active user of this JUnit output target, or
> at least if such a user exists it's not you anymore?
>
> I'm guessing we'd be unlikely to miss someone targeting JUnit from
> git.git's tests who's not on this list to chime in. So is it not used
> currently & could be removed?
No, I want to keep it because I hope that we'll get that lost
functionality back some time soon.
Sorry for not spelling that out more clearly.
> >> In any case, it'll be easy to whip up our own TAP emitter with a
> >> TAP::Parser and TAP::Formatter, both of whom come with perl itself,
> >> which we already rely on for tests.
> >>
> >> It should also be significantly faster on Windows,
> >
> > I really hate to have to harp on this when talking to you, but... well,
> > how can I say it? Perl is _slooooooooooooow_ on Windows.
> >
> > Like, _really_ slow. Ridiculously slow.
> >
> > I know, you recently got riled up when Jeff suggested that the Perl hook
> > calling FSMonior might be slow, but the truth is: it is super slow. It's
> > not even funny how slow it is. And it fills me with no joy having to
> > repeat it every time the question comes up whether running any part of Git
> > that is written in Perl might affect performance on Windows. I really
> > dislike having to be that messenger.
>
> Riled up? No, perplexed: yes. You're referring to
> https://lore.kernel.org/git/87h7lgfchm.fsf@evledraar.gmail.com/
>
> So as an aside about that particular issue / slowness I haven't been
> able to reproduce:
>
> If it's really a reference to some Windows-specific issue with Perl on
> Windows in particular that would certainly be helpful for the rest of us
> trying to follow along wondering where these wildly different
> performance numbers come from.
>
> So not that we don't want to more to some other fsmonitor implementation
ETOOMANYNEGATIONS, can't parse.
> for other reasons, but the Perl part of that hook would be rather easy
> to replace with some C JSON encoder or whatever.
For the time being, I'd rather avoid discussing that because we already
have an experimental, built-in FSMonitor that I personally use. That
FSMonitor will nicely side-step all of the Perl slowness.
> > I doubt that you will ever be able to replace my (admittedly slightly
> > hacky) C helper with anything written in Perl that does even come close to
> > being faster.
> >
> > In other words, I fear that your work here might not have the outcome you
> > hoped for.
>
> We shell out to your C helper at least once for every test we run. By
> converting the TAP after the full run we're only invoking Perl once
> per-test.
>
> If that one Perl invocation is still much more expensive than doing that
> ~20-50 times per-test we can easily move the loop to the Perl script and
> invoke Perl once per test suite run. At that point you've got 1 run
> v.s. >20k runs for the current helper being called in a loop by
> test-lib.sh
>
> But I don't have access to a Windows test box. So maybe it's slower than
> I'm imagining. How long do the equivalent of these take on Windows (not
> sure if the time built-in is there)?:
>
> $ time perl -MTAP::Harness -wE 'say "hello world"'
> hello world
>
> real 0m0.018s
> user 0m0.013s
> sys 0m0.005s
>
> $ time (echo hi | helper/test-tool xml-encode)
> hi

> real 0m0.002s
> user 0m0.001s
> sys 0m0.002s
$ time perl -MTAP::Harness -wE 'say "hello world"'
hello world
real 0m0.326s
user 0m0.030s
sys 0m0.124s
$ time perl -MTAP::Harness -wE 'say "hello world"'
hello world
real 0m0.131s
user 0m0.061s
sys 0m0.046s
$ time perl -MTAP::Harness -wE 'say "hello world"'
hello world
real 0m0.209s
user 0m0.015s
sys 0m0.156s
$ time (echo hi | t/helper/test-tool xml-encode)
hi

real 0m0.165s
user 0m0.015s
sys 0m0.093s
$ time (echo hi | t/helper/test-tool xml-encode)
hi

real 0m0.092s
user 0m0.000s
sys 0m0.047s
$ time (echo hi | t/helper/test-tool xml-encode)
hi

real 0m0.075s
user 0m0.000s
sys 0m0.046s
Completely unscientific, but maybe it gives you an idea.
> In any case, I regret focusing on the TAP::Formatter::JUnit part in this
> RFC. The real meaty part is being able to have stable TAP output, and
> thus not having to add N number of output formats into test-lib.sh
> itself.
>
> A tool that consumes that can then be e.g. Perl's TAP tooling, or just
> some C program or shellscript that calls the code that the below quoted
> diff is removing (aside from the test-lib.sh part, which would go away).
>
> But it would be easier to write such tools using Perl's tooling, as it
> saves one from writing a parser for TAP.
>
> I'd be surprised if Perl was adding much overhead in that context, and I
> see that the existing Windows CI jobs are running through "prove", so
> presumably even on Windows e.g. this:
>
> time make T=t000*.sh DEFAULT_TEST_TARGET=prove
>
> Is not much slower than:
>
> time make T=t000*.sh DEFAULT_TEST_TARGET=test
>
> I think this is the Nth time we've had some variant of this "Perl in
> git's tests" discussion.
And it is still an issue because the Perl we use is running on top of a
POSIX emulation layer which makes everything slow.
> I'm all for finding solutions to whatever issues you have, but I find it
> confusing that in these discussions you seem to conflate and jump
> between some or all of (just the ones I'm remembering):
>
> 1. Some imagined future state where there's no Perl whatsoever in the
> test suite or git.git's build toolchain, which isn't the case now,
> or anywhere in the forseeable future.
>
> 2. A future state where GFW etc. don't need to ship a Perl to users (I
> believe this is either close or already there, I haven't kept up..)
>
> 3. The slowness of Perl for: tight loops, a few per test, once per test
> suite run.
>
> 4. How #3 e.g. in a tight loop compares to calling say sed or awk in a
> tight shell loop. Is perl uniquely bad on Windows, or is it really a
> gripe about our use of shellscripting in general?
>
> 6. Not wanting to (understandably) package any more non-core Perl
> stuff, understandable, but we always have the option of extending
> perl/FromCPAN/ if we find something truly useful (in this case just
> having a custom JUnit XML emitter, if it's actually needed by anyone
> anymore, should be rather easy).
>
> So e.g. in this case I can guess at some of the gaps and *think* that
> some code in t/ that uses TAP::Parser (a perl core module) and was
> invoked either once per test-lib.sh invocation, or better (but perhaps
> not needed, as it would make it a bit more complex) once per test-suite
> run should be fine.
>
> That code could even be implemented as a prove(1) plugin, at which point
> we'd be invoking Perl exactly as many times as we are now, but I'd
> rather not paint myself into that particular corner without good reason.
Yes, I would be much happier if Perl was not required to run our tests. I
would be even happier if we did not run our tests through shell,
especially performance tests, because running performance tests through
shell is like getting the most precise stop watch you can think of and
then looking away from the athletes while timing their sprint.
If you provide patches to bring us closer to that reality, I will try to
set aside time to help. But this "let's remove the C helper that serves us
well and instead use Perl, just to shuffle things around" business really
looks like a lot of churn to me, and I want to spend my time elsewhere.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 47+ messages in thread