Re: [RFC PATCH 2/6] t4041, t4060: modernize test style

[Calvin Wan <calvinwan@google.com>]

> > -test_create_repo sm1 &&
> > -add_file . foo >/dev/null
> > -
> > -head1=$(add_file sm1 foo1 foo2)
> > -fullhead1=$(cd sm1; git rev-parse --verify HEAD)
> > +test_expect_success 'setup' '
> > +     test_create_repo sm1 &&
> > +     add_file . foo >/dev/null &&
>
> Now this is inside test_expect_success, redirection to /dev/null is
> unnecessary.

ack.

>
> > +     head1=$(add_file sm1 foo1 foo2) &&
> > +     fullhead1=$(cd sm1 && git rev-parse --verify HEAD)
> > +'
>
> Or "fullhead1=$(git -C sm1 rev-parse ...)".
>
> Both of the above can be ignored if we are trying to be a strict
> rewrite of the original, but moving code inside test_expect_success
> block is a large enough change that there may not be much point in
> avoiding such an obvious modernization "while at it".

I agree, will fix.

>
> > -rm sm2
> > -mv sm2-bak sm2
> > -
> >  test_expect_success 'setup nested submodule' '
> > +     rm sm2 &&
> > +     mv sm2-bak sm2 &&
>
> To me, this looks more like something test_when_finished in the test
> that wanted not to have sm2 (i.e. "deleted submodule with .git file")
> should have done as part of its own clean-up.
>
> There certainly can be two schools of thought when it comes to how to
> arrange the precondition of subsequent tests.  More modern tests tend
> to clean after themselves by reverting the damage they made to the
> environment inside test_when_finished in themselves.  This one, as
> the posted patch does, goes to the other extreme and forces the
> subsequent test to undo the damage done by the previous ones.
>
> The latter approach has two major downsides.  It would not work if
> the tester wants to skip an earlier step, or if an earlier step
> failed to cause the expected damage this step wants to undo.  The
> correctness of "what we should see as sm2 here must be in sm2-bak
> because we know an earlier step should have moved it there" can
> easily be broken.  It also makes it harder to update the earlier
> step to cause different damage to the environment---the "undoing the
> damage done by the previous step(s)" done as early parts of this
> step also needs to be updated.
>
> Whichever approach we pick to use in each script, it would be better
> to stick to one philosophy, and if we can make each step revert the
> damage it caused when it is done, that would be nice.
>
> > -mv sm2-bak sm2
> > +test_expect_success 'submodule cleanup' '
> > +     mv sm2-bak sm2
> > +'
> >
> >  test_done
>
> Likewise.

ack. Swapping to test_when_finished