* [PATCH 1/8] t6030: use test_path_is_missing()
2020-04-20 8:54 [PATCH 0/8] t: replace incorrect test_must_fail usage (part 4) Denton Liu
@ 2020-04-20 8:54 ` Denton Liu
2020-04-20 8:54 ` [PATCH 2/8] t7408: replace incorrect uses of test_must_fail Denton Liu
` (7 subsequent siblings)
8 siblings, 0 replies; 20+ messages in thread
From: Denton Liu @ 2020-04-20 8:54 UTC (permalink / raw)
To: Git Mailing List
The test_must_fail() function should only be used for git commands since
we should assume that external commands work sanely. Replace
`test_must_fail test -e` with `test_path_is_missing`.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
t/t6030-bisect-porcelain.sh | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 821a0c88cf..1313142564 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -148,7 +148,7 @@ test_expect_success 'bisect start: no ".git/BISECT_START" created if junk rev' '
test_must_fail git bisect start $HASH4 foo -- &&
git branch > branch.output &&
grep "* other" branch.output > /dev/null &&
- test_must_fail test -e .git/BISECT_START
+ test_path_is_missing .git/BISECT_START
'
test_expect_success 'bisect start: existing ".git/BISECT_START" not modified if junk rev' '
@@ -166,7 +166,7 @@ test_expect_success 'bisect start: no ".git/BISECT_START" if mistaken rev' '
test_must_fail git bisect start $HASH1 $HASH4 -- &&
git branch > branch.output &&
grep "* other" branch.output > /dev/null &&
- test_must_fail test -e .git/BISECT_START
+ test_path_is_missing .git/BISECT_START
'
test_expect_success 'bisect start: no ".git/BISECT_START" if checkout error' '
@@ -175,7 +175,7 @@ test_expect_success 'bisect start: no ".git/BISECT_START" if checkout error' '
git branch &&
git branch > branch.output &&
grep "* other" branch.output > /dev/null &&
- test_must_fail test -e .git/BISECT_START &&
+ test_path_is_missing .git/BISECT_START &&
test -z "$(git for-each-ref "refs/bisect/*")" &&
git checkout HEAD hello
'
@@ -485,7 +485,7 @@ test_expect_success 'optimized merge base checks' '
git bisect bad &&
git bisect good "$A_HASH" > my_bisect_log4.txt &&
test_i18ngrep "merge base must be tested" my_bisect_log4.txt &&
- test_must_fail test -f ".git/BISECT_ANCESTORS_OK"
+ test_path_is_missing ".git/BISECT_ANCESTORS_OK"
'
# This creates another side branch called "parallel" with some files
--
2.26.0.159.g23e2136ad0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/8] t7408: replace incorrect uses of test_must_fail
2020-04-20 8:54 [PATCH 0/8] t: replace incorrect test_must_fail usage (part 4) Denton Liu
2020-04-20 8:54 ` [PATCH 1/8] t6030: use test_path_is_missing() Denton Liu
@ 2020-04-20 8:54 ` Denton Liu
2020-04-20 8:54 ` [PATCH 3/8] t7508: don't use `test_must_fail test_cmp` Denton Liu
` (6 subsequent siblings)
8 siblings, 0 replies; 20+ messages in thread
From: Denton Liu @ 2020-04-20 8:54 UTC (permalink / raw)
To: Git Mailing List
According to t/README, test_must_fail() should only be used to test for
failure in Git commands.
Replace the invocation of `test_must_fail test_path_is_file` with
`test_path_is_missing` since, in this test case, the path should not
exist at all.
In all the cases where `test_must_fail test_alternate_is_used` appears,
test_alternate_is_used() fails because test_line_count() cannot open the
non-existent $alternates_file. Replace
`test_must_fail test_alternate_is_used` with `test_path_is_missing` to
test for this directly.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
t/t7408-submodule-reference.sh | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
index 34ac28c056..a3892f494b 100755
--- a/t/t7408-submodule-reference.sh
+++ b/t/t7408-submodule-reference.sh
@@ -122,8 +122,8 @@ test_expect_success 'missing submodule alternate fails clone and submodule updat
# update of the submodule succeeds
test_must_fail git submodule update --init &&
# and we have no alternates:
- test_must_fail test_alternate_is_used .git/modules/sub/objects/info/alternates sub &&
- test_must_fail test_path_is_file sub/file1
+ test_path_is_missing .git/modules/sub/objects/info/alternates &&
+ test_path_is_missing sub/file1
)
'
@@ -137,7 +137,7 @@ test_expect_success 'ignoring missing submodule alternates passes clone and subm
# update of the submodule succeeds
git submodule update --init &&
# and we have no alternates:
- test_must_fail test_alternate_is_used .git/modules/sub/objects/info/alternates sub &&
+ test_path_is_missing .git/modules/sub/objects/info/alternates &&
test_path_is_file sub/file1
)
'
@@ -182,7 +182,7 @@ check_that_two_of_three_alternates_are_used() {
# immediate submodule has alternate:
test_alternate_is_used .git/modules/subwithsub/objects/info/alternates subwithsub &&
# but nested submodule has no alternate:
- test_must_fail test_alternate_is_used .git/modules/subwithsub/modules/sub/objects/info/alternates subwithsub/sub
+ test_path_is_missing .git/modules/subwithsub/modules/sub/objects/info/alternates
}
--
2.26.0.159.g23e2136ad0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 3/8] t7508: don't use `test_must_fail test_cmp`
2020-04-20 8:54 [PATCH 0/8] t: replace incorrect test_must_fail usage (part 4) Denton Liu
2020-04-20 8:54 ` [PATCH 1/8] t6030: use test_path_is_missing() Denton Liu
2020-04-20 8:54 ` [PATCH 2/8] t7408: replace incorrect uses of test_must_fail Denton Liu
@ 2020-04-20 8:54 ` Denton Liu
2020-04-21 20:59 ` Johannes Sixt
2020-04-20 8:54 ` [PATCH 4/8] t9141: use test_path_is_missing() Denton Liu
` (5 subsequent siblings)
8 siblings, 1 reply; 20+ messages in thread
From: Denton Liu @ 2020-04-20 8:54 UTC (permalink / raw)
To: Git Mailing List
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/t7508-status.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index 482ce3510e..8e969f3e36 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -1471,7 +1471,7 @@ test_expect_success '"status.branch=true" same as "-b"' '
test_expect_success '"status.branch=true" different from "--no-branch"' '
git status -s --no-branch >expected_nobranch &&
git -c status.branch=true status -s >actual &&
- test_must_fail test_cmp expected_nobranch actual
+ ! test_cmp expected_nobranch actual
'
test_expect_success '"status.branch=true" weaker than "--no-branch"' '
--
2.26.0.159.g23e2136ad0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 3/8] t7508: don't use `test_must_fail test_cmp`
2020-04-20 8:54 ` [PATCH 3/8] t7508: don't use `test_must_fail test_cmp` Denton Liu
@ 2020-04-21 20:59 ` Johannes Sixt
0 siblings, 0 replies; 20+ messages in thread
From: Johannes Sixt @ 2020-04-21 20:59 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List
Am 20.04.20 um 10:54 schrieb Denton Liu:
> --- a/t/t7508-status.sh
> +++ b/t/t7508-status.sh
> @@ -1471,7 +1471,7 @@ test_expect_success '"status.branch=true" same as "-b"' '
> test_expect_success '"status.branch=true" different from "--no-branch"' '
> git status -s --no-branch >expected_nobranch &&
> git -c status.branch=true status -s >actual &&
> - test_must_fail test_cmp expected_nobranch actual
> + ! test_cmp expected_nobranch actual
> '
Not your fault, but this is, of course, a very weak test case. Check
that some output that the program generates is _not_ equal to something
else? That condition should be very easy to satisfy.
-- Hannes
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 4/8] t9141: use test_path_is_missing()
2020-04-20 8:54 [PATCH 0/8] t: replace incorrect test_must_fail usage (part 4) Denton Liu
` (2 preceding siblings ...)
2020-04-20 8:54 ` [PATCH 3/8] t7508: don't use `test_must_fail test_cmp` Denton Liu
@ 2020-04-20 8:54 ` Denton Liu
2020-04-20 8:54 ` [PATCH 5/8] t9160: " Denton Liu
` (4 subsequent siblings)
8 siblings, 0 replies; 20+ messages in thread
From: Denton Liu @ 2020-04-20 8:54 UTC (permalink / raw)
To: Git Mailing List
The test_must_fail() function should only be used for git commands since
we assume that external commands work sanely. Since, not only should
these directories not exist, but there shouldn't exist _any_ filesystem
entity in these paths, replace `test_must_fail test -d` with
`test_path_is_missing`.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
t/t9141-git-svn-multiple-branches.sh | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/t/t9141-git-svn-multiple-branches.sh b/t/t9141-git-svn-multiple-branches.sh
index 8e7f7d68b7..bf168a3645 100755
--- a/t/t9141-git-svn-multiple-branches.sh
+++ b/t/t9141-git-svn-multiple-branches.sh
@@ -90,10 +90,10 @@ test_expect_success 'Multiple branch or tag paths require -d' '
) &&
( cd svn_project &&
svn_cmd up &&
- test_must_fail test -d b_one/Nope &&
- test_must_fail test -d b_two/Nope &&
- test_must_fail test -d tags_A/Tagless &&
- test_must_fail test -d tags_B/Tagless
+ test_path_is_missing b_one/Nope &&
+ test_path_is_missing b_two/Nope &&
+ test_path_is_missing tags_A/Tagless &&
+ test_path_is_missing tags_B/Tagless
)
'
--
2.26.0.159.g23e2136ad0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 5/8] t9160: use test_path_is_missing()
2020-04-20 8:54 [PATCH 0/8] t: replace incorrect test_must_fail usage (part 4) Denton Liu
` (3 preceding siblings ...)
2020-04-20 8:54 ` [PATCH 4/8] t9141: use test_path_is_missing() Denton Liu
@ 2020-04-20 8:54 ` Denton Liu
2020-04-20 8:54 ` [PATCH 6/8] t9164: don't use `test_must_fail test_cmp` Denton Liu
` (3 subsequent siblings)
8 siblings, 0 replies; 20+ messages in thread
From: Denton Liu @ 2020-04-20 8:54 UTC (permalink / raw)
To: Git Mailing List
The test_must_fail() function should only be used for git commands since
we assume that external commands work sanely. Since, not only should
this file not exist, but there shouldn't exit _any_ filesystem entity in
these paths, replace `test_must_fail test -f` with
`test_path_is_missing`.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
t/t9160-git-svn-preserve-empty-dirs.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t9160-git-svn-preserve-empty-dirs.sh b/t/t9160-git-svn-preserve-empty-dirs.sh
index 0ede3cfedb..36c6b1a12f 100755
--- a/t/t9160-git-svn-preserve-empty-dirs.sh
+++ b/t/t9160-git-svn-preserve-empty-dirs.sh
@@ -86,8 +86,8 @@ test_expect_success 'remove non-last entry from directory' '
cd "$GIT_REPO" &&
git checkout HEAD~2
) &&
- test_must_fail test -f "$GIT_REPO"/2/.gitignore &&
- test_must_fail test -f "$GIT_REPO"/3/.gitignore
+ test_path_is_missing "$GIT_REPO"/2/.gitignore &&
+ test_path_is_missing "$GIT_REPO"/3/.gitignore
'
# After re-cloning the repository with --placeholder-file specified, there
--
2.26.0.159.g23e2136ad0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 6/8] t9164: don't use `test_must_fail test_cmp`
2020-04-20 8:54 [PATCH 0/8] t: replace incorrect test_must_fail usage (part 4) Denton Liu
` (4 preceding siblings ...)
2020-04-20 8:54 ` [PATCH 5/8] t9160: " Denton Liu
@ 2020-04-20 8:54 ` Denton Liu
2020-04-20 16:21 ` Eric Sunshine
2020-04-20 8:54 ` [PATCH 7/8] t9819: don't use test_must_fail with p4 Denton Liu
` (2 subsequent siblings)
8 siblings, 1 reply; 20+ messages in thread
From: Denton Liu @ 2020-04-20 8:54 UTC (permalink / raw)
To: Git Mailing List
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(-)
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
)
'
--
2.26.0.159.g23e2136ad0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 6/8] t9164: don't use `test_must_fail test_cmp`
2020-04-20 8:54 ` [PATCH 6/8] t9164: don't use `test_must_fail test_cmp` Denton Liu
@ 2020-04-20 16:21 ` Eric Sunshine
2020-04-20 20:09 ` Junio C Hamano
0 siblings, 1 reply; 20+ messages in thread
From: Eric Sunshine @ 2020-04-20 16:21 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List
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.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 6/8] t9164: don't use `test_must_fail test_cmp`
2020-04-20 16:21 ` Eric Sunshine
@ 2020-04-20 20:09 ` Junio C Hamano
2020-04-20 20:13 ` Eric Sunshine
2020-04-21 20:16 ` Denton Liu
0 siblings, 2 replies; 20+ messages in thread
From: Junio C Hamano @ 2020-04-20 20:09 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Denton Liu, Git Mailing List
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?
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 6/8] t9164: don't use `test_must_fail test_cmp`
2020-04-20 20:09 ` Junio C Hamano
@ 2020-04-20 20:13 ` Eric Sunshine
2020-04-21 20:16 ` Denton Liu
1 sibling, 0 replies; 20+ messages in thread
From: Eric Sunshine @ 2020-04-20 20:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Denton Liu, Git Mailing List
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.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 6/8] t9164: don't use `test_must_fail test_cmp`
2020-04-20 20:09 ` Junio C Hamano
2020-04-20 20:13 ` Eric Sunshine
@ 2020-04-21 20:16 ` Denton Liu
2020-04-21 20:44 ` Junio C Hamano
1 sibling, 1 reply; 20+ messages in thread
From: Denton Liu @ 2020-04-21 20:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Eric Sunshine, Git Mailing List
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.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 6/8] t9164: don't use `test_must_fail test_cmp`
2020-04-21 20:16 ` Denton Liu
@ 2020-04-21 20:44 ` Junio C Hamano
2020-04-22 8:54 ` Denton Liu
0 siblings, 1 reply; 20+ messages in thread
From: Junio C Hamano @ 2020-04-21 20:44 UTC (permalink / raw)
To: Denton Liu; +Cc: Eric Sunshine, Git Mailing List
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.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 6/8] t9164: don't use `test_must_fail test_cmp`
2020-04-21 20:44 ` Junio C Hamano
@ 2020-04-22 8:54 ` Denton Liu
2020-04-22 15:50 ` Junio C Hamano
0 siblings, 1 reply; 20+ messages in thread
From: Denton Liu @ 2020-04-22 8:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Eric Sunshine, Git Mailing List
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/
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 7/8] t9819: don't use test_must_fail with p4
2020-04-20 8:54 [PATCH 0/8] t: replace incorrect test_must_fail usage (part 4) Denton Liu
` (5 preceding siblings ...)
2020-04-20 8:54 ` [PATCH 6/8] t9164: don't use `test_must_fail test_cmp` Denton Liu
@ 2020-04-20 8:54 ` Denton Liu
2020-04-20 8:54 ` [PATCH 8/8] t9902: don't use `test_must_fail __git_*` Denton Liu
2020-04-20 11:49 ` [PATCH 0/8] t: replace incorrect test_must_fail usage (part 4) Derrick Stolee
8 siblings, 0 replies; 20+ messages in thread
From: Denton Liu @ 2020-04-20 8:54 UTC (permalink / raw)
To: Git Mailing List
We were using `test_must_fail p4` to test that the p4 command failed as
expected. However, test_must_fail() is used to ensure that commands fail
in an expected way, not due to something like a segv. Since we are not
in the business of verifying the sanity of the external world, replace
`test_must_fail p4` with `! p4` and assume that the `p4` command does
not die unexpectedly.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
t/t9819-git-p4-case-folding.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t9819-git-p4-case-folding.sh b/t/t9819-git-p4-case-folding.sh
index 600ce1e0b0..b4d93f0c17 100755
--- a/t/t9819-git-p4-case-folding.sh
+++ b/t/t9819-git-p4-case-folding.sh
@@ -30,7 +30,7 @@ test_expect_success 'Check p4 is in case-folding mode' '
cd "$cli" &&
>lc/FILE.TXT &&
p4 add lc/FILE.TXT &&
- test_must_fail p4 submit -d "Cannot add file differing only in case" lc/FILE.TXT
+ ! p4 submit -d "Cannot add file differing only in case" lc/FILE.TXT
)
'
--
2.26.0.159.g23e2136ad0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 8/8] t9902: don't use `test_must_fail __git_*`
2020-04-20 8:54 [PATCH 0/8] t: replace incorrect test_must_fail usage (part 4) Denton Liu
` (6 preceding siblings ...)
2020-04-20 8:54 ` [PATCH 7/8] t9819: don't use test_must_fail with p4 Denton Liu
@ 2020-04-20 8:54 ` Denton Liu
2020-04-21 21:16 ` Johannes Sixt
2020-04-20 11:49 ` [PATCH 0/8] t: replace incorrect test_must_fail usage (part 4) Derrick Stolee
8 siblings, 1 reply; 20+ messages in thread
From: Denton Liu @ 2020-04-20 8:54 UTC (permalink / raw)
To: Git Mailing List
We should only use test_must_fail() to test git commands. Replace
`test_must_fail` with `!` so that we don't use test_must_fail() on the
completion functions.
This is done because test_must_fail() is used to except git exiting with
an expected error but it will still return an error if it detects
unexpected errors such as a segfault. In the case of these completion
functions, the return codes of the git commands aren't checked and, most
of the time, they will just explicitly return 1 or have an unrelated
command return 0. As a result, it doesn't really make sense to use
`test_must_fail` so use `!` instead.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
t/t9902-completion.sh | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 5505e5aa24..320c755971 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -294,7 +294,7 @@ test_expect_success '__git_find_repo_path - "git -C" while .git directory in par
test_expect_success '__git_find_repo_path - non-existing path in "git -C"' '
(
__git_C_args=(-C non-existing) &&
- test_must_fail __git_find_repo_path &&
+ ! __git_find_repo_path &&
printf "$__git_repo_path" >"$actual"
) &&
test_must_be_empty "$actual"
@@ -303,7 +303,7 @@ test_expect_success '__git_find_repo_path - non-existing path in "git -C"' '
test_expect_success '__git_find_repo_path - non-existing path in $__git_dir' '
(
__git_dir="non-existing" &&
- test_must_fail __git_find_repo_path &&
+ ! __git_find_repo_path &&
printf "$__git_repo_path" >"$actual"
) &&
test_must_be_empty "$actual"
@@ -313,7 +313,7 @@ test_expect_success '__git_find_repo_path - non-existing $GIT_DIR' '
(
GIT_DIR="$ROOT/non-existing" &&
export GIT_DIR &&
- test_must_fail __git_find_repo_path &&
+ ! __git_find_repo_path &&
printf "$__git_repo_path" >"$actual"
) &&
test_must_be_empty "$actual"
@@ -362,7 +362,7 @@ test_expect_success '__git_find_repo_path - not a git repository' '
cd non-repo &&
GIT_CEILING_DIRECTORIES="$ROOT" &&
export GIT_CEILING_DIRECTORIES &&
- test_must_fail __git_find_repo_path &&
+ ! __git_find_repo_path &&
printf "$__git_repo_path" >"$actual"
) &&
test_must_be_empty "$actual"
@@ -381,7 +381,7 @@ test_expect_success '__gitdir - finds repo' '
test_expect_success '__gitdir - returns error when cannot find repo' '
(
__git_dir="non-existing" &&
- test_must_fail __gitdir >"$actual"
+ ! __gitdir >"$actual"
) &&
test_must_be_empty "$actual"
'
@@ -608,7 +608,7 @@ test_expect_success '__git_is_configured_remote' '
git remote add remote_2 git://remote_2 &&
(
verbose __git_is_configured_remote remote_2 &&
- test_must_fail __git_is_configured_remote non-existent
+ ! __git_is_configured_remote non-existent
)
'
--
2.26.0.159.g23e2136ad0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 8/8] t9902: don't use `test_must_fail __git_*`
2020-04-20 8:54 ` [PATCH 8/8] t9902: don't use `test_must_fail __git_*` Denton Liu
@ 2020-04-21 21:16 ` Johannes Sixt
2020-04-22 8:35 ` Denton Liu
0 siblings, 1 reply; 20+ messages in thread
From: Johannes Sixt @ 2020-04-21 21:16 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List
Am 20.04.20 um 10:54 schrieb Denton Liu:
> We should only use test_must_fail() to test git commands. Replace
> `test_must_fail` with `!` so that we don't use test_must_fail() on the
> completion functions.
>
> This is done because test_must_fail() is used to except git exiting with
> an expected error but it will still return an error if it detects
> unexpected errors such as a segfault. In the case of these completion
> functions, the return codes of the git commands aren't checked and, most
> of the time, they will just explicitly return 1 or have an unrelated
> command return 0. As a result, it doesn't really make sense to use
> `test_must_fail` so use `!` instead.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> t/t9902-completion.sh | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 5505e5aa24..320c755971 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -294,7 +294,7 @@ test_expect_success '__git_find_repo_path - "git -C" while .git directory in par
> test_expect_success '__git_find_repo_path - non-existing path in "git -C"' '
> (
> __git_C_args=(-C non-existing) &&
> - test_must_fail __git_find_repo_path &&
> + ! __git_find_repo_path &&
> printf "$__git_repo_path" >"$actual"
> ) &&
> test_must_be_empty "$actual"
> @@ -303,7 +303,7 @@ test_expect_success '__git_find_repo_path - non-existing path in "git -C"' '
> test_expect_success '__git_find_repo_path - non-existing path in $__git_dir' '
> (
> __git_dir="non-existing" &&
> - test_must_fail __git_find_repo_path &&
> + ! __git_find_repo_path &&
> printf "$__git_repo_path" >"$actual"
> ) &&
> test_must_be_empty "$actual"
> @@ -313,7 +313,7 @@ test_expect_success '__git_find_repo_path - non-existing $GIT_DIR' '
> (
> GIT_DIR="$ROOT/non-existing" &&
> export GIT_DIR &&
> - test_must_fail __git_find_repo_path &&
> + ! __git_find_repo_path &&
> printf "$__git_repo_path" >"$actual"
> ) &&
> test_must_be_empty "$actual"
> @@ -362,7 +362,7 @@ test_expect_success '__git_find_repo_path - not a git repository' '
> cd non-repo &&
> GIT_CEILING_DIRECTORIES="$ROOT" &&
> export GIT_CEILING_DIRECTORIES &&
> - test_must_fail __git_find_repo_path &&
> + ! __git_find_repo_path &&
> printf "$__git_repo_path" >"$actual"
> ) &&
> test_must_be_empty "$actual"
> @@ -381,7 +381,7 @@ test_expect_success '__gitdir - finds repo' '
> test_expect_success '__gitdir - returns error when cannot find repo' '
> (
> __git_dir="non-existing" &&
> - test_must_fail __gitdir >"$actual"
> + ! __gitdir >"$actual"
> ) &&
> test_must_be_empty "$actual"
> '
> @@ -608,7 +608,7 @@ test_expect_success '__git_is_configured_remote' '
> git remote add remote_2 git://remote_2 &&
> (
> verbose __git_is_configured_remote remote_2 &&
> - test_must_fail __git_is_configured_remote non-existent
> + ! __git_is_configured_remote non-existent
> )
> '
>
>
I actually think that the use of test_must_fail has some merit in these
cases, because the shell function __git_find_repo_path runs `git
rev-parse` behind the scenes, and it runs it in such a way that
test_must_fail would do the right thing: the function just dispatches
into a handful of simple cases, each basically only with a variable
assignment, two of them in the form
var=$(git rev-parse ...)
I would suggest to drop this patch.
-- Hannes
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 8/8] t9902: don't use `test_must_fail __git_*`
2020-04-21 21:16 ` Johannes Sixt
@ 2020-04-22 8:35 ` Denton Liu
0 siblings, 0 replies; 20+ messages in thread
From: Denton Liu @ 2020-04-22 8:35 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Git Mailing List
Hi Hannes,
On Tue, Apr 21, 2020 at 11:16:09PM +0200, Johannes Sixt wrote:
> Am 20.04.20 um 10:54 schrieb Denton Liu:
> > We should only use test_must_fail() to test git commands. Replace
> > `test_must_fail` with `!` so that we don't use test_must_fail() on the
> > completion functions.
> >
> > This is done because test_must_fail() is used to except git exiting with
> > an expected error but it will still return an error if it detects
> > unexpected errors such as a segfault. In the case of these completion
> > functions, the return codes of the git commands aren't checked and, most
> > of the time, they will just explicitly return 1 or have an unrelated
> > command return 0. As a result, it doesn't really make sense to use
> > `test_must_fail` so use `!` instead.
> >
> > Signed-off-by: Denton Liu <liu.denton@gmail.com>
> > ---
> > t/t9902-completion.sh | 12 ++++++------
> > 1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> > index 5505e5aa24..320c755971 100755
> > --- a/t/t9902-completion.sh
> > +++ b/t/t9902-completion.sh
> > @@ -294,7 +294,7 @@ test_expect_success '__git_find_repo_path - "git -C" while .git directory in par
> > test_expect_success '__git_find_repo_path - non-existing path in "git -C"' '
> > (
> > __git_C_args=(-C non-existing) &&
> > - test_must_fail __git_find_repo_path &&
> > + ! __git_find_repo_path &&
> > printf "$__git_repo_path" >"$actual"
> > ) &&
> > test_must_be_empty "$actual"
> > @@ -303,7 +303,7 @@ test_expect_success '__git_find_repo_path - non-existing path in "git -C"' '
> > test_expect_success '__git_find_repo_path - non-existing path in $__git_dir' '
> > (
> > __git_dir="non-existing" &&
> > - test_must_fail __git_find_repo_path &&
> > + ! __git_find_repo_path &&
> > printf "$__git_repo_path" >"$actual"
> > ) &&
> > test_must_be_empty "$actual"
> > @@ -313,7 +313,7 @@ test_expect_success '__git_find_repo_path - non-existing $GIT_DIR' '
> > (
> > GIT_DIR="$ROOT/non-existing" &&
> > export GIT_DIR &&
> > - test_must_fail __git_find_repo_path &&
> > + ! __git_find_repo_path &&
> > printf "$__git_repo_path" >"$actual"
> > ) &&
> > test_must_be_empty "$actual"
> > @@ -362,7 +362,7 @@ test_expect_success '__git_find_repo_path - not a git repository' '
> > cd non-repo &&
> > GIT_CEILING_DIRECTORIES="$ROOT" &&
> > export GIT_CEILING_DIRECTORIES &&
> > - test_must_fail __git_find_repo_path &&
> > + ! __git_find_repo_path &&
> > printf "$__git_repo_path" >"$actual"
> > ) &&
> > test_must_be_empty "$actual"
> > @@ -381,7 +381,7 @@ test_expect_success '__gitdir - finds repo' '
> > test_expect_success '__gitdir - returns error when cannot find repo' '
> > (
> > __git_dir="non-existing" &&
> > - test_must_fail __gitdir >"$actual"
> > + ! __gitdir >"$actual"
> > ) &&
> > test_must_be_empty "$actual"
> > '
> > @@ -608,7 +608,7 @@ test_expect_success '__git_is_configured_remote' '
> > git remote add remote_2 git://remote_2 &&
> > (
> > verbose __git_is_configured_remote remote_2 &&
> > - test_must_fail __git_is_configured_remote non-existent
> > + ! __git_is_configured_remote non-existent
> > )
> > '
> >
> >
>
> I actually think that the use of test_must_fail has some merit in these
> cases, because the shell function __git_find_repo_path runs `git
> rev-parse` behind the scenes, and it runs it in such a way that
> test_must_fail would do the right thing: the function just dispatches
> into a handful of simple cases, each basically only with a variable
> assignment, two of them in the form
>
> var=$(git rev-parse ...)
>
> I would suggest to drop this patch.
Thanks for the analysis. I agree with you. I think it's good to avoid
using test_must_fail unnecessarily but it wouldn't hurt to err on the
side of caution when we're potentially wrapping a git command (like in
this case).
In a future patch where I disable test_must_fail except for approved
commands, I'll add __git* commands to the whitelist.
Thanks,
Denton
> -- Hannes
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/8] t: replace incorrect test_must_fail usage (part 4)
2020-04-20 8:54 [PATCH 0/8] t: replace incorrect test_must_fail usage (part 4) Denton Liu
` (7 preceding siblings ...)
2020-04-20 8:54 ` [PATCH 8/8] t9902: don't use `test_must_fail __git_*` Denton Liu
@ 2020-04-20 11:49 ` Derrick Stolee
8 siblings, 0 replies; 20+ messages in thread
From: Derrick Stolee @ 2020-04-20 11:49 UTC (permalink / raw)
To: Denton Liu, Git Mailing List
On 4/20/2020 4:54 AM, Denton Liu wrote:
> The overall scope of these patches is to replace inappropriate uses of
> test_must_fail. IOW, we should only allow test_must_fail to run on `git`
> and `test-tool`. Ultimately, we will conclude by making test_must_fail
> error out on non-git commands. An advance view of the final series can
> be found here[1].
I appreciate your efforts here to use best-practices throughout the test
suite. Too often I look at a test script as an example on which to base
a new test, but then I just repeat a bad pattern.
This series looks good to me.
Thanks,
-Stolee
^ permalink raw reply [flat|nested] 20+ messages in thread