Re: [PATCH 2/3] t/t7004-tag: add failing tag message file test

From: Junio C Hamano <gitster@pobox.com>
Date: May 15, 2023, 07:02 UTC
Message-ID: <xmqq4joeaxgw.fsf@gitster.g>
In-Reply-To: <1f24aa43f70b16381ef0cfb4f1d482706161554d.1684067644.git.code@khaugsbakk.name>

Kristoffer Haugsbakk <code@khaugsbakk.name> writes:

> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> ---
>  t/t7004-tag.sh | 10 ++++++++++
>  1 file changed, 10 insertions(+)

Does this document the current behaviour, i.e. before applying the patch [3/3]? Or is this a new test designed to fail until [3/3] is applied?

If the latter, please don't [*].

Instead, combine this with [3/3] and make it [2/2] that changes the behaviour of the command and protects the new behaviour from future breakages in a single step. Those who are truly curious to see why the code change in it is necessary can apply the "code change plus new test" patch, and then temporarily revert only the code change part in their working tree to see how the test breaks without the code change.

Show 15 quoted lines

> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index 550b5b1cce..1e512dbe06 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -2136,4 +2136,14 @@ test_expect_success 'If tag is created then tag message file is unlinked' '
>  	! test -e .git/TAG_EDITMSG
>  '
>  
> +test_expect_success 'If tag cannot be created then tag message file is not unlinked' '
> +	test_when_finished "git tag -d foo/bar" &&
> +	write_script fakeeditor <<-\EOF &&
> +	echo Message >.git/TAG_EDITMSG
> +	EOF
> +	git tag foo/bar &&
> +	! GIT_EDITOR=./fakeeditor git tag -a foo &&

Imitate other tests that expect a controlled failure from our command, and write something like

	test_must_fail env GIT_EDITOR=./fakeeditor git tag -a foo

so that a segfaulting "git tag" will not count as "failing as expected".

> +	test -e .git/TAG_EDITMSG

Use "test_path_exists" instead.

Thanks.

[Footnote]

 * Introducing this as a failing test "test_expect_failure" in [2/3]
   and then flip it to "test_expect_success" in [3/3] would make
   tests pass without applying [3/3], but generally it is not a
   recommended practice.

   This is because such a [3/3] patch would only show the line with
   the test title and the behaviour of the test will not be shown in
   the diff context.  That hurts reviewability.