| Message ID | patch-2.2-062f34abf1a-20210720T115052Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | bundle tests: modernize, fix missing coverage & test_cmp | expand |
On Tue, Jul 20, 2021 at 01:52:09PM +0200, Ævar Arnfjörð Bjarmason wrote: > So let's use test_cmp instead, and also in the other nearby tests > where it's easy. I took a look at this patch carefully to make sure that this transformation also improved the readability, too. Looking around, I think that this was a good improvement in readability, but also hardened the tests (for the reasons that you mentioned). One tiny note below. > test_expect_success 'empty bundle file is rejected' ' > @@ -67,11 +83,33 @@ test_expect_success 'ridiculously long subject in boundary' ' > printf "%01200d\n" 0 | git commit -F - && > test_commit fifth && > git bundle create long-subject-bundle.bdl HEAD^..HEAD && > - git bundle list-heads long-subject-bundle.bdl >heads && > - test -s heads && > + cat >expect <<-EOF && > + $(git rev-parse main) HEAD > + EOF > + git bundle list-heads long-subject-bundle.bdl >actual && > + test_cmp expect actual && This is quite readable, but the assertion below gets much more complicated as a result of the change. > git fetch long-subject-bundle.bdl && > - sed -n "/^-/{p;q;}" long-subject-bundle.bdl >boundary && > - grep "^-$OID_REGEX " boundary > + > + cat >expect.common <<-EOF && > + -$(git log --pretty=format:"%H %s" -1 HEAD^) > + $(git rev-parse HEAD) HEAD > + EOF > + if test_have_prereq SHA1 > + then > + cp expect.common expect > + else > + echo @object-format=sha256 >expect > + cat expect.common >>expect > + fi && Here we're setting up expect, but I think flipping the order might make things a little easier to follow. Maybe something like this: rm expect && if ! test_have_prereq SHA1 then echo "@object-format=sha256" >expect fi && cat >>expect <<-EOF && -$(git log --pretty=format:"%H %s" -1 HEAD^) $(git rev-parse HEAD) HEAD EOF && Or, if you wanted to go further, you could do something like: cat >expect <<-EOF $(test_have_prereq SHA1 || echo "@object-format=sha256") -$(git log --pretty=format:"%H %s" -1 HEAD^) $(git rev-parse HEAD) HEAD EOF which is arguably a little tighter (although I find the echo-in-a-heredoc thing to be kind of ugly). > + if test_have_prereq SHA1 > + then > + head -n 3 long-subject-bundle.bdl >bundle-header > + else > + head -n 4 long-subject-bundle.bdl >bundle-header > + fi && > + grep -v "^#" bundle-header >actual && Here I would suggest getting rid of the bundle-header intermediary and instead writing: if test_have_prereq SHA1 then head -n 3 long-subject-bundle.bdl else head -n 4 long-subject-bundle.bdl fi | grep -v "^#" >actual and then having your > + test_cmp expect actual below. Thanks, Taylor
On Tue, Jul 20 2021, Taylor Blau wrote: > On Tue, Jul 20, 2021 at 01:52:09PM +0200, Ævar Arnfjörð Bjarmason wrote: >> So let's use test_cmp instead, and also in the other nearby tests >> where it's easy. > > I took a look at this patch carefully to make sure that this > transformation also improved the readability, too. > > Looking around, I think that this was a good improvement in readability, > but also hardened the tests (for the reasons that you mentioned). One > tiny note below. > >> test_expect_success 'empty bundle file is rejected' ' >> @@ -67,11 +83,33 @@ test_expect_success 'ridiculously long subject in boundary' ' >> printf "%01200d\n" 0 | git commit -F - && >> test_commit fifth && >> git bundle create long-subject-bundle.bdl HEAD^..HEAD && >> - git bundle list-heads long-subject-bundle.bdl >heads && >> - test -s heads && >> + cat >expect <<-EOF && >> + $(git rev-parse main) HEAD >> + EOF >> + git bundle list-heads long-subject-bundle.bdl >actual && >> + test_cmp expect actual && > > This is quite readable, but the assertion below gets much more > complicated as a result of the change. > >> git fetch long-subject-bundle.bdl && >> - sed -n "/^-/{p;q;}" long-subject-bundle.bdl >boundary && >> - grep "^-$OID_REGEX " boundary >> + >> + cat >expect.common <<-EOF && >> + -$(git log --pretty=format:"%H %s" -1 HEAD^) >> + $(git rev-parse HEAD) HEAD >> + EOF >> + if test_have_prereq SHA1 >> + then >> + cp expect.common expect >> + else >> + echo @object-format=sha256 >expect >> + cat expect.common >>expect >> + fi && > > Here we're setting up expect, but I think flipping the order might make > things a little easier to follow. Maybe something like this: > > rm expect && > if ! test_have_prereq SHA1 > then > echo "@object-format=sha256" >expect > fi && > cat >>expect <<-EOF && > -$(git log --pretty=format:"%H %s" -1 HEAD^) > $(git rev-parse HEAD) HEAD > EOF && Thanks, I used pretty much that as-is for v2. > Or, if you wanted to go further, you could do something like: > > cat >expect <<-EOF > $(test_have_prereq SHA1 || echo "@object-format=sha256") > -$(git log --pretty=format:"%H %s" -1 HEAD^) > $(git rev-parse HEAD) HEAD > EOF > > which is arguably a little tighter (although I find the > echo-in-a-heredoc thing to be kind of ugly). This one won't work because you'll have an empty line at the start under SHA-1. >> + if test_have_prereq SHA1 >> + then >> + head -n 3 long-subject-bundle.bdl >bundle-header >> + else >> + head -n 4 long-subject-bundle.bdl >bundle-header >> + fi && >> + grep -v "^#" bundle-header >actual && > > Here I would suggest getting rid of the bundle-header intermediary and > instead writing: > > if test_have_prereq SHA1 > then > head -n 3 long-subject-bundle.bdl > else > head -n 4 long-subject-bundle.bdl > fi | grep -v "^#" >actual > > and then having your Thanks, used that.
On Thu, Jul 22, 2021 at 01:53:54AM +0200, Ævar Arnfjörð Bjarmason wrote: > > Or, if you wanted to go further, you could do something like: > > > > cat >expect <<-EOF > > $(test_have_prereq SHA1 || echo "@object-format=sha256") > > -$(git log --pretty=format:"%H %s" -1 HEAD^) > > $(git rev-parse HEAD) HEAD > > EOF > > > > which is arguably a little tighter (although I find the > > echo-in-a-heredoc thing to be kind of ugly). > > This one won't work because you'll have an empty line at the start under > SHA-1. Ah, you're totally right: good catch. I think it's avoidable by smashing the first two lines into one. If the subshell prints nothing, then the first line will start with "-$(git log ...)", otherwise, if it does print something, then the echo will print a newline to separate it from the "-$(git log ...) output which will go on the second line. But that is definitely uglier than my first suggestion, so I'm glad that you didn't try and make what I wrote above work :). Thanks, Taylor
diff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh index c9323a08fe8..a7f18407e32 100755 --- a/t/t5607-clone-bundle.sh +++ b/t/t5607-clone-bundle.sh @@ -29,11 +29,21 @@ test_expect_success '"verify" needs a worktree' ' test_expect_success 'annotated tags can be excluded by rev-list options' ' git bundle create bundle --all --since=7.Apr.2005.15:14:00.-0700 && - git ls-remote bundle > output && - grep tag output && + cat >expect <<-EOF && + $(git rev-parse HEAD) HEAD + $(git rev-parse tag) refs/tags/tag + $(git rev-parse main) refs/heads/main + EOF + git ls-remote bundle >actual && + test_cmp expect actual && + git bundle create bundle --all --since=7.Apr.2005.15:16:00.-0700 && - git ls-remote bundle > output && - ! grep tag output + cat >expect <<-EOF && + $(git rev-parse HEAD) HEAD + $(git rev-parse main) refs/heads/main + EOF + git ls-remote bundle >actual && + test_cmp expect actual ' test_expect_success 'die if bundle file cannot be created' ' @@ -43,14 +53,20 @@ test_expect_success 'die if bundle file cannot be created' ' test_expect_success 'bundle --stdin' ' echo main | git bundle create stdin-bundle.bdl --stdin && - git ls-remote stdin-bundle.bdl >output && - grep main output + cat >expect <<-EOF && + $(git rev-parse main) refs/heads/main + EOF + git ls-remote stdin-bundle.bdl >actual && + test_cmp expect actual ' test_expect_success 'bundle --stdin <rev-list options>' ' echo main | git bundle create hybrid-bundle.bdl --stdin tag && - git ls-remote hybrid-bundle.bdl >output && - grep main output + cat >expect <<-EOF && + $(git rev-parse main) refs/heads/main + EOF + git ls-remote stdin-bundle.bdl >actual && + test_cmp expect actual ' test_expect_success 'empty bundle file is rejected' ' @@ -67,11 +83,33 @@ test_expect_success 'ridiculously long subject in boundary' ' printf "%01200d\n" 0 | git commit -F - && test_commit fifth && git bundle create long-subject-bundle.bdl HEAD^..HEAD && - git bundle list-heads long-subject-bundle.bdl >heads && - test -s heads && + cat >expect <<-EOF && + $(git rev-parse main) HEAD + EOF + git bundle list-heads long-subject-bundle.bdl >actual && + test_cmp expect actual && + git fetch long-subject-bundle.bdl && - sed -n "/^-/{p;q;}" long-subject-bundle.bdl >boundary && - grep "^-$OID_REGEX " boundary + + cat >expect.common <<-EOF && + -$(git log --pretty=format:"%H %s" -1 HEAD^) + $(git rev-parse HEAD) HEAD + EOF + if test_have_prereq SHA1 + then + cp expect.common expect + else + echo @object-format=sha256 >expect + cat expect.common >>expect + fi && + if test_have_prereq SHA1 + then + head -n 3 long-subject-bundle.bdl >bundle-header + else + head -n 4 long-subject-bundle.bdl >bundle-header + fi && + grep -v "^#" bundle-header >actual && + test_cmp expect actual ' test_expect_success 'prerequisites with an empty commit message' ' @@ -103,7 +141,11 @@ test_expect_success 'fetch SHA-1 from bundle' ' test_expect_success 'git bundle uses expected default format' ' git bundle create bundle HEAD^.. && - head -n1 bundle | grep "^# v$(test_oid version) git bundle$" + cat >expect <<-EOF && + # v$(test_oid version) git bundle + EOF + head -n1 bundle >actual && + test_cmp expect actual ' test_expect_success 'git bundle v3 has expected contents' '
Change the bundle tests to fully compare the expected "git ls-remote" or "git bundle list-heads" output, instead of merely grepping it. This avoids subtle regressions in the tests. In f62e0a39b6 (t5704 (bundle): add tests for bundle --stdin, 2010-04-19) the "bundle --stdin <rev-list options>" test was added to make sure we didn't include the tag. But since the --stdin mode didn't work until 5bb0fd2cab (bundle: arguments can be read from stdin, 2021-01-11) our grepping of "master" (later "main") missed the important part of the test. Namely that we should not include the "refs/tags/tag" tag in that case. Since the test only grepped for "main" in the output we'd miss a regression in that code. So let's use test_cmp instead, and also in the other nearby tests where it's easy. This does make things a bit more verbose in the case of the test that's checking the bundle header, since it's different under SHA1 and SHA256. I think this makes test easier to follow. I've got some WIP changes to extend the "git bundle" command to dump parts of the header out, which are easier to understand if we test the output explicitly like this. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/t5607-clone-bundle.sh | 68 +++++++++++++++++++++++++++++++++-------- 1 file changed, 55 insertions(+), 13 deletions(-)