Re: [PATCH 11/11] t7001: move cleanup code from outside the tests into them

From: Junio C Hamano <gitster@pobox.com>
To: shubham verma <shubhunic@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 11/11] t7001: move cleanup code from outside the tests into them
Date: Fri, 25 Sep 2020 13:54:25 -0700	[thread overview]
Message-ID: <xmqqlfgxk266.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20200925170256.11490-12-shubhunic@gmail.com> (shubham verma's message of "Fri, 25 Sep 2020 22:32:56 +0530")

shubham verma <shubhunic@gmail.com> writes:

> From: Shubham Verma <shubhunic@gmail.com>
>
> Let's use test_when_finished() to include cleanup code inside the tests,
> as it's cleaner and safer to not have any code outside the tests.
>
> Signed-off-by: shubham verma <shubhunic@gmail.com>
> ---
>  t/t7001-mv.sh | 25 +++++++++++--------------
>  1 file changed, 11 insertions(+), 14 deletions(-)
>
> diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
> index 7bb4a7b759..b4d04ceaf8 100755
> --- a/t/t7001-mv.sh
> +++ b/t/t7001-mv.sh
> @@ -32,6 +32,7 @@ test_expect_success 'commiting the change' '
>  '
>  
>  test_expect_success 'checking the commit' '
> +	test_when_finished "rmdir path1" &&
>  	git diff-tree -r -M --name-status  HEAD^ HEAD >actual &&
>  	grep "^R100..*path1/COPYING..*path0/COPYING" actual
>  '

Sorry, but why in this test?  It only runs diff-tree and runs grep,
neither of which changes any state in the repository.  Because the
test does *not* create path1, and having or not having path1 on the
filesystem would not affect the outcome of the test, I do not see
how it makes sense to use test_when_finished in here.

If you are saying that path1 will no longer be used after this test
finishes, test_when_finished should be done in the test before this
one that used path1 the last, because this test does not care.  It
is probably the one that moves the file out of path1 back to path0
and records the result as a commit with title "move-in" (although if
I were writing this test today, I would merge the "move and commit"
into one step).

> @@ -43,6 +44,7 @@ test_expect_success 'mv --dry-run does not move file' '
>  '
>  
>  test_expect_success 'checking -k on non-existing file' '
> +	test_when_finished "rm -f idontexist path0/idontexist" &&
>  	git mv -k idontexist path0
>  '

I do not see the point of "rm -f idontexist" in the
post-test-cleanup at all.  Some might see that path0/ideontexist is
worth having there, in case the "mv" command gets so broken that it
creates such a file by mistake, but Personally I'd prefer to use
test_when_finished to clean up the side effects we _expect_ to
cause.  We cannot anticipate each and every breakage.

The other side of the coin is that this test DEPENDS ON the fact
that idontexist does *NOT* exist before it runs "git mv -k
idontexist path0", but nobody before us gives us any explicitly
guarantee.  This test also depends on the presence of path0
directory the same way.  Instead of relying on others that came
before us to have cleaned after themselves for us, we can more
explicitly protect ourselves by making sure the pre-condition we
depend on holds.  I.e.

    test_expect_success 'mv -k on non-exising file would not fail' '
	mkdir -p path0 &&
	rm -f idontexist path0/idontexist &&
	git mv -k idontexist path0
    '

A broken "git mv" may or may not leave path0/idontexist behind, but
as long as the tests that come after us protect themselves with the
same principle of making sure the preconditions they care about do
hold, we do not necessarily have to clean after ourselves.  Since we
expect we do not leave any side effect, I'd rather not to use
test_when_finished here.

@@ -55,6 +57,7 @@ test_expect_success 'checking -k on untracked file' '
>  
>  test_expect_success 'checking -k on multiple untracked files' '
>  	: > untracked2 &&
> +	test_when_finished "rm -f untracked2 path0/untracked2" &&
>  	git mv -k untracked1 untracked2 path0 &&
>  	test -f untracked1 &&
>  	test -f untracked2 &&

An exercise to readers.  Explain why

 - we want to move test_when_finished _before_ ">untracked2" is created;

 - "rm -f untrackd2" in test_when_finished is a good idea;

 - "rm -f path0/untracked2" is not a good idea;

 - we may want to do

	>untracked1 &&
	mkdir -p path0 &&

   before "git mv -k ..." is tested.

> -# clean up the mess in case bad things happen
> -rm -f idontexist untracked1 untracked2 \
> -     path0/idontexist path0/untracked1 path0/untracked2 \
> -     .git/index.lock
> -rmdir path1
> -
>  test_expect_success 'moving to absent target with trailing slash' '
>  	test_must_fail git mv path0/COPYING no-such-dir/ &&
>  	test_must_fail git mv path0/COPYING no-such-dir// &&

It may be a better approach to move the above removals at the
beginning of this test, just before the first test_must_fail line.

> -rm -fr papers partA path?
> -
>  test_expect_success "Sergey Vlasov's test case" '

Likewise.
