| Message ID | 20211120150401.254408-4-fs@gigacodes.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | test-lib: improve missing prereq handling | expand |
On Sat, Nov 20 2021, Fabian Stelzer wrote: > BAIL_OUT() is meant to abort the whole test run and print a message with > a standard prefix that can be parsed to stdout. Since for every test the > normal fd`s are redirected in test_eval_ this output would not be seen > when used within the context of a test or prereq like we do in > test_have_prereq(). To make this function work in these contexts we move > the setup of the fd aliases a few lines up before the first use of > BAIL_OUT() and then have this function always print to the alias. > > Signed-off-by: Fabian Stelzer <fs@gigacodes.de> > --- > t/test-lib.sh | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/t/test-lib.sh b/t/test-lib.sh > index f61da562f6..96a09a26a1 100644 > --- a/t/test-lib.sh > +++ b/t/test-lib.sh > @@ -589,6 +589,11 @@ USER_TERM="$TERM" > TERM=dumb > export TERM USER_TERM > > +# Set up additional fds so we can control single test i/o > +exec 5>&1 > +exec 6<&0 > +exec 7>&2 > + > _error_exit () { > finalize_junit_xml > GIT_EXIT_OK=t > @@ -612,7 +617,7 @@ BAIL_OUT () { > local bail_out="Bail out! " > local message="$1" > > - say_color error $bail_out "$message" > + say_color error $bail_out "$message" >&5 > _error_exit > } > > @@ -637,9 +642,6 @@ then > exit 0 > fi > > -exec 5>&1 > -exec 6<&0 > -exec 7>&2 This doesn't break (I think) with your change here because you only manipulate >&5, but I think the post-image would be lot clearer if... > if test "$verbose_log" = "t" > then > exec 3>>"$GIT_TEST_TEE_OUTPUT_FILE" 4>&3 ...this bit of code were moved up along with the "exec". They're currently intentionally snuggled together as we conditionally set the 3rd and 4th fd depending on verbose/tee settings right after the setup of 5/6/7, keeping them grouped in the post-image makes more sense than splitting them up here.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> -exec 5>&1 >> -exec 6<&0 >> -exec 7>&2 > > This doesn't break (I think) with your change here because you only > manipulate >&5, but I think the post-image would be lot clearer if... > >> if test "$verbose_log" = "t" >> then >> exec 3>>"$GIT_TEST_TEE_OUTPUT_FILE" 4>&3 > > ...this bit of code were moved up along with the "exec". They're > currently intentionally snuggled together as we conditionally set the > 3rd and 4th fd depending on verbose/tee settings right after the setup > of 5/6/7, keeping them grouped in the post-image makes more sense than > splitting them up here. I actually have to wonder if the handling of 3 and 4 should be moved down, not up like you suggest, so that they are grouped together with the maybe_teardown_verbose and the maybe_setup_verbose helper functions. The lines we see here are the file descriptors that are always redirected, which is a bit different. Raising all of them up to group them together is also fine. In any case, the comment in front of the block of exec wants to become a bit more detailed than just "# Set up additional fds", with an explanation about which FD is used for what. And any change that involves handling of FD #4 probably wants to further include the BASH_XTRACEFD setting. Thanks.
On 22.11.2021 09:05, Junio C Hamano wrote: >Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >>> -exec 5>&1 >>> -exec 6<&0 >>> -exec 7>&2 >> >> This doesn't break (I think) with your change here because you only >> manipulate >&5, but I think the post-image would be lot clearer if... >> >>> if test "$verbose_log" = "t" >>> then >>> exec 3>>"$GIT_TEST_TEE_OUTPUT_FILE" 4>&3 >> >> ...this bit of code were moved up along with the "exec". They're >> currently intentionally snuggled together as we conditionally set the >> 3rd and 4th fd depending on verbose/tee settings right after the setup >> of 5/6/7, keeping them grouped in the post-image makes more sense than >> splitting them up here. > >I actually have to wonder if the handling of 3 and 4 should be moved >down, not up like you suggest, so that they are grouped together >with the maybe_teardown_verbose and the maybe_setup_verbose helper >functions. The lines we see here are the file descriptors that are >always redirected, which is a bit different. Raising all of them up >to group them together is also fine. Since there is lots of other things happening in between i'd rather leave 3 & 4 where they are for now. I'm having a hard time grasping what test-lib.sh does when / where since there is lots of intermingled function definition / executed code. It could probably benefit from a new include to move functions to (and wrap some code into new ones). At the moment i'm a bit swamped with other work though :/ > >In any case, the comment in front of the block of exec wants to >become a bit more detailed than just "# Set up additional fds", with >an explanation about which FD is used for what. > How about: # Set up additional fds to allow i/o with the surrounding test # harness when redirecting individual test i/o in test_eval_ # fd 5 -> stdout # fd 6 <- stdin # fd 7 -> stderr exec 5>&1 exec 6<&0 exec 7>&2
Fabian Stelzer <fs@gigacodes.de> writes: >>In any case, the comment in front of the block of exec wants to >>become a bit more detailed than just "# Set up additional fds", with >>an explanation about which FD is used for what. >> > > How about: > > # Set up additional fds to allow i/o with the surrounding test > # harness when redirecting individual test i/o in test_eval_ This does not quite say how "setting up additional fds" helpss the test harness, though. And this ... > # fd 5 -> stdout > # fd 6 <- stdin > # fd 7 -> stderr ... is literal translation of what is written below, without adding any new information. > exec 5>&1 > exec 6<&0 > exec 7>&2 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. Thanks.
On 26.11.2021 13:02, Junio C Hamano wrote: >Fabian Stelzer <fs@gigacodes.de> writes: > >>>In any case, the comment in front of the block of exec wants to >>>become a bit more detailed than just "# Set up additional fds", with >>>an explanation about which FD is used for what. >>> >> >> How about: >> >> # Set up additional fds to allow i/o with the surrounding test >> # harness when redirecting individual test i/o in test_eval_ > >This does not quite say how "setting up additional fds" helpss the >test harness, though. And this ... > >> # fd 5 -> stdout >> # fd 6 <- stdin >> # fd 7 -> stderr > >... is literal translation of what is written below, without adding >any new information. > >> exec 5>&1 >> exec 6<&0 >> exec 7>&2 > >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 understand that you would like to give exaxples what the fd aliases are used for and I think thats a good idea. But I'm having a bit of a hard time understanding the use of the stdin alias. I did not find a single use in the test suite - but my grep might not have been complete. Or if it's just there for the sake of completeness. As far as i understand the test framework runs the individual tests in test_eval_ redirecting its stdout/err to fd #3 & #4 (with some xtracefd logic for -x) and passing /dev/null as stdin to the test. What would FD #6 actually be used for then? A test lib function being called from within a test that expects user input?
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.
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.
On Tue, Nov 30 2021, Fabian Stelzer wrote: > 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.
diff --git a/t/test-lib.sh b/t/test-lib.sh index f61da562f6..96a09a26a1 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -589,6 +589,11 @@ USER_TERM="$TERM" TERM=dumb export TERM USER_TERM +# Set up additional fds so we can control single test i/o +exec 5>&1 +exec 6<&0 +exec 7>&2 + _error_exit () { finalize_junit_xml GIT_EXIT_OK=t @@ -612,7 +617,7 @@ BAIL_OUT () { local bail_out="Bail out! " local message="$1" - say_color error $bail_out "$message" + say_color error $bail_out "$message" >&5 _error_exit } @@ -637,9 +642,6 @@ then exit 0 fi -exec 5>&1 -exec 6<&0 -exec 7>&2 if test "$verbose_log" = "t" then exec 3>>"$GIT_TEST_TEE_OUTPUT_FILE" 4>&3
BAIL_OUT() is meant to abort the whole test run and print a message with a standard prefix that can be parsed to stdout. Since for every test the normal fd`s are redirected in test_eval_ this output would not be seen when used within the context of a test or prereq like we do in test_have_prereq(). To make this function work in these contexts we move the setup of the fd aliases a few lines up before the first use of BAIL_OUT() and then have this function always print to the alias. Signed-off-by: Fabian Stelzer <fs@gigacodes.de> --- t/test-lib.sh | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)