| Message ID | 20181119132818.3116-1-szeder.dev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | test-lib-functions: make 'test_cmp_rev' more informative on failure | expand |
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/
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
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.
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
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
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(-)