[RFC] t: move metadata into TAP test description
diff mbox series

Message ID 20200515150041.22873-1-carenas@gmail.com
State New
Headers show
Series
  • [RFC] t: move metadata into TAP test description
Related show

Commit Message

Carlo Marcelo Arenas Belón May 15, 2020, 3 p.m. UTC
662f9cf154 (tests: when run in Bash, annotate test failures with file
name/line number, 2020-04-11) adds metadata to the TAP output to help
identify the location of the failed test, but does it in a way that
break the TAP format and therefore confuses prove.

Move the metadata to the description to workaround the issue and
change the regex from 676eb0c1ce (ci: add a problem matcher for GitHub
Actions, 2020-04-11) to match.

Reported-by: Alban Gruin <alban.gruin@gmail.com>
---
 ci/git-problem-matcher.json | 10 +++++-----
 t/test-lib.sh               |  6 +++---
 2 files changed, 8 insertions(+), 8 deletions(-)

Comments

Eric Sunshine May 15, 2020, 3:08 p.m. UTC | #1
On Fri, May 15, 2020 at 11:00 AM Carlo Marcelo Arenas Belón
<carenas@gmail.com> wrote:
> 662f9cf154 (tests: when run in Bash, annotate test failures with file
> name/line number, 2020-04-11) adds metadata to the TAP output to help
> identify the location of the failed test, but does it in a way that
> break the TAP format and therefore confuses prove.
>
> Move the metadata to the description to workaround the issue and
> change the regex from 676eb0c1ce (ci: add a problem matcher for GitHub
> Actions, 2020-04-11) to match.
>
> Reported-by: Alban Gruin <alban.gruin@gmail.com>

Missing sign-off.
Alban Gruin May 15, 2020, 3:38 p.m. UTC | #2
Le 15/05/2020 à 17:00, Carlo Marcelo Arenas Belón a écrit :
> 662f9cf154 (tests: when run in Bash, annotate test failures with file
> name/line number, 2020-04-11) adds metadata to the TAP output to help
> identify the location of the failed test, but does it in a way that
> break the TAP format and therefore confuses prove.
> 
> Move the metadata to the description to workaround the issue and
> change the regex from 676eb0c1ce (ci: add a problem matcher for GitHub
> Actions, 2020-04-11) to match.
> 
> Reported-by: Alban Gruin <alban.gruin@gmail.com>
> ---
>  ci/git-problem-matcher.json | 10 +++++-----
>  t/test-lib.sh               |  6 +++---
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/ci/git-problem-matcher.json b/ci/git-problem-matcher.json
> index 506dfbd97f..e10e88bba1 100644
> --- a/ci/git-problem-matcher.json
> +++ b/ci/git-problem-matcher.json
> @@ -4,11 +4,11 @@
>              "owner": "git-test-suite",
>              "pattern": [
>                  {
> -                    "regexp": "^([^ :]+\\.sh):(\\d+): (error|warning|info):\\s+(.*)$",
> -                    "file": 1,
> -                    "line": 2,
> -                    "severity": 3,
> -                    "message": 4
> +                    "regexp": "^(.*)(error|warning|info):\\([^ :]+\\.sh):(\\d+)\\)$",
> +                    "file": 3,
> +                    "line": 4,
> +                    "severity": 2,
> +                    "message": 1
>                  }
>              ]
>          }
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index baf94546da..d5f59ab3bf 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -682,8 +682,8 @@ file_lineno () {
>  		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;;
> +			0,t[0-9]*.sh) echo "(${1+$1:}t/${BASH_SOURCE[$i]}:$LINENO)"; return;;
> +			*,t[0-9]*.sh) echo "(${1+$1:}t/${BASH_SOURCE[$i]}:${BASH_LINENO[$(($i-1))]})"; return;;

Could you add a space after the "error:"/"warn:"/"info:"?

