Re: [PATCH v2 1/6] t4041, t4060: modernize test style

From: Glen Choo <chooglen@google.com>
Date: Mar 6, 2023, 19:32 UTC
Message-ID: <kl6l8rg9ekvz.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <20230228185642.2357806-1-calvinwan@google.com>

Calvin Wan <calvinwan@google.com> writes:

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

The changes where we moved lines outside of blocks into blocks without changing them look good to me.

> While at it, have tests clean up after themselves with
> test_when_finished

I believe this came about as part of the discussion in

  https://lore.kernel.org/git/xmqqedqtbbf4.fsf@gitster.g

I think it's good to have tests clean up after themselves, but I'm not sure if that's what we're doing in all of these cases, see below.

I'm leaving the diff header in place, since the two files have very confusingly similar tests.

Show 6 quoted lines

> diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh
>  test_expect_success 'typechanged submodule(submodule->blob)' '
> +	test_when_finished rm -rf sm1 &&
>  	git diff --submodule=log >actual &&
>  	cat >expected <<-EOF &&
>  	diff --git a/sm1 b/sm1

This hunk and the next...

Show 12 quoted lines

> @@ -212,9 +215,9 @@ test_expect_success 'typechanged submodule(submodule->blob)' '
>  	test_cmp expected actual
>  '
>  
> -rm -rf sm1 &&
> -git checkout-index sm1
>  test_expect_success 'typechanged submodule(submodule->blob)' '
> +	test_when_finished rm -f sm1 &&
> +	git checkout-index sm1 &&
>  	git diff-index -p --submodule=log HEAD >actual &&
>  	cat >expected <<-EOF &&
>  	Submodule sm1 $head4...0000000 (submodule deleted)

were changed so that the "rm -rf" happens in the clean up phase of the earlier test (test 14) instead of set up phase of the later test (test 15). But, the "rm -rf" actually results in a _different_ state from before 14, so it isn't actually cleaning up, it really is preparation for 15's git checkout-index.

You can observe this by running

  ./t4041-diff-submodule-option.sh --run=1-13,15

which fails as expected. On the other hand, it passes if we move the "rm -rf" into test 15.

Nearly all of the other test_when_finished here have the same problem, where they 'clean up' state that wasn't changed in the same test body. I believe they will show similar dependency issues, though I didn't go through and test them all.

Show 8 quoted lines

> @@ -643,7 +643,6 @@ test_expect_success 'modified submodule contains modified content' '
>  	diff_cmp expected actual
>  '
>  
> -rm -rf sm1
>  test_expect_success 'deleted submodule' '
>  	git diff-index -p --submodule=diff HEAD >actual &&
>  	cat >expected <<-EOF &&

This one is fairly obvious, since the test says 'deleted submodule', but we no longer delete the submodule in the setup.

Show 29 quoted lines

> @@ -779,9 +780,8 @@ test_expect_success 'diff --submodule=diff with .git file' '
>  	diff_cmp expected actual
>  '
>  
> -mv sm2 sm2-bak
> -
>  test_expect_success 'deleted submodule with .git file' '
> +	mv sm2 sm2-bak &&
>  	git diff-index -p --submodule=diff HEAD >actual &&
>  	cat >expected <<-EOF &&
>  	Submodule sm1 $head7...0000000 (submodule deleted)
> @@ -804,9 +804,9 @@ test_expect_success 'deleted submodule with .git file' '
>  	diff_cmp expected actual
>  '
>  
> -echo submodule-to-blob>sm2
> -
>  test_expect_success 'typechanged(submodule->blob) submodule with .git file' '
> +	test_when_finished "rm sm2 && mv sm2-bak sm2" &&
> +	echo submodule-to-blob>sm2 &&
>  	git diff-index -p --submodule=diff HEAD >actual &&
>  	cat >expected <<-EOF &&
>  	Submodule sm1 $head7...0000000 (submodule deleted)
> @@ -836,9 +836,6 @@ test_expect_success 'typechanged(submodule->blob) submodule with .git file' '
>  	diff_cmp expected actual
>  '
>  
> -rm sm2
> -mv sm2-bak sm2

This is the original case that Junio flagged, which I think is an almost correct use of test_when_finished, since we do get back to an earlier state before this string of tests, but not to the state before the actual test with the test_when_finished.

If we want to use test_when_finished here (which I think we do), we should add another test_when_finished to remove the dependency between the two tests. like so:

  test_expect_success 'deleted submodule with .git file' '
  +	test_when_finished "mv sm2-bak sm2" &&
  	mv sm2 sm2-bak &&
    git diff-index -p --submodule=diff HEAD >actual &&

...

 test_expect_success 'typechanged(submodule->blob) submodule with .git file' '
	test_when_finished "rm sm2 && mv sm2-bak sm2" &&
+ mv sm2 sm2-bak &&

Currently, they're still dependent because one creates sm2-bak and the other moves it back, but if we have each test restore sm2, there will be no more dependency.