| Message ID | dfccb04e2d03656e18286bcca2c558e19d748ffd.1587372771.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t: replace incorrect test_must_fail usage (part 4) | expand |
On Mon, Apr 20, 2020 at 4:55 AM Denton Liu <liu.denton@gmail.com> wrote: > The test_must_fail function should only be used for git commands since > we assume that external commands work sanely. Since test_cmp() just > wraps an external command, replace `test_must_fail test_cmp` with > `! test_cmp`. > > Signed-off-by: Denton Liu <liu.denton@gmail.com> > --- > diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh > @@ -103,7 +103,7 @@ test_expect_success 'check if pre-commit hook fails' ' > - test_must_fail svn_cmd commit -m "this commit should fail" && > + ! svn_cmd commit -m "this commit should fail" && Hmm, this doesn't look like 'test_cmp' mentioned in the commit message.
Eric Sunshine <sunshine@sunshineco.com> writes: > On Mon, Apr 20, 2020 at 4:55 AM Denton Liu <liu.denton@gmail.com> wrote: >> The test_must_fail function should only be used for git commands since >> we assume that external commands work sanely. Since test_cmp() just >> wraps an external command, replace `test_must_fail test_cmp` with >> `! test_cmp`. >> >> Signed-off-by: Denton Liu <liu.denton@gmail.com> >> --- >> diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh >> @@ -103,7 +103,7 @@ test_expect_success 'check if pre-commit hook fails' ' >> - test_must_fail svn_cmd commit -m "this commit should fail" && >> + ! svn_cmd commit -m "this commit should fail" && > > Hmm, this doesn't look like 'test_cmp' mentioned in the commit message. Yeah, the other hunk is about test_cmp and this hunk is about svn_cmd. The stated rationale applies to both wrappers, I think. Subject: [PATCH 6/8] t9164: use test_must_fail only on git The `test_must_fail` function should only be used for git commands; we are not in the business of catching segmentation fault by external commands. Shell helper functions test_cmp and svn_cmd used in this script are wrappers around external commands, so just use `! cmd` instead of `test_must_fail cmd` perhaps, without any change to the code?
On Mon, Apr 20, 2020 at 4:09 PM Junio C Hamano <gitster@pobox.com> wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > > Hmm, this doesn't look like 'test_cmp' mentioned in the commit message. > > Yeah, the other hunk is about test_cmp and this hunk is about > svn_cmd. The stated rationale applies to both wrappers, I think. > > Subject: [PATCH 6/8] t9164: use test_must_fail only on git > > The `test_must_fail` function should only be used for git commands; > we are not in the business of catching segmentation fault by external > commands. Shell helper functions test_cmp and svn_cmd used in this > script are wrappers around external commands, so just use `! cmd` > instead of `test_must_fail cmd` > > perhaps, without any change to the code? That sounds fine.
On Mon, Apr 20, 2020 at 01:09:49PM -0700, Junio C Hamano wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > > > On Mon, Apr 20, 2020 at 4:55 AM Denton Liu <liu.denton@gmail.com> wrote: > >> The test_must_fail function should only be used for git commands since > >> we assume that external commands work sanely. Since test_cmp() just > >> wraps an external command, replace `test_must_fail test_cmp` with > >> `! test_cmp`. > >> > >> Signed-off-by: Denton Liu <liu.denton@gmail.com> > >> --- > >> diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh > >> @@ -103,7 +103,7 @@ test_expect_success 'check if pre-commit hook fails' ' > >> - test_must_fail svn_cmd commit -m "this commit should fail" && > >> + ! svn_cmd commit -m "this commit should fail" && > > > > Hmm, this doesn't look like 'test_cmp' mentioned in the commit message. > > Yeah, the other hunk is about test_cmp and this hunk is about > svn_cmd. The stated rationale applies to both wrappers, I think. > > Subject: [PATCH 6/8] t9164: use test_must_fail only on git > > The `test_must_fail` function should only be used for git commands; > we are not in the business of catching segmentation fault by external > commands. Shell helper functions test_cmp and svn_cmd used in this > script are wrappers around external commands, so just use `! cmd` > instead of `test_must_fail cmd` > > perhaps, without any change to the code? Thanks, this looks good to me too.
Denton Liu <liu.denton@gmail.com> writes: > On Mon, Apr 20, 2020 at 01:09:49PM -0700, Junio C Hamano wrote: >> Eric Sunshine <sunshine@sunshineco.com> writes: >> >> > On Mon, Apr 20, 2020 at 4:55 AM Denton Liu <liu.denton@gmail.com> wrote: >> >> The test_must_fail function should only be used for git commands since >> >> we assume that external commands work sanely. Since test_cmp() just >> >> wraps an external command, replace `test_must_fail test_cmp` with >> >> `! test_cmp`. >> >> >> >> Signed-off-by: Denton Liu <liu.denton@gmail.com> >> >> --- >> >> diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh >> >> @@ -103,7 +103,7 @@ test_expect_success 'check if pre-commit hook fails' ' >> >> - test_must_fail svn_cmd commit -m "this commit should fail" && >> >> + ! svn_cmd commit -m "this commit should fail" && >> > >> > Hmm, this doesn't look like 'test_cmp' mentioned in the commit message. >> >> Yeah, the other hunk is about test_cmp and this hunk is about >> svn_cmd. The stated rationale applies to both wrappers, I think. >> >> Subject: [PATCH 6/8] t9164: use test_must_fail only on git >> >> The `test_must_fail` function should only be used for git commands; >> we are not in the business of catching segmentation fault by external >> commands. Shell helper functions test_cmp and svn_cmd used in this >> script are wrappers around external commands, so just use `! cmd` >> instead of `test_must_fail cmd` >> >> perhaps, without any change to the code? > > Thanks, this looks good to me too. Thanks. So the 4-patch test-must-fail-fix series is now complete? Whee.
Hi Junio, On Tue, Apr 21, 2020 at 01:44:08PM -0700, Junio C Hamano wrote: > Denton Liu <liu.denton@gmail.com> writes: > > > On Mon, Apr 20, 2020 at 01:09:49PM -0700, Junio C Hamano wrote: > >> Eric Sunshine <sunshine@sunshineco.com> writes: > >> > >> > On Mon, Apr 20, 2020 at 4:55 AM Denton Liu <liu.denton@gmail.com> wrote: > >> >> The test_must_fail function should only be used for git commands since > >> >> we assume that external commands work sanely. Since test_cmp() just > >> >> wraps an external command, replace `test_must_fail test_cmp` with > >> >> `! test_cmp`. > >> >> > >> >> Signed-off-by: Denton Liu <liu.denton@gmail.com> > >> >> --- > >> >> diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh > >> >> @@ -103,7 +103,7 @@ test_expect_success 'check if pre-commit hook fails' ' > >> >> - test_must_fail svn_cmd commit -m "this commit should fail" && > >> >> + ! svn_cmd commit -m "this commit should fail" && > >> > > >> > Hmm, this doesn't look like 'test_cmp' mentioned in the commit message. > >> > >> Yeah, the other hunk is about test_cmp and this hunk is about > >> svn_cmd. The stated rationale applies to both wrappers, I think. > >> > >> Subject: [PATCH 6/8] t9164: use test_must_fail only on git > >> > >> The `test_must_fail` function should only be used for git commands; > >> we are not in the business of catching segmentation fault by external > >> commands. Shell helper functions test_cmp and svn_cmd used in this > >> script are wrappers around external commands, so just use `! cmd` > >> instead of `test_must_fail cmd` > >> > >> perhaps, without any change to the code? > > > > Thanks, this looks good to me too. > > Thanks. > > So the 4-patch test-must-fail-fix series is now complete? Whee. Hannes suggested that we should drop the tip commit of this series[1] and I tend to agree with him. Aside from that, though, the series is ready to go. (I could improve 3/8 as suggested here[2] but I'll throw it in the next series instead since I'm trying to get into the habit of not adding in unrelated patches.) [1]: https://lore.kernel.org/git/6cfa2c447e1196d6c4325aff9fac52434d10fda8.1587372771.git.liu.denton@gmail.com/ [2]: https://lore.kernel.org/git/90faeb7a-1c6a-3fc6-6410-5e264c9340e8@kdbg.org/
Denton Liu <liu.denton@gmail.com> writes: > Hannes suggested that we should drop the tip commit of this series[1] > and I tend to agree with him. Aside from that, though, the series is > ready to go. > > (I could improve 3/8 as suggested here[2] but I'll throw it in the next > series instead since I'm trying to get into the habit of not adding in > unrelated patches.) > > [1]: https://lore.kernel.org/git/6cfa2c447e1196d6c4325aff9fac52434d10fda8.1587372771.git.liu.denton@gmail.com/ > [2]: https://lore.kernel.org/git/90faeb7a-1c6a-3fc6-6410-5e264c9340e8@kdbg.org/ I agree with you two that test_must_fail may make sense with these __git_foo helpers used in the completion. It is a bit of shame that there is no opportunity to leave the reasoning behind the decision for later developers, other than in the discussion thread. I agree that 3/8 is good as-is in this series. Thanks.
diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh index 90346ff4e9..8466269bf5 100755 --- a/t/t9164-git-svn-dcommit-concurrent.sh +++ b/t/t9164-git-svn-dcommit-concurrent.sh @@ -92,7 +92,7 @@ test_expect_success 'check if post-commit hook creates a concurrent commit' ' echo 1 >> file && svn_cmd commit -m "changing file" && svn_cmd up && - test_must_fail test_cmp auto_updated_file au_file_saved + ! test_cmp auto_updated_file au_file_saved ) ' @@ -103,7 +103,7 @@ test_expect_success 'check if pre-commit hook fails' ' echo 2 >> file && svn_cmd commit -m "changing file once again" && echo 3 >> file && - test_must_fail svn_cmd commit -m "this commit should fail" && + ! svn_cmd commit -m "this commit should fail" && svn_cmd revert file ) '
The test_must_fail function should only be used for git commands since we assume that external commands work sanely. Since test_cmp() just wraps an external command, replace `test_must_fail test_cmp` with `! test_cmp`. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- t/t9164-git-svn-dcommit-concurrent.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)