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

["Carlo Marcelo Arenas Belón" <carenas@gmail.com>]

On Wed, May 06, 2020 at 04:33:47PM +0200, Johannes Schindelin wrote:
> 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.

FWIW, at least NetBSD's dash (0.5.10.2) always sets LINENO

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

that is what I got from the commmit message but then I was puzzled not
to find the line inside the test case that faile but instead the one
where the last test function was located.

eitherway, I was careless to send the previous version without checking
more deeply; indeed the use of LINENO in the original code was part of
that confusion and is therefore gone in this new one.

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

that was very useful to understand better the constrain that was referred
in the commit message and that forced to build a bash specific solution.

I have to admit too, I was confused by the code, but with the clear guidance
you provided I think the new proposed[1] version is also clearer, and goes
away with the problem that triggered this whole discussion.

Carlo

[1] https://lore.kernel.org/git/20200507055118.69971-1-carenas@gmail.com/