diff mbox series

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

Message ID 20200925170256.11490-12-shubhunic@gmail.com
State New
Headers show
Series Modernizing the t7001 test script | expand

Commit Message

Shubham Verma Sept. 25, 2020, 5:02 p.m. UTC
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(-)

Comments

Eric Sunshine Sept. 25, 2020, 7:36 p.m. UTC | #1
On Fri, Sep 25, 2020 at 1:03 PM shubham verma <shubhunic@gmail.com> wrote:
> 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>
> ---
> diff --git 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
>  '

This one doesn't really pass the smell test. `path1` was created by
the "prepare reference tree" test; it is not created by this test, so
it's not really this test's responsibility to clean it up. But it also
can't be cleaned up by "prepare reference tree" since that is just a
"setup" test, and `path` is used by subsequent tests.

Does anything actually fail if directory `path1` is not removed? I ask
because slightly below the point at which `path1` is removed (outside
of any tests) a different test goes ahead and re-creates `path1`. If
it turns out that removal of `path1` isn't actually necessary, then a
better option might be simply to drop the global `rmdir path1`
altogether, along with the subsequent `mkdir path1` which comes a bit
later.

> @@ -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
>  '

The paths being removed here shouldn't actually be created by this
test, but if Git is somehow buggy and they do get created, then this
is the appropriate place to clean them up. Good.

> @@ -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 &&
> @@ -64,18 +67,14 @@ test_expect_success 'checking -k on multiple untracked files' '
>  test_expect_success 'checking -f on untracked file with existing target' '
>         : > path0/untracked1 &&
> +       test_when_finished "rm -f untracked1 path0/untracked1" &&
> +       test_when_finished "rm -f .git/index.lock" &&
>         test_must_fail git mv -f untracked1 path0 &&
>         test ! -f .git/index.lock &&
>         test -f untracked1 &&
>         test -f path0/untracked1
>  '

This is a big ugly. `untracked1` gets created by an earlier test but
is then cleaned up by this subsequent test. That goes against the idea
of test_when_finished(), which is that tests should clean up after
themselves. Doing it this way also creates a smelly dependency between
the tests. What I would recommend instead is having each test
independently create and cleanup the "untracked" files it needs. This
makes the tests a tiny bit more verbose but makes it much clearer to
the reader that the tests are independent.

> -# 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
> @@ -149,10 +148,12 @@ test_expect_success 'do not move directory over existing directory' '
>  test_expect_success 'move into "."' '
> +       test_when_finished "rm -fr path?" &&
>         git mv path1/path2/ .
>  '

This is another of those cases which doesn't really pass the smell
test. This may indeed be the final test in which the various `path?`
subdirectories are used, but it isn't the test which created them,
thus it isn't "cleaning up after itself".

If the test which might get tripped up by these `path?` directories is
the "Sergey Vlasov's test case" test, then it probably would make more
sense for _that_ test to do `rm -fr path?` as its very first step (not
as a test_when_finished()) in order to prepare things the way it needs
them (just as it already does `rm -fr .git`).

>  test_expect_success "Michael Cassar's test case" '
> +       test_when_finished "rm -fr papers partA" &&
>         rm -fr .git papers partA &&
>         git init &&
>         mkdir -p papers/unsorted papers/all-papers partA &&

Cleaning these paths here makes sense since they are created and only
used by this test.

> @@ -168,8 +169,6 @@ test_expect_success "Michael Cassar's test case" '
> -rm -fr papers partA path?
> -
>  test_expect_success "Sergey Vlasov's test case" '
>         rm -fr .git &&
>         git init &&

So, given what I said above, the first line of this test might become:

    rm -fr .git path? &&

> @@ -230,6 +229,7 @@ test_expect_success 'git mv to move multiple sources into a directory' '
>  test_expect_success 'git mv should not change sha1 of moved cache entry' '
> +       test_when_finished "rm -f dirty dirty2" &&
>         rm -fr .git &&
>         git init &&
>         echo 1 >dirty &&
> @@ -242,8 +242,6 @@ test_expect_success 'git mv should not change sha1 of moved cache entry' '
>         test "$entry" = "$(git ls-files --stage dirty | cut -f 1)"
>  '
>
> -rm -f dirty dirty2

Makes perfect sense.

> @@ -262,6 +260,7 @@ test_expect_success 'git mv error on conflicted file' '
>  test_expect_success 'git mv should overwrite symlink to a file' '
> +       test_when_finished "rm -f moved symlink" &&
>         rm -fr .git &&
>         git init &&
>         echo 1 >moved &&
> @@ -276,9 +275,8 @@ test_expect_success 'git mv should overwrite symlink to a file' '
>         git diff-files --quiet
>  '
>
> -rm -f moved symlink

