Re: [PATCH v2 1/6] t4041, t4060: modernize test style

[Calvin Wan <calvinwan@google.com>]

On Mon, Mar 6, 2023 at 11:32 AM Glen Choo <chooglen@google.com> wrote:
>
> Calvin Wan <calvinwan@google.com> writes:
>
> > In preparation for later changes, move setup code into test_expect
> > blocks. Smaller sections are moved into existing blocks, while larger
> > sections with a standalone purpose are given their own new blocks.
>
> The changes where we moved lines outside of blocks into blocks without
> changing them look good to me.
>
> > While at it, have tests clean up after themselves with
> > test_when_finished
>
> I believe this came about as part of the discussion in
>
>   https://lore.kernel.org/git/xmqqedqtbbf4.fsf@gitster.g
>
> I think it's good to have tests clean up after themselves, but I'm not
> sure if that's what we're doing in all of these cases, see below.
>
> I'm leaving the diff header in place, since the two files have very
> confusingly similar tests.
>
> > diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh
> >  test_expect_success 'typechanged submodule(submodule->blob)' '
> > +     test_when_finished rm -rf sm1 &&
> >       git diff --submodule=log >actual &&
> >       cat >expected <<-EOF &&
> >       diff --git a/sm1 b/sm1
>
> This hunk and the next...
>
> > @@ -212,9 +215,9 @@ test_expect_success 'typechanged submodule(submodule->blob)' '
> >       test_cmp expected actual
> >  '
> >
> > -rm -rf sm1 &&
> > -git checkout-index sm1
> >  test_expect_success 'typechanged submodule(submodule->blob)' '
> > +     test_when_finished rm -f sm1 &&
> > +     git checkout-index sm1 &&
> >       git diff-index -p --submodule=log HEAD >actual &&
> >       cat >expected <<-EOF &&
> >       Submodule sm1 $head4...0000000 (submodule deleted)
>
> were changed so that the "rm -rf" happens in the clean up phase of the
> earlier test (test 14) instead of set up phase of the later test (test
> 15). But, the "rm -rf" actually results in a _different_ state from
> before 14, so it isn't actually cleaning up, it really is preparation
> for 15's git checkout-index.
>
> You can observe this by running
>
>   ./t4041-diff-submodule-option.sh --run=1-13,15
>
> which fails as expected. On the other hand, it passes if we move the "rm
> -rf" into test 15.
>
> Nearly all of the other test_when_finished here have the same problem,
> where they 'clean up' state that wasn't changed in the same test body. I
> believe they will show similar dependency issues, though I didn't go
> through and test them all.

Good catch. I'll go thru the rest of them and remove the dependency
issues.

>
> > @@ -643,7 +643,6 @@ test_expect_success 'modified submodule contains modified content' '
> >       diff_cmp expected actual
> >  '
> >
> > -rm -rf sm1
> >  test_expect_success 'deleted submodule' '
> >       git diff-index -p --submodule=diff HEAD >actual &&
> >       cat >expected <<-EOF &&
>
> This one is fairly obvious, since the test says 'deleted submodule', but
> we no longer delete the submodule in the setup.
>
> > @@ -779,9 +780,8 @@ test_expect_success 'diff --submodule=diff with .git file' '
> >       diff_cmp expected actual
> >  '
> >
> > -mv sm2 sm2-bak
> > -
> >  test_expect_success 'deleted submodule with .git file' '
> > +     mv sm2 sm2-bak &&
> >       git diff-index -p --submodule=diff HEAD >actual &&
> >       cat >expected <<-EOF &&
> >       Submodule sm1 $head7...0000000 (submodule deleted)
> > @@ -804,9 +804,9 @@ test_expect_success 'deleted submodule with .git file' '
> >       diff_cmp expected actual
> >  '
> >
> > -echo submodule-to-blob>sm2
> > -
> >  test_expect_success 'typechanged(submodule->blob) submodule with .git file' '
> > +     test_when_finished "rm sm2 && mv sm2-bak sm2" &&
> > +     echo submodule-to-blob>sm2 &&
> >       git diff-index -p --submodule=diff HEAD >actual &&
> >       cat >expected <<-EOF &&
> >       Submodule sm1 $head7...0000000 (submodule deleted)
> > @@ -836,9 +836,6 @@ test_expect_success 'typechanged(submodule->blob) submodule with .git file' '
> >       diff_cmp expected actual
> >  '
> >
> > -rm sm2
> > -mv sm2-bak sm2
>
> This is the original case that Junio flagged, which I think is an almost
> correct use of test_when_finished, since we do get back to an earlier
> state before this string of tests, but not to the state before the
> actual test with the test_when_finished.
>
> If we want to use test_when_finished here (which I think we do), we
> should add another test_when_finished to remove the dependency between
> the two tests. like so:
>
>   test_expect_success 'deleted submodule with .git file' '
>   +     test_when_finished "mv sm2-bak sm2" &&
>         mv sm2 sm2-bak &&
>     git diff-index -p --submodule=diff HEAD >actual &&
>
> ...
>
>  test_expect_success 'typechanged(submodule->blob) submodule with .git file' '
>         test_when_finished "rm sm2 && mv sm2-bak sm2" &&
> + mv sm2 sm2-bak &&
>
> Currently, they're still dependent because one creates sm2-bak and the
> other moves it back, but if we have each test restore sm2, there will be
> no more dependency.
>

That all makes sense. Thanks for the recommendations