Re: [PATCH v3 3/3] test-lib: make BAIL_OUT() work in tests and prereq

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 30, 2021, 14:59 UTC
Message-ID: <211130.86wnkpd6ou.gmgdl@evledraar.gmail.com>
In-Reply-To: <20211130143821.7dz5jj2z2x2q2ytn@fs>

On Tue, Nov 30 2021, Fabian Stelzer wrote:

Show 52 quoted lines

> On 28.11.2021 15:38, Junio C Hamano wrote:
>>Fabian Stelzer <fs@gigacodes.de> writes:
>>
>>>>I was expecting something along the lines of ...
>>>>
>>>># What is written by tests to their FD #1 and #2 are sent to
>>>># different places depending on the test mode (e.g. /dev/null in
>>>># non-verbose mode, piped to tee with --tee option, etc.)  Original
>>>># FD #1 and #2 are saved away to #5 and #7, so that test framework
>>>># can use them to send the output to these low FDs before the
>>>># mode-specific redirection.
>>>>
>>>>... but this only talks about the output side.  The final version
>>>>needs to mention the input side, too.
>>>>
>>>
>>> I like to use the term stdin/err/out since that is what i would grep for
>>> when trying to find out more about the test i/o behaviour.
>>
>>I do not mind phrasing "original FD #1" as "original standard
>>output" at all.  I just wanted to make sure it is clear to readers
>>whose FD #1 and FD #5 we are talking about. In other words, the
>>readers should get a clear understanding of where they are writing
>>to, when the code they write in test_expect_success block outputs to
>>FD #1, and what the code needs to do if it wants to always show
>>something to the original standard output stream.
>
> The current version in my branch is now:
>
> What is written by tests to stdout and stderr is sent so different places
> depending on the test mode (e.g. /dev/null in non-verbose mode, piped to tee
> with --tee option, etc.). We save the original stdin to FD #6 and stdout and
> stderr to #5 and #7, so that the test framework can use them (e.g. for
> printing errors within the test framework) independently of the test mode.
>
> which I think should make this sufficiently clear.
> I'm wondering now though if we should write to #7 instead of #5 in
> BAIL_OUT(). The current use in test-lib/test-lib-functions seems a bit 
> inconsistent.
>
> For example:
> error >&7 "bug in the test script: $*"
> echo >&7 "test_must_fail: only 'git' is allowed: $*"
>
> but:
> echo >&5 "FATAL: Cannot prepare test area"
> echo >&5 "FATAL: Unexpected exit with code $code"
>
> Sometimes these errors result in immediate exit 1, but not always.
>
> I'm not sure if the TAP framework that BAIL_OUT() references expects
> the bail out error on a specific fd.

All TAP must be emitted to stdout. You can test that with e.g.:
    
    $ cat tap.sh
    #!/bin/sh 
    echo "ok 1 one"
    echo "ok 2 two" >&2
    echo "1..1"
    $ prove --exec /bin/sh tap.sh
    tap.sh .. 1/? ok 2 two
    tap.sh .. ok   
    All tests successful.
    Files=1, Tests=1,  0 wallclock secs ( 0.01 usr +  0.00 sys =  0.01 CPU)
    Result: PASS

Note how the "ok 2 two" is emitted to STDERR, and doesn't count towards
the number of tests. If it's changed to:
    
    $ cat tap.sh
    #!/bin/sh
    echo "ok 1 one"
    echo "ok 2 two"
    echo "1..2"
    $ prove --exec /bin/sh tap.sh
    tap.sh .. ok   
    All tests successful.
    Files=1, Tests=2,  0 wallclock secs ( 0.00 usr +  0.01 sys =  0.01 CPU)
    Result: PASS

You can see it runs two tests.

The reason the existing cases are inconsistent are because of various reasons, probably none good at this point.

Some are just because the error handling pre-dates the TAP support in the test suite, I think at this point we should just be moving to making it first-class in terms of TAP support. I.e. it's clearly the most commonly used test mode (and we use it in CI etc.). So we should emit all directives on STDOUT.

And some are probably just copy/pasting, or error handling that didn't consider TAP at the time of writing.

Note that not all of these should be "Bail out!". We should really reserve that for wanting to stall the entire test run, but e.g. not for "cannot prep test area", which might only be a permission error with one trash directory.