Re: [PATCH] tests: send "bug in the test script" errors to the script's stderr
On Tue, Nov 20, 2018 at 05:58:25AM -0500, Jeff King wrote: > On Tue, Nov 20, 2018 at 05:45:28AM -0500, Jeff King wrote: > > > On Tue, Nov 20, 2018 at 12:34:04AM +0100, SZEDER Gábor wrote: > > > > > > I do notice that many of the existing "FATAL:" errors use descriptor 5, > > > > which goes to stdout. I'm not sure if those should actually be going to > > > > stderr (or if there's some TAP significance to those lines). > > > > > > I chose to send these messages to standard error, because they are, > > > well, errors. TAP only cares about stdout, but by aborting the test > > > script in BUG(), error() or die() we are already violating TAP anyway, > > > so I don't think it matters whether we send "bug in test script" or > > > "FATAL" errors to stdout or stderr. > > > > Yeah, I agree it probably doesn't matter. I was mostly suggesting to > > make the existing ">&5" into ">&7" for consistency. But I don't think > > that needs to block your patch. > > By the way, I did check to see whether this might help the situation > where running under "prove" we see only "Dubious..." and not the actual > error() message produced by the test script. But no, it eats both stdout > and stderr. You can sneak a line through by prepending it with "#", but > only if "prove -o" is used (and even then, it's hard to associate it > with a particular test when you're running many in parallel). Just to be clear: I don't mind if in some combination of test harnesses and test options a "bug in the test script" message doesn't reach the terminal as long as I get a clearly visible error from somewhere. > So I guess the status quo is not too bad: prove says "dubious", and then > you re-run the test with "./t1234-foo.sh -v -i" and you get to see the > error. And with '--verbose-log' the "bug in the test script" message goes to the test's log as well, even when it has to go through fd 7 first, so if you use 'prove' and your GIT_TEST_OPTS includes '--verbose-log' then you can just look at the log, there's no need to re-run the test.
Re: [PATCH] tests: send "bug in the test script" errors to the script's stderr
On Tue, Nov 20, 2018 at 05:45:28AM -0500, Jeff King wrote: > On Tue, Nov 20, 2018 at 12:34:04AM +0100, SZEDER Gábor wrote: > > > > I do notice that many of the existing "FATAL:" errors use descriptor 5, > > > which goes to stdout. I'm not sure if those should actually be going to > > > stderr (or if there's some TAP significance to those lines). > > > > I chose to send these messages to standard error, because they are, > > well, errors. TAP only cares about stdout, but by aborting the test > > script in BUG(), error() or die() we are already violating TAP anyway, > > so I don't think it matters whether we send "bug in test script" or > > "FATAL" errors to stdout or stderr. > > Yeah, I agree it probably doesn't matter. I was mostly suggesting to > make the existing ">&5" into ">&7" for consistency. But I don't think > that needs to block your patch. By the way, I did check to see whether this might help the situation where running under "prove" we see only "Dubious..." and not the actual error() message produced by the test script. But no, it eats both stdout and stderr. You can sneak a line through by prepending it with "#", but only if "prove -o" is used (and even then, it's hard to associate it with a particular test when you're running many in parallel). So I guess the status quo is not too bad: prove says "dubious", and then you re-run the test with "./t1234-foo.sh -v -i" and you get to see the error. -Peff
Re: [PATCH] tests: send "bug in the test script" errors to the script's stderr
On Tue, Nov 20, 2018 at 12:34:04AM +0100, SZEDER Gábor wrote: > > I do notice that many of the existing "FATAL:" errors use descriptor 5, > > which goes to stdout. I'm not sure if those should actually be going to > > stderr (or if there's some TAP significance to those lines). > > I chose to send these messages to standard error, because they are, > well, errors. TAP only cares about stdout, but by aborting the test > script in BUG(), error() or die() we are already violating TAP anyway, > so I don't think it matters whether we send "bug in test script" or > "FATAL" errors to stdout or stderr. Yeah, I agree it probably doesn't matter. I was mostly suggesting to make the existing ">&5" into ">&7" for consistency. But I don't think that needs to block your patch. > BTW, TAP understands "Bail out!" as bail out from the _entire_ test > suite, but that's not what we want here, I'd think. > > https://testanything.org/tap-specification.html#bail-out Yes, I added the only existing "Bail out!" to test-lib.sh. :) I agree that's not what we want here. I actually think the current behavior (to exit non-zero) does what we want. A TAP harness will realize that's an error, but not block other scripts. -Peff
Re: [PATCH] tests: send "bug in the test script" errors to the script's stderr
On Mon, Nov 19, 2018 at 02:44:32PM -0500, Jeff King wrote:
> On Mon, Nov 19, 2018 at 02:13:26PM +0100, SZEDER Gábor wrote:
>
> > Send these "bug in the test script" error messages directly to the
> > test scripts standard error and thus to the terminal, so those bugs
> > will be much harder to overlook. Instead of updating all ~20 such
> > 'error' calls with a redirection, let's add a BUG() function to
> > 'test-lib.sh', wrapping an 'error' call with the proper redirection
> > and also including the common prefix of those error messages, and
> > convert all those call sites [4] to use this new BUG() function
> > instead.
>
> Yes, I think this is an improvement.
>
> > +BUG () {
> > + error >&7 "bug in the test script: $*"
> > +}
>
> I naively expected this to go to >&4, but of course that is the
> conditional "stderr or /dev/null, depending on --verbose" descriptor.
The first version of this patch did send the error message to fd 4 ;)
> I do notice that many of the existing "FATAL:" errors use descriptor 5,
> which goes to stdout. I'm not sure if those should actually be going to
> stderr (or if there's some TAP significance to those lines).
I chose to send these messages to standard error, because they are,
well, errors. TAP only cares about stdout, but by aborting the test
script in BUG(), error() or die() we are already violating TAP anyway,
so I don't think it matters whether we send "bug in test script" or
"FATAL" errors to stdout or stderr.
BTW, TAP understands "Bail out!" as bail out from the _entire_ test
suite, but that's not what we want here, I'd think.
https://testanything.org/tap-specification.html#bail-out
Re: [PATCH] tests: send "bug in the test script" errors to the script's stderr
On Mon, Nov 19, 2018 at 02:13:26PM +0100, SZEDER Gábor wrote:
> Send these "bug in the test script" error messages directly to the
> test scripts standard error and thus to the terminal, so those bugs
> will be much harder to overlook. Instead of updating all ~20 such
> 'error' calls with a redirection, let's add a BUG() function to
> 'test-lib.sh', wrapping an 'error' call with the proper redirection
> and also including the common prefix of those error messages, and
> convert all those call sites [4] to use this new BUG() function
> instead.
Yes, I think this is an improvement.
> +BUG () {
> + error >&7 "bug in the test script: $*"
> +}
I naively expected this to go to >&4, but of course that is the
conditional "stderr or /dev/null, depending on --verbose" descriptor. I
have a feeling that we could get rid of descriptors 5 and 7 in favor of
3 and 4, if we did the conditional redirection when running each test,
instead of ahead of time.
But unless we are running out of descriptors, it's not worth the effort
(it's debatable whether we are; 9be795fbce (t5615: avoid re-using
descriptor 4, 2017-12-08) made me nervous, but it's more about the
special-ness of BASHE_XTRACEFD than anything).
Anyway, that's all a tangent to your patch.
I do notice that many of the existing "FATAL:" errors use descriptor 5,
which goes to stdout. I'm not sure if those should actually be going to
stderr (or if there's some TAP significance to those lines).
-Peff
