test-lib-functions: make 'test_cmp_rev' more informative on failure
PATCH 1/1 test-lib-functions: make 'test_cmp_rev' more informative on failure
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': 139b20d8e6c5b496de61f033f642d0e3dbff528dDoing 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.
--- t/test-lib-functions.sh | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)
t/test-lib-functions.sh +17 -3
@@ -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
Signature · 1 line
2.20.0.rc0.134.gf0022f8e60
Quoted 5 lines · SZEDER Gábor
t/test-lib-functions.sh +0
-0
t/test-lib-functions.sh test_must_be_empty () { · line 859
Quoted 9 lines
t/test-lib-functions.sh +3
-3@@ -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 $#"
Quoted 24 lines · 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
Quoted 6 lines
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:
t/test-lib-functions.sh test_must_be_empty () { · line 872
Quoted 13 lines
t/test-lib-functions.sh +13
-0+ 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
Quoted 30 lines · Jeff King
Quoted 28 lines · 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': 139b20d8e6c5b496de61f033f642d0e3dbff528dDoing 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.
Quoted 6 lines
Quoted 2 lines
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.
Quoted 18 lines
One question:
t/test-lib-functions.sh test_must_be_empty () { · line 872
Quoted 13 lines
t/test-lib-functions.sh +13
-0+ 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 + fiWhy 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.
Quoted 2 lines
but we do not
usually bother to do so for our helper functions (liketest_cmp).
test_i18ngrep() does since your 03aa3783f2 (t: send verbose test-helper output to fd 4, 2018-02-22).
Quoted 2 lines
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 :)
Quoted 2 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/[email protected]/
but then went travelling, and then the whole thing got forgotten.
Quoted 5 lines · SZEDER Gábor
Quoted 2 lines
but we do not
usually bother to do so for our helper functions (liketest_cmp).
test_i18ngrep()does since your03aa3783f2(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. :)
Quoted 8 lines
Quoted 2 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/[email protected]/
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