| Message ID | 20230628181753.10384-1-five231003@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | t4205: correctly test %(describe:abbrev=...) | expand |
Kousik Sanagavarapu <five231003@gmail.com> writes: > The pretty format %(describe:abbrev=<number>) tells describe to use only > <number> characters of the oid to generate the human-readable format of > the commit-ish. Is that *only* correct? I thought it was "at least <number> hexdigits" to allow for future growth of the project. > This is not apparent in the test for %(describe:abbrev=...) because we > directly tag HEAD and use that, in which case the human-readable format > is just the tag name. So, create a new commit and use that instead. Nice. How was this found, I have to wonder, and more importantly, how would we have written this test in the first place to avoid testing "the wrong thing", to learn from this experience? > test_expect_success '%(describe:abbrev=...) vs git describe --abbrev=...' ' > - test_when_finished "git tag -d tagname" && > - git tag -a -m tagged tagname && > + test_commit --no-tag file && > git describe --abbrev=15 >expect && > git log -1 --format="%(describe:abbrev=15)" >actual && > test_cmp expect actual The current test checks that the output in the case where the number of commits since the tag is 0, and "describe --abbrev=15" and "log --format='%(describe:abbrev=15)'" give exactly the same result. Which is a good thing to test. But we *also* want to test a more typical case where there are commits between HEAD and the tag that is used to describe it. And we *also* want to make sure that the hexadecimal object name suffix used in the description is at least 15 hexdigits long, if not more. The updated test drops test #1 (which is questionable), adds test #2 (which is good), and still omits test #3 (which is not so good). So, perhaps test_when_finished "git tag -d tagname" && - git tag -a -m tagged tagname && test_commit --no-tag file && git describe --abbrev=15 >expect && git log -1 --format="%(describe:abbrev=15)" >actual && test_cmp expect actual && + sed -e "s/^.*-g\([0-9a-f]*\)$/\1/" <actual >hexpart && + test 16 -le $(wc -c <hexpart) && + + git tag -a -m tagged tagname && + git describe --abbrev=15 >expect && + git log -1 --format="%(describe:abbrev=15)" >actual && + test_cmp expect actual && + test tagname = $(cat actual) or something along the line? First we test with a commit that is not tagged at all to have some commits between the tag and HEAD with the original comparison (this is for #2), then we make sure the length of the hexpart (new---this is for #3), and then we add the tag to see the "exact" case also works (this is for #1). Thanks.
On Wed, Jun 28, 2023 at 02:30:18PM -0700, Junio C Hamano wrote: > Kousik Sanagavarapu <five231003@gmail.com> writes: > > > The pretty format %(describe:abbrev=<number>) tells describe to use only > > <number> characters of the oid to generate the human-readable format of > > the commit-ish. > > Is that *only* correct? I thought it was "at least <number> hexdigits" > to allow for future growth of the project. Yeah, this is wrong. It should be "at least" for being more precise as we may need more than <number> in some cases. Will change that. Thanks for catching it. > > This is not apparent in the test for %(describe:abbrev=...) because we > > directly tag HEAD and use that, in which case the human-readable format > > is just the tag name. So, create a new commit and use that instead. > > Nice. How was this found, I have to wonder, and more importantly, I was working on duplicating "%(describe)" from pretty, in ref-filter and was writing tests for it. While going through the trash directory for some other breakage, I found this. So it was kind of a chance. > how would we have written this test in the first place to avoid > testing "the wrong thing", to learn from this experience? I don't have a clue :). In this particular test, this is not "the wrong thing" anyways, as you explain below. We just fail to test it wholly (we missed some cases). > > test_expect_success '%(describe:abbrev=...) vs git describe --abbrev=...' ' > > - test_when_finished "git tag -d tagname" && > > - git tag -a -m tagged tagname && > > + test_commit --no-tag file && > > git describe --abbrev=15 >expect && > > git log -1 --format="%(describe:abbrev=15)" >actual && > > test_cmp expect actual > > The current test checks that the output in the case where the number > of commits since the tag is 0, and "describe --abbrev=15" and "log > --format='%(describe:abbrev=15)'" give exactly the same result. > Which is a good thing to test. > > But we *also* want to test a more typical case where there are > commits between HEAD and the tag that is used to describe it. > > And we *also* want to make sure that the hexadecimal object name > suffix used in the description is at least 15 hexdigits long, if not > more. > > The updated test drops test #1 (which is questionable), adds test #2 > (which is good), and still omits test #3 (which is not so good). > > So, perhaps > > test_when_finished "git tag -d tagname" && > - git tag -a -m tagged tagname && > test_commit --no-tag file && > git describe --abbrev=15 >expect && > git log -1 --format="%(describe:abbrev=15)" >actual && > test_cmp expect actual && > + sed -e "s/^.*-g\([0-9a-f]*\)$/\1/" <actual >hexpart && > + test 16 -le $(wc -c <hexpart) && > + > + git tag -a -m tagged tagname && > + git describe --abbrev=15 >expect && > + git log -1 --format="%(describe:abbrev=15)" >actual && > + test_cmp expect actual && > + test tagname = $(cat actual) > > or something along the line? First we test with a commit that is > not tagged at all to have some commits between the tag and HEAD with > the original comparison (this is for #2), then we make sure the > length of the hexpart (new---this is for #3), and then we add the > tag to see the "exact" case also works (this is for #1). Yeah, makes sense. Thanks for such a nice explanation. I have applied this and it works. I'll reroll with this change and also the change in the log message (and also maybe add some comments about these cases). I'll make sure to do this in the tests for ref-filter too, about which I mentioned above. Thanks
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index 4cf8a77667..b631b5a142 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -1011,8 +1011,7 @@ test_expect_success '%(describe:tags) vs git describe --tags' ' ' test_expect_success '%(describe:abbrev=...) vs git describe --abbrev=...' ' - test_when_finished "git tag -d tagname" && - git tag -a -m tagged tagname && + test_commit --no-tag file && git describe --abbrev=15 >expect && git log -1 --format="%(describe:abbrev=15)" >actual && test_cmp expect actual
The pretty format %(describe:abbrev=<number>) tells describe to use only <number> characters of the oid to generate the human-readable format of the commit-ish. This is not apparent in the test for %(describe:abbrev=...) because we directly tag HEAD and use that, in which case the human-readable format is just the tag name. So, create a new commit and use that instead. Mentored-by: Christian Couder <christian.couder@gmail.com> Mentored-by: Hariom Verma <hariom18599@gmail.com> Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com> --- t/t4205-log-pretty-formats.sh | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)