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

shubham verma <shubhunic@gmail.com> writes:

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

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.