threads / patch / 49858

patchtest-lib-functions: make 'test_cmp_rev' more informative on failure

Subject: [PATCH] test-lib-functions: make 'test_cmp_rev' more informative on failure

## tl;dr

5 messages between Nov 19, 2018 and Nov 20, 2018. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

SZEDER Gábor· Nov 19, 2018, 13:28 UTC · lore

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(-)
Show changes to t/test-lib-functions.sh +17 −3
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
SZEDER Gábor· Nov 19, 2018, 13:31 UTC · re: SZEDER Gábor · lore

Re: [PATCH] test-lib-functions: make 'test_cmp_rev' more informative on failure

On Mon, Nov 19, 2018 at 02:28:18PM +0100, SZEDER Gábor wrote:
Show 14 quoted lines
> 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/
Jeff King· Nov 19, 2018, 19:49 UTC · re: SZEDER Gábor · lore

Re: [PATCH] test-lib-functions: make 'test_cmp_rev' more informative on failure

On Mon, Nov 19, 2018 at 02:28:18PM +0100, SZEDER Gábor wrote:
Show 30 quoted lines
> 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:
Show 13 quoted lines
> +	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
SZEDER Gábor· Nov 20, 2018, 11:25 UTC · re: Jeff King · lore

Re: [PATCH] test-lib-functions: make 'test_cmp_rev' more informative on failure

On Mon, Nov 19, 2018 at 02:49:20PM -0500, Jeff King wrote:
Show 36 quoted lines
> 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.

Show 18 quoted lines
> 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.
Jeff King· Nov 20, 2018, 11:37 UTC · re: SZEDER Gábor · lore

Re: [PATCH] test-lib-functions: make 'test_cmp_rev' more informative on failure

On Tue, Nov 20, 2018 at 12:25:38PM +0100, SZEDER Gábor wrote:
Show 5 quoted lines
> > 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. :)

Show 8 quoted lines
> > 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

← back to recent threads