Re: [PATCH 1/6] fetch: increase test coverage of fetches

From: Patrick Steinhardt <ps@pks.im>
Date: Feb 17, 2022, 11:13 UTC
Message-ID: <Yg4tyssgW9OAu6K/@ncase>
In-Reply-To: <CAP8UFD2HswmXhqYTAMxQ0iYFtsErMS=DB18iv52Ujs=cAW9ytw@mail.gmail.com>

On Tue, Feb 15, 2022 at 07:19:19AM +0100, Christian Couder wrote:

Show 11 quoted lines

> On Fri, Feb 11, 2022 at 8:38 PM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > The `--atomic` flag is missing test coverage for pruning of deleted
> > references and backfilling of tags, and of course both aren't covered
> > correctly by this flag.
> 
> It's not clear to me what "both aren't covered correctly by this flag"
> actually means here. If it means that pruning of deleted references
> and backfilling of tags don't work correctly when --atomic is used,
> then it could be stated more clearly. Otherwise this seems to just be
> repeating the first part of the sentence.

Yeah, the commit message was a bit lazy to be honest. I've reworded it to hopefully make clearer what one is looking at.

Show 50 quoted lines

> > Furthermore, we don't have tests demonstrating
> > error cases for backfilling tags.
> >
> > Add tests to cover those testing gaps.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> 
> > +test_expect_success 'atomic fetch with failing backfill' '
> > +       git init clone3 &&
> > +
> > +       # We want to test whether a failure when backfilling tags correctly
> > +       # aborts the complete transaction when `--atomic` is passed: we should
> > +       # neither create the branch nor should we create the tag when either
> > +       # one of both fails to update correctly.
> > +       #
> > +       # To trigger failure we simply abort when backfilling a tag.
> > +       write_script clone3/.git/hooks/reference-transaction <<-\EOF &&
> > +               #!/bin/sh
> > +
> > +               while read oldrev newrev reference
> > +               do
> > +                       if test "$reference" = refs/tags/tag1
> > +                       then
> > +                               exit 1
> > +                       fi
> > +               done
> > +       EOF
> > +
> > +       test_must_fail git -C clone3 fetch --atomic .. $B:refs/heads/something &&
> > +
> > +       # Creation of the tag has failed, so ideally refs/heads/something
> > +       # should not exist. The fact that it does is demonstrates that there is
> 
> s/The fact that it does is demonstrates/The fact that it does demonstrates/
> 
> > +       # missing coverage in the `--atomic` flag.
> 
> Maybe s/missing coverage/a bug/ would make things clearer.
> 
> > +       test $B = "$(git -C clone3 rev-parse --verify refs/heads/something)"
> > +'
> 
> As this patch series is about fixing buggy parts of the behavior with
> --atomic, I think it would make more sense to use test_expect_failure,
> instead of test_expect_success, in this test, and to check that we
> have the correct behavior, instead of checking that we have the buggy
> behavior.
> 
> Of course when later in this patch series the buggy behavior is fixed,
> then test_expect_failure should be replaced with test_expect_success.

The downside of using `test_expect_failure` is that one cannot easily see what exactly is broken in the testcase. Yes, you can document it, but when using `test_expect_success` the huge advantage is that you can see exactly what behaviour is changing in subsequent commits by just having a look at the diff of the test which adapts it from its initially-broken state to the newly-fixed behaviour.

Show 24 quoted lines

> > +test_expect_success 'atomic fetch with backfill should use single transaction' '
> > +       git init clone4 &&
> > +
> > +       # Fetching with the `--atomic` flag should update all references in a
> > +       # single transaction, including backfilled tags. We thus expect to see
> > +       # a single reference transaction for the created branch and tags.
> > +       cat >expected <<-EOF &&
> > +               prepared
> > +               $ZERO_OID $B refs/heads/something
> > +               $ZERO_OID $S refs/tags/tag2
> > +               committed
> > +               $ZERO_OID $B refs/heads/something
> > +               $ZERO_OID $S refs/tags/tag2
> > +               prepared
> > +               $ZERO_OID $T refs/tags/tag1
> > +               committed
> > +               $ZERO_OID $T refs/tags/tag1
> > +       EOF
> 
> The comment says that we expect to see a single reference transaction,
> but the expected file we create seems to show 2 transactions. So I
> think here too, we should use test_expect_failure, instead of
> test_expect_success, and check that we have the correct behavior
> instead of a buggy one.

Same comment as above. I've also amended the commit message to say why we're introducing the tests like this.

Show 5 quoted lines

> > +       write_script clone4/.git/hooks/reference-transaction <<-\EOF &&
> 
> Here there is no #!/bin/sh while other uses of write_script in your
> patch have it. If it's not necessary, it could be removed in the other
> uses.

Good point, I always forget that the shebang is added automatically by this helper.

Show 10 quoted lines

> > +               ( echo "$*" && cat ) >>actual
> > +       EOF
> > +
> > +       git -C clone4 fetch --atomic .. $B:refs/heads/something &&
> > +       test_cmp expected clone4/actual
> > +'
> 
> I took a quick look at the 2 other tests after this one, and I think
> test_expect_failure should be used there too, instead of
> test_expect_success.

Patrick