Re: [PATCH v3 1/5] t3200: improve test style

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 5, 2024, 01:25 UTC
Message-ID: <xmqqplw9lbav.fsf@gitster.g>
In-Reply-To: <e6a2628ce57668aa17101e73edaead0ef34d8a8c.1709590037.git.code@khaugsbakk.name>

Kristoffer Haugsbakk <code@khaugsbakk.name> writes:

Show 9 quoted lines

> Also:
>
> • Remove a now-irrelevant comment about test placement and switch back
>   to `main` post-test.
> • Prefer indented literal heredocs (`-\EOF`) except for a block which
>   says that this is intentional
> • Move a `git config` command into the test and mark it as `setup` since
>   the next test depends on it
>

Especially the change to use "-\EOF" to make them align better caused too many tests to be touched, but overall the result may have become much easier to follow. Good job.

Show 10 quoted lines

> -mv .git/config .git/config-saved
> -
>  test_expect_success DEFAULT_REPO_FORMAT 'git branch -m q q2 without config should succeed' '
> +	test_when_finished mv .git/config-saved .git/config &&
> +	mv .git/config .git/config-saved &&
>  	git branch -m q q2 &&
>  	git branch -m q2 q
>  '
>  
> -mv .git/config-saved .git/config

The above is a truly valuable clean-up.

But I am not really sure if the paritcular condition is worth testing in the first place these days. No configuration file means we cannot even read the repository format version, and working under such a condition is quite a bad promise that we would rather not to having to keep. But that is an entirely different topic from what this patch is doing.

Show 8 quoted lines

> -git config branch.s/s.dummy Hello
> -
> -test_expect_success 'git branch -m s/s s should work when s/t is deleted' '
> +test_expect_success '(setup) git branch -m s/s s should work when s/t is deleted' '
> +	git config branch.s/s.dummy Hello &&
>  	git branch --create-reflog s/s &&
>  	git reflog exists refs/heads/s/s &&
>  	git branch --create-reflog s/t &&

I do not know if the change of the title is warranted. It is doing its own test, not just setup. It may be merely donw for the side effect of making the step unskippable, but still ....

> -# Keep this test last, as it changes the current branch

Yes, removal of this line is really appreciated ;-)

Show 14 quoted lines

> -cat >expect <<EOF
> -$HEAD refs/heads/g/h/i@{0}: branch: Created from main
> -EOF
>  test_expect_success 'git checkout -b g/h/i -l should create a branch and a log' '
> +	test_when_finished git checkout main &&
>  	GIT_COMMITTER_DATE="2005-05-26 23:30" \
>  	git checkout -b g/h/i -l main &&
>  	test_ref_exists refs/heads/g/h/i &&
> +	cat >expect <<-EOF &&
> +	$HEAD refs/heads/g/h/i@{0}: branch: Created from main
> +	EOF
>  	git reflog show --no-abbrev-commit refs/heads/g/h/i >actual &&
>  	test_cmp expect actual
>  '

Thanks.