| Message ID | 20200515150041.22873-1-carenas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] t: move metadata into TAP test description | expand |
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.
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
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
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; }
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.
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
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
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.
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; } >
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
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.
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; }