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

[Junio C Hamano <gitster@pobox.com>]

Calvin Wan <calvinwan@google.com> writes:

> From: Josh Steadmon <steadmon@google.com>
>
> 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.

Nice.

> -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.

> +	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".


> -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.

Thanks.