Re: [PATCH 6/7] test-lib: make --verbose output valid TAP

On Tue, Mar 09 2021, SZEDER Gábor wrote:

On Tue, Mar 09, 2021 at 09:57:03PM +0100, Ævar Arnfjörð Bjarmason wrote:

quoted

quoted

quoted

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.

quoted

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... :)

quoted

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"

quoted

    # 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.