Re: [PATCH v5 10/12] tests: when run in Bash, annotate test failures with file name/line number

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 3866 bytes --]

Hi Carlo,

On Wed, 6 May 2020, Carlo Marcelo Arenas Belón wrote:

> On Wed, May 06, 2020 at 02:54:38PM +0200, Johannes Schindelin wrote:
> > On Wed, 6 May 2020, Carlo Marcelo Arenas Belón wrote:
> > > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > > index 1b221951a8..a8f8e4106b 100644
> > > --- a/t/test-lib.sh
> > > +++ b/t/test-lib.sh
> > > @@ -676,15 +676,9 @@ die () {
> > >  }
> > >
> > >  file_lineno () {
> > > -	test -z "$GIT_TEST_FRAMEWORK_SELFTEST" && test -n "$BASH" || return 0
> > > -	local i
> > > -	for i in ${!BASH_SOURCE[*]}
> > > -	do
> > > -		case $i,"${BASH_SOURCE[$i]##*/}" in
> > > -		0,t[0-9]*.sh) echo "t/${BASH_SOURCE[$i]}:$LINENO: ${1+$1: }"; return;;
> > > -		*,t[0-9]*.sh) echo "t/${BASH_SOURCE[$i]}:${BASH_LINENO[$(($i-1))]}: ${1+$1: }"; return;;
> > > -		esac
> > > -	done
> > > +	test -z "$GIT_TEST_FRAMEWORK_SELFTEST" || return 0
> > > +
> > > +	echo "$0:$LINENO: ${1+$1: }"
> >
> > That suppresses the error all right.
> >
> > Unfortunately, it completely breaks the feature. At that point, `$LINENO`
> > is either unset (e.g. in `dash`) or it contains the number of the line
> > _containing the `echo`. That is totally useless information at this point,
> > we want the line number of the caller.
>
> that seems like a bug in dash, which NetBSD sh doesn't have, as LINENO
> wouldn't be unset.

And your patch makes this a real problem as you no longer skip the `echo`
in the non-Bash case.

That's what I wanted to point out: this needs to be fixed.

> > Try this, for example:
> >
> > ```
> > #!/bin/sh
> >
> > file_lineno () {
> > 	echo "$0:$LINENO: hello"
> > }
> >
> > file_lineno
> > ```
> >
> > When you run this, it will print `4`. What we want is `7`.
>
> so you need instead :
>
> ```
> #!/bin/sh
>
> file_lineno () {
> 	echo "$0:$1: hello"
> }
>
> file_lineno $LINENO

No.

Please understand what the intention of the current (Bash-specific) code
is: in case that there is a failure, it needs to print out the file and
line number of the actual statement that caused that problem.

Take this example:

	test_expect_success 'For Carlo' '
		false
	'

Obviously, this will fail, and it will print out an error message. What we
want here is that the file that contains that `test_expect_success` and
the actual line number of this call are printed.

Your suggestion would be to clutter each and every such call with
`$LINENO`, like so:

	test_expect_success $LINENO 'For Carlo' '

I don't think that is sensible an idea.

Besides, it would _still_ not work, for parameterized functions that call
`test_expect_success` and that are defined in `lib-<whatever>.sh`.

Example:

	# in t/lib-whatever.sh
	super_repetitive_test () {
		test_expect_success "first $1" '
			...
		'

		test_expect_success "second $1" '
			...
		'

		...

		test_expect_success "gazillionth $1" '
			...
		'
	}

	# in t/t1234-actual-caller.sh
	. lib-whatever.sh

	super_repetitive_test hello
	super_repetitive_test world
	super_repetitive_test good-bye
	super_repetitive_test dreams

We will not want to print out the line number of the call in
t/lib-whatever.sh. That is what your proposal would amount to, unless you
want to clutter even the `super_repetitive_test` calles, which would fly
even less.

> > Even worse, as `$0` does _not_ contain `test-lib.sh` at this point,
> > the printed information is totally bogus.
>
> not sure I understand what you mean here, at least when runnning with bash
> the original code shows $0 correctly as t????.sh when I tried to force a
> failure to test.

Yes, $0 shows the correct file. But since the line number that is printed
is from a totally different file, the combination <file>:<lineno> is
completely and utterly bogus. Misleading. Less than useless.

Ciao,
Dscho