[03/16] t2018: use test_must_fail for failing git commands

Commit Message

Denton Liu Dec. 27, 2019, 1:47 p.m. UTC

Before, when we expected `git diff` to fail, we negated its status with
`!`. However, if git ever exits unexpectedly, say due to a segfault, we
would not be able to tell the difference between that and a controlled
failure. Use `test_must_fail git diff` instead so that if an unepxected
failure occurs, we can catch it.

We had some instances of `test_must_fail test_dirty_{un,}mergable`.
However, `test_must_fail` should only be used on git commands. Teach
test_dirty_{un,}mergable() to accept `!` as a potential first argument
which will run the git command without test_must_fail(). This prevents
the double-negation where we were effectively running
`test_must_fail test_must_fail git diff ...`.

While we're at it, remove redirections to /dev/null since output is
silenced when running without `-v` and debugging output is useful with
`-v`, remove redirections to /dev/null as it is not useful.

Signed-off-by: Denton Liu <[e-mail]>
---
 t/t2018-checkout-branch.sh | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

Comments

Eric Sunshine Dec. 28, 2019, 7:55 a.m. UTC | #1

On Fri, Dec 27, 2019 at 8:47 AM Denton Liu <[e-mail]> wrote:
> We had some instances of `test_must_fail test_dirty_{un,}mergable`.
> However, `test_must_fail` should only be used on git commands. Teach
> test_dirty_{un,}mergable() to accept `!` as a potential first argument

s/mergable/mergeable/ twice above.

> which will run the git command without test_must_fail(). This prevents
> the double-negation where we were effectively running
> `test_must_fail test_must_fail git diff ...`.

This change makes the situation even more confusing than it already
is. For instance, you can now say:

    test_dirty_unmergable !

which effectively says "not unmergeable", which is the same as "not
not mergeable". Does that mean it is mergeable? Does that mean it
should be calling test_dirty_mergeable()? Same situation arises with:

    test_dirty_mergeable !

It seems like there are four distinct cases that are being tested
here. If that's so, then rather than changing these functions to
accept "!" as an argument, perhaps there should be four distinct
one-liner functions for testing those cases?

Jakub Narębski Dec. 30, 2019, 8:30 p.m. UTC | #2

Denton Liu <[e-mail]> writes:

> While we're at it, remove redirections to /dev/null since output is
> silenced when running without `-v` and debugging output is useful with
> `-v`, remove redirections to /dev/null as it is not useful.
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Very minor nit: The underlined part (after last comma) duplicates what
already has been said.

Patch

diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
index 79b16e4677..e6b852939c 100755
--- a/t/t2018-checkout-branch.sh
+++ b/t/t2018-checkout-branch.sh
@@ -33,7 +33,12 @@  do_checkout () {
 }
 
 test_dirty_unmergeable () {
-	! git diff --exit-code >/dev/null
+	should_fail="test_expect_code 1" &&
+	if test "x$1" = "x!"
+	then
+		should_fail=
+	fi &&
+	$should_fail git diff --exit-code
 }
 
 setup_dirty_unmergeable () {
@@ -41,7 +46,12 @@  setup_dirty_unmergeable () {
 }
 
 test_dirty_mergeable () {
-	! git diff --cached --exit-code >/dev/null
+	should_fail="test_expect_code 1"  &&
+	if test "x$1" = "x!"
+	then
+		should_fail=
+	fi &&
+	$should_fail git diff --cached --exit-code
 }
 
 setup_dirty_mergeable () {
@@ -93,7 +103,7 @@  test_expect_success 'checkout -f -b to a new branch with unmergeable changes dis
 
 	# still dirty and on branch1
 	do_checkout branch2 $HEAD1 "-f -b" &&
-	test_must_fail test_dirty_unmergeable
+	test_dirty_unmergeable !
 '
 
 test_expect_success 'checkout -b to a new branch preserves mergeable changes' '
@@ -111,7 +121,7 @@  test_expect_success 'checkout -f -b to a new branch with mergeable changes disca
 	test_when_finished git reset --hard HEAD &&
 	setup_dirty_mergeable &&
 	do_checkout branch2 $HEAD1 "-f -b" &&
-	test_must_fail test_dirty_mergeable
+	test_dirty_mergeable !
 '
 
 test_expect_success 'checkout -b to an existing branch fails' '
@@ -162,7 +172,7 @@  test_expect_success 'checkout -B to an existing branch with unmergeable changes
 test_expect_success 'checkout -f -B to an existing branch with unmergeable changes discards changes' '
 	# still dirty and on branch1
 	do_checkout branch2 $HEAD1 "-f -B" &&
-	test_must_fail test_dirty_unmergeable
+	test_dirty_unmergeable !
 '
 
 test_expect_success 'checkout -B to an existing branch preserves mergeable changes' '
@@ -179,7 +189,7 @@  test_expect_success 'checkout -f -B to an existing branch with mergeable changes
 
 	setup_dirty_mergeable &&
 	do_checkout branch2 $HEAD1 "-f -B" &&
-	test_must_fail test_dirty_mergeable
+	test_dirty_mergeable !
 '
 
 test_expect_success 'checkout -b <describe>' '