Okay.

>  test_expect_success 'git mv should overwrite file with a symlink' '
> +       test_when_finished "rm -f symlink" &&
>         rm -fr .git &&
>         git init &&
>         echo 1 >moved &&

This makes sense, but...

> @@ -292,11 +290,10 @@ test_expect_success 'git mv should overwrite file with a symlink' '
>  test_expect_success SYMLINKS 'check moved symlink' '
> +       test_when_finished "rm -f moved" &&
>         test -h moved
>  '

... this test only gets run on platforms which support symlinks (see
the SYMLINKS predicate in the test definition), so the `moved` file
won't get cleaned up on platforms which don't support symlinks.

> -rm -f moved symlink

If the `moved` file actually causes subsequent tests to fail, then
this might be one of those rare instances in which you'd introduce a
test merely to clean up state from earlier tests. Perhaps something
like this:

    test_expect_success 'cleanup symlink detritus' '
        rm -r moved
    '

However, if `moved` doesn't cause subsequent tests to fail, then it
might also make sense instead just to leave it alone and not bother
cleaning it up.
Junio C Hamano Sept. 25, 2020, 8:54 p.m. UTC | #2
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.
diff mbox series

Patch

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
 '
@@ -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
 '
 
@@ -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 &&
@@ -64,18 +67,14 @@  test_expect_success 'checking -k on multiple untracked files' '
 
 test_expect_success 'checking -f on untracked file with existing target' '
 	: > path0/untracked1 &&
+	test_when_finished "rm -f untracked1 path0/untracked1" &&
+	test_when_finished "rm -f .git/index.lock" &&
 	test_must_fail git mv -f untracked1 path0 &&
 	test ! -f .git/index.lock &&
 	test -f untracked1 &&
 	test -f path0/untracked1
 '
 
-# 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// &&
@@ -149,10 +148,12 @@  test_expect_success 'do not move directory over existing directory' '
 '
 
 test_expect_success 'move into "."' '
+	test_when_finished "rm -fr path?" &&
 	git mv path1/path2/ .
 '
 
 test_expect_success "Michael Cassar's test case" '
+	test_when_finished "rm -fr papers partA" &&
 	rm -fr .git papers partA &&
 	git init &&
 	mkdir -p papers/unsorted papers/all-papers partA &&
@@ -168,8 +169,6 @@  test_expect_success "Michael Cassar's test case" '
 	git ls-tree -r $T | verbose grep partA/outline.txt
 '
 
-rm -fr papers partA path?
-
 test_expect_success "Sergey Vlasov's test case" '
 	rm -fr .git &&
 	git init &&
@@ -230,6 +229,7 @@  test_expect_success 'git mv to move multiple sources into a directory' '
 '
 
 test_expect_success 'git mv should not change sha1 of moved cache entry' '
+	test_when_finished "rm -f dirty dirty2" &&
 	rm -fr .git &&
 	git init &&
 	echo 1 >dirty &&
@@ -242,8 +242,6 @@  test_expect_success 'git mv should not change sha1 of moved cache entry' '
 	test "$entry" = "$(git ls-files --stage dirty | cut -f 1)"
 '
 
-rm -f dirty dirty2
-
 # NB: This test is about the error message
 # as well as the failure.
 test_expect_success 'git mv error on conflicted file' '
@@ -262,6 +260,7 @@  test_expect_success 'git mv error on conflicted file' '
 '
 
 test_expect_success 'git mv should overwrite symlink to a file' '
+	test_when_finished "rm -f moved symlink" &&
 	rm -fr .git &&
 	git init &&
 	echo 1 >moved &&
@@ -276,9 +275,8 @@  test_expect_success 'git mv should overwrite symlink to a file' '
 	git diff-files --quiet
 '
 
-rm -f moved symlink
-
 test_expect_success 'git mv should overwrite file with a symlink' '
+	test_when_finished "rm -f symlink" &&
 	rm -fr .git &&
 	git init &&
 	echo 1 >moved &&
@@ -292,11 +290,10 @@  test_expect_success 'git mv should overwrite file with a symlink' '
 '
 
 test_expect_success SYMLINKS 'check moved symlink' '
+	test_when_finished "rm -f moved" &&
 	test -h moved
 '
 
-rm -f moved symlink
-
 test_expect_success 'setup submodule' '
 	git commit -m initial &&
 	git reset --hard &&