>  			esac
>  		done
>  	'
> @@ -734,7 +734,7 @@ test_failure_ () {
>  		write_junit_xml_testcase "$1" "      $junit_insert"
>  	fi
>  	test_failure=$(($test_failure + 1))
> -	say_color error "$(file_lineno error)not ok $test_count - $1"
> +	say_color error "not ok $test_count - $1$(file_lineno error)"

Could you add a space before `$(file_lineno error)'?

>  	shift
>  	printf '%s\n' "$*" | sed -e 's/^/#	/'
>  	test "$immediate" = "" || { finalize_junit_xml; GIT_EXIT_OK=t; exit 1; }
> 

Thinking about it, I think it would make sense to put this kind of
information in a diagnostic line, as we already do for the code of a
failed test.

Instead of this:

> not ok 1 - plain(error:t/./t0001-init.sh:33)
> #
> #               false &&
> #               git init plain &&
> #               check_config plain/.git false unset
> #

Something like this:

> not ok 1 - plain
> # (error:t/./t0001-init.sh:33)
> #
> #               false &&
> #               git init plain &&
> #               check_config plain/.git false unset
> #

Cheers,
Alban
Carlo Marcelo Arenas Belón May 15, 2020, 3:45 p.m. UTC | #3
On Fri, May 15, 2020 at 05:38:21PM +0200, Alban Gruin wrote:
> 
> Thinking about it, I think it would make sense to put this kind of
> information in a diagnostic line, as we already do for the code of a
> failed test.
> 
> Instead of this:
> 
> > not ok 1 - plain(error:t/./t0001-init.sh:33)
> > #
> > #               false &&
> > #               git init plain &&
> > #               check_config plain/.git false unset
> > #
> 
> Something like this:
> 
> > not ok 1 - plain
> > # (error:t/./t0001-init.sh:33)
> > #
> > #               false &&
> > #               git init plain &&
> > #               check_config plain/.git false unset
> > #

indeed, and that is why I mentioned this as a workaround only.

to move to a format like the one you suggest, it might be better to
do it by also moving to TAP13[1] (which allows all that metadata)

but of course that would also require us to do more changes to the
integration with GitHub's problem matchers and probably a lot more
changes I am not even aware of.

Carlo

[1] http://testanything.org/tap-version-13-specification.html
Junio C Hamano May 15, 2020, 4:38 p.m. UTC | #4
Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> 662f9cf154 (tests: when run in Bash, annotate test failures with file
> name/line number, 2020-04-11) adds metadata to the TAP output to help
> identify the location of the failed test, but does it in a way that
> break the TAP format and therefore confuses prove.
>
> Move the metadata to the description to workaround the issue and
> change the regex from 676eb0c1ce (ci: add a problem matcher for GitHub
> Actions, 2020-04-11) to match.
>
> Reported-by: Alban Gruin <alban.gruin@gmail.com>
> ---
>  ci/git-problem-matcher.json | 10 +++++-----
>  t/test-lib.sh               |  6 +++---
>  2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/ci/git-problem-matcher.json b/ci/git-problem-matcher.json
> index 506dfbd97f..e10e88bba1 100644
> --- a/ci/git-problem-matcher.json
> +++ b/ci/git-problem-matcher.json
> @@ -4,11 +4,11 @@
>              "owner": "git-test-suite",
>              "pattern": [
>                  {
> -                    "regexp": "^([^ :]+\\.sh):(\\d+): (error|warning|info):\\s+(.*)$",
> -                    "file": 1,
> -                    "line": 2,
> -                    "severity": 3,
> -                    "message": 4
> +                    "regexp": "^(.*)(error|warning|info):\\([^ :]+\\.sh):(\\d+)\\)$",

Do we ever run our tests in ci environments with a shell other than
bash?  Would we still work fine without the "error:<F>:<L>:" suffix?
I guess we just live with degraded output in such a case, and is
much better than breaking TAP when tests are run under bash.

The tail part of this pattern (starting at error/warn/info) is tight
enough that the overly loose (.*) for the message part may still
match the "rest of the message that has arbitrary string" just fine,
but it might be less error prone to add an unusual letter (e.g. "|")
in the output between "not ok $test_count - $1" and the output from
the file_lineno() helper to make sure that we won't end up eating a
word "error" etc. at the end of the test title.  I dunno.

> +                    "file": 3,
> +                    "line": 4,
> +                    "severity": 2,
> +                    "message": 1
>                  }
>              ]
>          }
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index baf94546da..d5f59ab3bf 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -682,8 +682,8 @@ file_lineno () {
>  		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;;
> +			0,t[0-9]*.sh) echo "(${1+$1:}t/${BASH_SOURCE[$i]}:$LINENO)"; return;;
> +			*,t[0-9]*.sh) echo "(${1+$1:}t/${BASH_SOURCE[$i]}:${BASH_LINENO[$(($i-1))]})"; return;;
>  			esac
>  		done
>  	'
> @@ -734,7 +734,7 @@ test_failure_ () {
>  		write_junit_xml_testcase "$1" "      $junit_insert"
>  	fi
>  	test_failure=$(($test_failure + 1))
> -	say_color error "$(file_lineno error)not ok $test_count - $1"
> +	say_color error "not ok $test_count - $1$(file_lineno error)"
>  	shift
>  	printf '%s\n' "$*" | sed -e 's/^/#	/'
>  	test "$immediate" = "" || { finalize_junit_xml; GIT_EXIT_OK=t; exit 1; }
Junio C Hamano May 15, 2020, 4:50 p.m. UTC | #5
Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:

> On Fri, May 15, 2020 at 05:38:21PM +0200, Alban Gruin wrote:
>> 
>> Thinking about it, I think it would make sense to put this kind of
>> information in a diagnostic line, as we already do for the code of a
>> failed test.
>> 
>> Instead of this:
>> 
>> > not ok 1 - plain(error:t/./t0001-init.sh:33)
>> > #
>> > #               false &&
>> > #               git init plain &&
>> > #               check_config plain/.git false unset
>> > #
>> 
>> Something like this:
>> 
>> > not ok 1 - plain
>> > # (error:t/./t0001-init.sh:33)
>> > #
>> > #               false &&
>> > #               git init plain &&
>> > #               check_config plain/.git false unset
>> > #
>
> indeed, and that is why I mentioned this as a workaround only.
>
> to move to a format like the one you suggest, it might be better to
> do it by also moving to TAP13[1] (which allows all that metadata)
>
> but of course that would also require us to do more changes to the
> integration with GitHub's problem matchers and probably a lot more
> changes I am not even aware of.

At this late stage in the cycle, would it be a safer change to
revert the whole thing, I wonder, rather than piling fixes on top of
fixes to the initial breakage?

303775a2 (t/test_lib: avoid naked bash arrays in file_lineno, 2020-05-07)
662f9cf1 (tests: when run in Bash, annotate test failures with file name/line number, 2020-04-11)

I'll prepare a topic to revert these two directly on top of -rc0 and
see how well it goes.
Carlo Marcelo Arenas Belón May 15, 2020, 5:14 p.m. UTC | #6
On Fri, May 15, 2020 at 09:50:38AM -0700, Junio C Hamano wrote:
> 
> At this late stage in the cycle, would it be a safer change to
> revert the whole thing, I wonder, rather than piling fixes on top of
> fixes to the initial breakage?
> 
> 303775a2 (t/test_lib: avoid naked bash arrays in file_lineno, 2020-05-07)
> 662f9cf1 (tests: when run in Bash, annotate test failures with file name/line number, 2020-04-11)

will also need:

  676eb0c1ce (ci: add a problem matcher for GitHub Actions, 2020-04-11)

Carlo
Carlo Marcelo Arenas Belón May 15, 2020, 5:22 p.m. UTC | #7
On Fri, May 15, 2020 at 09:38:06AM -0700, Junio C Hamano wrote:
> 
> Do we ever run our tests in ci environments with a shell other than
> bash?

the alpine test uses busybox, and the cirrus-ci job uses FreeBSD's sh
but most of the other tests (including all windows jobs) use bash and
therefore can benefit from this as shown by :

  https://github.com/carenas/git/commit/5517179c37904e9b8ac6408fa22643759e91538d#annotation_205089161

Carlo
Junio C Hamano May 15, 2020, 5:23 p.m. UTC | #8
Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:

> On Fri, May 15, 2020 at 09:50:38AM -0700, Junio C Hamano wrote:
>> 
>> At this late stage in the cycle, would it be a safer change to
>> revert the whole thing, I wonder, rather than piling fixes on top of
>> fixes to the initial breakage?
>> 
>> 303775a2 (t/test_lib: avoid naked bash arrays in file_lineno, 2020-05-07)
>> 662f9cf1 (tests: when run in Bash, annotate test failures with file name/line number, 2020-04-11)
>
> will also need:
>
>   676eb0c1ce (ci: add a problem matcher for GitHub Actions, 2020-04-11)

Yeah, I think that is a good idea.  I suspect that leaving it there
won't cause problems, though---it would be just nothing is found to
be clicked and that's the end of it, no?

Will add a revert to the series anyway.

Thanks.
Alban Gruin May 15, 2020, 7:04 p.m. UTC | #9
Le 15/05/2020 à 17:00, Carlo Marcelo Arenas Belón a écrit :
> 662f9cf154 (tests: when run in Bash, annotate test failures with file
> name/line number, 2020-04-11) adds metadata to the TAP output to help
> identify the location of the failed test, but does it in a way that
> break the TAP format and therefore confuses prove.
> 
> Move the metadata to the description to workaround the issue and
> change the regex from 676eb0c1ce (ci: add a problem matcher for GitHub
> Actions, 2020-04-11) to match.
> 
> Reported-by: Alban Gruin <alban.gruin@gmail.com>
> ---
>  ci/git-problem-matcher.json | 10 +++++-----
>  t/test-lib.sh               |  6 +++---
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/ci/git-problem-matcher.json b/ci/git-problem-matcher.json
> index 506dfbd97f..e10e88bba1 100644
> --- a/ci/git-problem-matcher.json
> +++ b/ci/git-problem-matcher.json
> @@ -4,11 +4,11 @@
>              "owner": "git-test-suite",
>              "pattern": [
>                  {
> -                    "regexp": "^([^ :]+\\.sh):(\\d+): (error|warning|info):\\s+(.*)$",
> -                    "file": 1,
> -                    "line": 2,
> -                    "severity": 3,
> -                    "message": 4
> +                    "regexp": "^(.*)(error|warning|info):\\([^ :]+\\.sh):(\\d+)\\)$",

I missed this earlier but this regex is invalid; the last parenthesis in
unmatched.

Alban

> +                    "file": 3,
> +                    "line": 4,
> +                    "severity": 2,
> +                    "message": 1
>                  }
>              ]
>          }
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index baf94546da..d5f59ab3bf 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -682,8 +682,8 @@ file_lineno () {
>  		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;;
> +			0,t[0-9]*.sh) echo "(${1+$1:}t/${BASH_SOURCE[$i]}:$LINENO)"; return;;
> +			*,t[0-9]*.sh) echo "(${1+$1:}t/${BASH_SOURCE[$i]}:${BASH_LINENO[$(($i-1))]})"; return;;
>  			esac
>  		done
>  	'
> @@ -734,7 +734,7 @@ test_failure_ () {
>  		write_junit_xml_testcase "$1" "      $junit_insert"
>  	fi
>  	test_failure=$(($test_failure + 1))
> -	say_color error "$(file_lineno error)not ok $test_count - $1"
> +	say_color error "not ok $test_count - $1$(file_lineno error)"
>  	shift
>  	printf '%s\n' "$*" | sed -e 's/^/#	/'
>  	test "$immediate" = "" || { finalize_junit_xml; GIT_EXIT_OK=t; exit 1; }
>
Johannes Schindelin May 15, 2020, 10:42 p.m. UTC | #10
Hi,

On Fri, 15 May 2020, Junio C Hamano wrote:

> Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:
>
> > On Fri, May 15, 2020 at 09:50:38AM -0700, Junio C Hamano wrote:
> >>
> >> At this late stage in the cycle, would it be a safer change to
> >> revert the whole thing, I wonder, rather than piling fixes on top of
> >> fixes to the initial breakage?
> >>
> >> 303775a2 (t/test_lib: avoid naked bash arrays in file_lineno, 2020-05-07)
> >> 662f9cf1 (tests: when run in Bash, annotate test failures with file name/line number, 2020-04-11)
> >
> > will also need:
> >
> >   676eb0c1ce (ci: add a problem matcher for GitHub Actions, 2020-04-11)
>
> Yeah, I think that is a good idea.  I suspect that leaving it there
> won't cause problems, though---it would be just nothing is found to
> be clicked and that's the end of it, no?
>
> Will add a revert to the series anyway.

Yes, I'm fine with reverting this, as breaking TAP is a rather bad side
effect.

Ciao,
Dscho
Junio C Hamano May 15, 2020, 10:57 p.m. UTC | #11
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> >> 303775a2 (t/test_lib: avoid naked bash arrays in file_lineno, 2020-05-07)
>> >> 662f9cf1 (tests: when run in Bash, annotate test failures with file name/line number, 2020-04-11)
>> >
>> > will also need:
>> >
>> >   676eb0c1ce (ci: add a problem matcher for GitHub Actions, 2020-04-11)
>>
>> Yeah, I think that is a good idea.  I suspect that leaving it there
>> won't cause problems, though---it would be just nothing is found to
>> be clicked and that's the end of it, no?
>>
>> Will add a revert to the series anyway.
>
> Yes, I'm fine with reverting this, as breaking TAP is a rather bad side
> effect.

Yup.  We can revisit it in the next cycle to find a solution that
does not break TAP, either the "a separate comment line", "machine
readable cruft at the end of the same line", or some other approach.

Patch
diff mbox series

diff --git a/ci/git-problem-matcher.json b/ci/git-problem-matcher.json
index 506dfbd97f..e10e88bba1 100644
--- a/ci/git-problem-matcher.json
+++ b/ci/git-problem-matcher.json
@@ -4,11 +4,11 @@ 
             "owner": "git-test-suite",
             "pattern": [
                 {
-                    "regexp": "^([^ :]+\\.sh):(\\d+): (error|warning|info):\\s+(.*)$",
-                    "file": 1,
-                    "line": 2,
-                    "severity": 3,
-                    "message": 4
+                    "regexp": "^(.*)(error|warning|info):\\([^ :]+\\.sh):(\\d+)\\)$",
+                    "file": 3,
+                    "line": 4,
+                    "severity": 2,
+                    "message": 1
                 }
             ]
         }
diff --git a/t/test-lib.sh b/t/test-lib.sh
index baf94546da..d5f59ab3bf 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -682,8 +682,8 @@  file_lineno () {
 		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;;
+			0,t[0-9]*.sh) echo "(${1+$1:}t/${BASH_SOURCE[$i]}:$LINENO)"; return;;
+			*,t[0-9]*.sh) echo "(${1+$1:}t/${BASH_SOURCE[$i]}:${BASH_LINENO[$(($i-1))]})"; return;;
 			esac
 		done
 	'
@@ -734,7 +734,7 @@  test_failure_ () {
 		write_junit_xml_testcase "$1" "      $junit_insert"
 	fi
 	test_failure=$(($test_failure + 1))
-	say_color error "$(file_lineno error)not ok $test_count - $1"
+	say_color error "not ok $test_count - $1$(file_lineno error)"
 	shift
 	printf '%s\n' "$*" | sed -e 's/^/#	/'
 	test "$immediate" = "" || { finalize_junit_xml; GIT_EXIT_OK=t; exit 1; }