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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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