Re: [PATCH v2 1/3] stash: add test to ensure reflog --rewrite --updatref behavior

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 25, 2022, 17:23 UTC
Message-ID: <xmqq5yp2g8rr.fsf@gitster.g>
In-Reply-To: <220225.86lexz88sp.gmgdl@evledraar.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 6 quoted lines

>> Curious what your thoughts are on an effort to isolate these tests from each other.
>> I like your approach in t/t1417 in creating a test repo and copying it over each time.
>> Something like this?
>
> That looks good to me if you're willing to do that legwork, probably
> better in a preceding cleanup commit.

Yup. Thanks for helping other contributors. I agree with many things you said in your review.

Show 12 quoted lines

>> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
>> index ac345eced8cb..40254f8dc99c 100755
>> --- a/t/t3903-stash.sh
>> +++ b/t/t3903-stash.sh
>> @@ -41,7 +41,9 @@ diff_cmp () {
>>         rm -f "$1.compare" "$2.compare"
>>  }
>>
>> -test_expect_success 'stash some dirty working directory' '
>> +test_expect_success 'setup' '
>> +       git init repo &&
>> +       cd repo &&

We do not want to "chdir" around without isolating it in a subprocess. If this test fails after it goes to "repo" but before it does "cd ..", the next test begins in the "repo" directory, but it is most likely not expecting that.

Show 16 quoted lines

>> -cat >expect <<EOF
>> -diff --git a/file b/file
>> -index 0cfbf08..00750ed 100644
>> ---- a/file
>> -+++ b/file
>> -@@ -1 +1 @@
>> --2
>> -+3
>> -EOF
>> +test_stash () {
>> +       cp -R repo copy &&
>> +       cd copy &&
>> +       test_expect_success "$@" &&
>> +       cd ../ &&
>> +       rm -rf copy
>> +}

This will create an anti-pattern, because you would want to have the part between "cd copy" and "cd .." in a subshell, but you do not want to do test_expect_success inside a subshell. Hence, this is a bad helper that does not help and should not be used, I would think.

Show 17 quoted lines

>> -test_expect_success 'parents of stash' '
>> +test_stash 'parents of stash' '
>>         test $(git rev-parse stash^) = $(git rev-parse HEAD) &&
>>         git diff stash^2..stash >output &&
>>         diff_cmp expect output
>>  '
>
> For this sort of thing I think it's usually better to override
> "test_expect_success" as a last resort, i.e. to have that
> "test_setup_stash_copy" just be a "setup_stash" or whatever function
> called from within your test_expect_success.
>
> And instead of the "rm -rf" later, just do:
>
>     test_when_finished "rm -rf copy" &&
>     cp -R repo copy &&
>     [...]

Yup.  I think this is how we would write it:

	test_expect_success 'parents of stash' '
		test_when_finished "rm -fr copy" &&
		cp -R repo copy &&
		(
			cd copy &&
			... the real body of the test here, like ...
			test $(git rev-parse stash^) = $(git rev-parse HEAD) &&
		)
	'

> The test still needs to deal with the sub-repo, but it could cd or use
> "-C".

I am not sure about this.  test_expect_success does not take "-C".