* [PATCH] test-lib-functions: make 'test_cmp_rev' more informative on failure
@ 2018-11-19 13:28 SZEDER Gábor
2018-11-19 13:31 ` SZEDER Gábor
2018-11-19 19:49 ` Jeff King
0 siblings, 2 replies; 5+ messages in thread
From: SZEDER Gábor @ 2018-11-19 13:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, SZEDER Gábor
The 'test_cmp_rev' helper is merely a wrapper around 'test_cmp'
checking the output of two 'git rev-parse' commands, which means that
its output on failure is not particularly informative, as it's
basically two OIDs with a bit of extra clutter of the diff header, but
without any indication of which two revisions have caused the failure:
--- expect.rev 2018-11-17 14:02:11.569747033 +0000
+++ actual.rev 2018-11-17 14:02:11.569747033 +0000
@@ -1 +1 @@
-d79ce1670bdcb76e6d1da2ae095e890ccb326ae9
+139b20d8e6c5b496de61f033f642d0e3dbff528d
It also pollutes the test repo with these two intermediate files,
though that doesn't seem to cause any complications in our current
tests (meaning that I couldn't find any tests that have to work around
the presence of these files by explicitly removing or ignoring them).
Enhance 'test_cmp_rev' to provide a more useful output on failure with
less clutter:
error: two revisions point to different objects:
'HEAD^': d79ce1670bdcb76e6d1da2ae095e890ccb326ae9
'extra': 139b20d8e6c5b496de61f033f642d0e3dbff528d
Doing so is more convenient when storing the OIDs outputted by 'git
rev-parse' in a local variable each, which, as a bonus, won't pollute
the repository with intermediate files.
While at it, also ensure that 'test_cmp_rev' is invoked with the right
number of parameters, namely two.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/test-lib-functions.sh | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index d158c8d0bf..fc84db67a1 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -854,9 +854,23 @@ test_must_be_empty () {
# Tests that its two parameters refer to the same revision
test_cmp_rev () {
- git rev-parse --verify "$1" >expect.rev &&
- git rev-parse --verify "$2" >actual.rev &&
- test_cmp expect.rev actual.rev
+ if test $# != 2
+ then
+ error "bug in the test script: test_cmp_rev requires two revisions, but got $#"
+ else
+ local r1 r2
+ r1=$(git rev-parse --verify "$1") &&
+ r2=$(git rev-parse --verify "$2") &&
+ if test "$r1" != "$r2"
+ then
+ cat >&4 <<-EOF
+ error: two revisions point to different objects:
+ '$1': $r1
+ '$2': $r2
+ EOF
+ return 1
+ fi
+ fi
}
# Print a sequence of integers in increasing order, either with
--
2.20.0.rc0.134.gf0022f8e60
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] test-lib-functions: make 'test_cmp_rev' more informative on failure
2018-11-19 13:28 [PATCH] test-lib-functions: make 'test_cmp_rev' more informative on failure SZEDER Gábor
@ 2018-11-19 13:31 ` SZEDER Gábor
2018-11-19 19:49 ` Jeff King
1 sibling, 0 replies; 5+ messages in thread
From: SZEDER Gábor @ 2018-11-19 13:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Mon, Nov 19, 2018 at 02:28:18PM +0100, SZEDER Gábor wrote:
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index d158c8d0bf..fc84db67a1 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -854,9 +854,23 @@ test_must_be_empty () {
>
> # Tests that its two parameters refer to the same revision
> test_cmp_rev () {
> - git rev-parse --verify "$1" >expect.rev &&
> - git rev-parse --verify "$2" >actual.rev &&
> - test_cmp expect.rev actual.rev
> + if test $# != 2
> + then
> + error "bug in the test script: test_cmp_rev requires two revisions, but got $#"
And this here is another "bug in the test script" error that should be
turned into:
BUG "test_cmp_rev requires two revisions, but got $#"
See: https://public-inbox.org/git/20181119131326.2435-1-szeder.dev@gmail.com/
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] test-lib-functions: make 'test_cmp_rev' more informative on failure
2018-11-19 13:28 [PATCH] test-lib-functions: make 'test_cmp_rev' more informative on failure SZEDER Gábor
2018-11-19 13:31 ` SZEDER Gábor
@ 2018-11-19 19:49 ` Jeff King
2018-11-20 11:25 ` SZEDER Gábor
1 sibling, 1 reply; 5+ messages in thread
From: Jeff King @ 2018-11-19 19:49 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Junio C Hamano, git
On Mon, Nov 19, 2018 at 02:28:18PM +0100, SZEDER Gábor wrote:
> The 'test_cmp_rev' helper is merely a wrapper around 'test_cmp'
> checking the output of two 'git rev-parse' commands, which means that
> its output on failure is not particularly informative, as it's
> basically two OIDs with a bit of extra clutter of the diff header, but
> without any indication of which two revisions have caused the failure:
>
> --- expect.rev 2018-11-17 14:02:11.569747033 +0000
> +++ actual.rev 2018-11-17 14:02:11.569747033 +0000
> @@ -1 +1 @@
> -d79ce1670bdcb76e6d1da2ae095e890ccb326ae9
> +139b20d8e6c5b496de61f033f642d0e3dbff528d
>
> It also pollutes the test repo with these two intermediate files,
> though that doesn't seem to cause any complications in our current
> tests (meaning that I couldn't find any tests that have to work around
> the presence of these files by explicitly removing or ignoring them).
>
> Enhance 'test_cmp_rev' to provide a more useful output on failure with
> less clutter:
>
> error: two revisions point to different objects:
> 'HEAD^': d79ce1670bdcb76e6d1da2ae095e890ccb326ae9
> 'extra': 139b20d8e6c5b496de61f033f642d0e3dbff528d
>
> Doing so is more convenient when storing the OIDs outputted by 'git
> rev-parse' in a local variable each, which, as a bonus, won't pollute
> the repository with intermediate files.
>
> While at it, also ensure that 'test_cmp_rev' is invoked with the right
> number of parameters, namely two.
This is an improvement, in my opinion (and I agree that using your new
BUG for this last part would be better still). It also saves a process
in the common case.
One question:
> + else
> + local r1 r2
> + r1=$(git rev-parse --verify "$1") &&
> + r2=$(git rev-parse --verify "$2") &&
> + if test "$r1" != "$r2"
> + then
> + cat >&4 <<-EOF
> + error: two revisions point to different objects:
> + '$1': $r1
> + '$2': $r2
> + EOF
> + return 1
> + fi
Why does this cat go to descriptor 4? I get why you'd want it to (it's
meant for the user's eyes, and that's where 4 goes), but we do not
usually bother to do so for our helper functions (like test_cmp).
I don't think it matters usually in practice, because nobody tries to
capture the stderr of test_cmp, etc. I don't think it would ever hurt,
though. Should we be doing that for all the others, too?
-Peff
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] test-lib-functions: make 'test_cmp_rev' more informative on failure
2018-11-19 19:49 ` Jeff King
@ 2018-11-20 11:25 ` SZEDER Gábor
2018-11-20 11:37 ` Jeff King
0 siblings, 1 reply; 5+ messages in thread
From: SZEDER Gábor @ 2018-11-20 11:25 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
On Mon, Nov 19, 2018 at 02:49:20PM -0500, Jeff King wrote:
> On Mon, Nov 19, 2018 at 02:28:18PM +0100, SZEDER Gábor wrote:
>
> > The 'test_cmp_rev' helper is merely a wrapper around 'test_cmp'
> > checking the output of two 'git rev-parse' commands, which means that
> > its output on failure is not particularly informative, as it's
> > basically two OIDs with a bit of extra clutter of the diff header, but
> > without any indication of which two revisions have caused the failure:
> >
> > --- expect.rev 2018-11-17 14:02:11.569747033 +0000
> > +++ actual.rev 2018-11-17 14:02:11.569747033 +0000
> > @@ -1 +1 @@
> > -d79ce1670bdcb76e6d1da2ae095e890ccb326ae9
> > +139b20d8e6c5b496de61f033f642d0e3dbff528d
> >
> > It also pollutes the test repo with these two intermediate files,
> > though that doesn't seem to cause any complications in our current
> > tests (meaning that I couldn't find any tests that have to work around
> > the presence of these files by explicitly removing or ignoring them).
> >
> > Enhance 'test_cmp_rev' to provide a more useful output on failure with
> > less clutter:
> >
> > error: two revisions point to different objects:
> > 'HEAD^': d79ce1670bdcb76e6d1da2ae095e890ccb326ae9
> > 'extra': 139b20d8e6c5b496de61f033f642d0e3dbff528d
> >
> > Doing so is more convenient when storing the OIDs outputted by 'git
> > rev-parse' in a local variable each, which, as a bonus, won't pollute
> > the repository with intermediate files.
> >
> > While at it, also ensure that 'test_cmp_rev' is invoked with the right
> > number of parameters, namely two.
>
> This is an improvement, in my opinion (and I agree that using your new
> BUG for this last part would be better still). It also saves a process
> in the common case.
But then it adds two subshells to the common case right away... I'm
not sure which is worse on Windows, where it matters the most.
> One question:
>
> > + else
> > + local r1 r2
> > + r1=$(git rev-parse --verify "$1") &&
> > + r2=$(git rev-parse --verify "$2") &&
> > + if test "$r1" != "$r2"
> > + then
> > + cat >&4 <<-EOF
> > + error: two revisions point to different objects:
> > + '$1': $r1
> > + '$2': $r2
> > + EOF
> > + return 1
> > + fi
>
> Why does this cat go to descriptor 4? I get why you'd want it to (it's
> meant for the user's eyes, and that's where 4 goes),
Exactly.
> but we do not
> usually bother to do so for our helper functions (like test_cmp).
test_i18ngrep() does since your 03aa3783f2 (t: send verbose
test-helper output to fd 4, 2018-02-22).
> I don't think it matters usually in practice, because nobody tries to
> capture the stderr of test_cmp, etc.
See 54ce2e9be9 (t9400-git-cvsserver-server: don't rely on the output
of 'test_cmp', 2018-03-08) for a fun example, I remember it caused a
spike in WTF/minutes :)
> I don't think it would ever hurt,
> though. Should we be doing that for all the others, too?
I think we should, and you agreed:
https://public-inbox.org/git/20180303065648.GA17312@sigill.intra.peff.net/
but then went travelling, and then the whole thing got forgotten.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] test-lib-functions: make 'test_cmp_rev' more informative on failure
2018-11-20 11:25 ` SZEDER Gábor
@ 2018-11-20 11:37 ` Jeff King
0 siblings, 0 replies; 5+ messages in thread
From: Jeff King @ 2018-11-20 11:37 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Junio C Hamano, git
On Tue, Nov 20, 2018 at 12:25:38PM +0100, SZEDER Gábor wrote:
> > but we do not
> > usually bother to do so for our helper functions (like test_cmp).
>
> test_i18ngrep() does since your 03aa3783f2 (t: send verbose
> test-helper output to fd 4, 2018-02-22).
Oh, indeed. Usually I find myself forgetting about patches I worked on
from 5 years ago. Forgetting about one from 9 months ago is a new low.
:)
> > I don't think it would ever hurt,
> > though. Should we be doing that for all the others, too?
>
> I think we should, and you agreed:
>
> https://public-inbox.org/git/20180303065648.GA17312@sigill.intra.peff.net/
>
> but then went travelling, and then the whole thing got forgotten.
Stupid vacation. :)
OK, yes, I reaffirm my agreement that this is the right direction, then.
Sorry for my lack of memory.
-Peff
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-11-20 11:37 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-19 13:28 [PATCH] test-lib-functions: make 'test_cmp_rev' more informative on failure SZEDER Gábor
2018-11-19 13:31 ` SZEDER Gábor
2018-11-19 19:49 ` Jeff King
2018-11-20 11:25 ` SZEDER Gábor
2018-11-20 11:37 ` Jeff King
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).