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

From: Junio C Hamano <[email-masked]>
Date: Mon, 15 May 2023 00:00:38 -0700

Kristoffer Haugsbakk <[email-masked]> writes:

> Signed-off-by: Kristoffer Haugsbakk <[email-masked]>
> ---
>  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.

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