| Message ID | 1f24aa43f70b16381ef0cfb4f1d482706161554d.1684067644.git.code@khaugsbakk.name (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tag: keep the message file in case ref transaction fails | expand |
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. > 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.
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. > 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.
On Mon, May 15, 2023, at 09:00, Junio C Hamano wrote: > 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 [*]. Yep, I was going for the latter. I’ll combine [2/3] and [3/3]. > 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". Ah, I was wondering how to use `test_must_fail` on a command with an environment variable. Thanks.
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 && + test -e .git/TAG_EDITMSG +' + test_done
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> --- t/t7004-tag.sh | 10 ++++++++++ 1 file changed, 10 insertions(+)