git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: "SZEDER Gábor" <szeder.dev@gmail.com>
Cc: git@vger.kernel.org, Denton Liu <liu.denton@gmail.com>
Subject: Re: [PATCH 2/2] test-lib-functions: use BUG() in 'test_must_fail'
Date: Mon, 22 Feb 2021 14:11:02 -0500	[thread overview]
Message-ID: <YDQBxqTbuYgq1xV8@coredump.intra.peff.net> (raw)
In-Reply-To: <YDLXf+OoJabrJTWu@coredump.intra.peff.net>

On Sun, Feb 21, 2021 at 04:58:23PM -0500, Jeff King wrote:

> This is a bit intimate with the magic 7 descriptor. I think it would be
> cleaner to trigger the bug in a sub-test. We do have helpers for that,
> like:
> [...]
> This is modeled after other similar tests. I find the use of
> check_sub_test_lib_test_err here a bit verbose, but I think we could
> also easily do:
> 
>   grep "bug in the test.*only .git. is allowed" bug-fail-nongit/err

In case it helps, here is a patch which can go on top of yours that
implements my suggestion using the (IMHO) more readable grep.

It also adds "test_done" to the sub-test snippets, which my earlier
patch did not include. That's not strictly necessary (we should error
out before we even get there), but it makes the test more robust (we are
sure that the BUG is what caused us to exit non-zero, not the missing
test_done).

> Note that there are some other cases which could likewise be converted
> (the one for test_bool_env, which I noticed when grepping for "7>" when
> investigating the first patch).

That looks like the only other one, and I think is likewise worth
converting (which is in the patch below).

I thought it first it was also a problem that test_bool_env does not use
BUG to catch invalid values. But I think it is trying to make its
message a bit less confusing when the user is the one who provided us
with the invalid value, such as setting GIT_TEST_GIT_DAEMON=nonsense. We
do still exit the test script immediately, though, which is the right
thing.

It does use BUG to complain when test_bool_env didn't get two
parameters, but we don't bother to test it. We could.

-- >8 --
Subject: [PATCH] t0000: put bug/error checks into a sub-test

When checking whether test_bool_env and test_must_fail correctly trigger
errors or bugs, we run them in a subshell (to avoid their "exit" calls
impacting the greater script) with descriptor 7 redirected (to catch
their direct-to-user output). Let's instead run them using our sub-test
helpers. That gives us a more accurate view of what a calling user sees,
and avoids knowing details like the magic of descriptor 7.

Note that I didn't use check_sub_test_lib_test_err here. We don't really
care what (if anything) is printed on stdout, as long as we see our
expected error on stderr. Plus using grep makes it easier to formulate
the expected text (e.g., using "." instead of tricky quoting).

Signed-off-by: Jeff King <peff@peff.net>
---
This does end up with more lines, and certainly more processes, than the
original. I do think the conceptual cleanliness is worth it, though.

 t/t0000-basic.sh | 50 ++++++++++++++++++++++++++++++++++--------------
 1 file changed, 36 insertions(+), 14 deletions(-)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index b9d5c6c404..e5c06d055b 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -981,20 +981,32 @@ test_expect_success 'test_bool_env' '
 
 		envvar=false &&
 		! test_bool_env envvar true &&
-		! test_bool_env envvar false &&
+		! test_bool_env envvar false
+	)
+'
 
+test_expect_success 'test_bool_env invalid value' '
+	run_sub_test_lib_test_err invalid-bool-value "invalid value" <<-\EOF &&
+	test_expect_success "invalid bool" "
 		envvar=invalid &&
-		# When encountering an invalid bool value, test_bool_env
-		# prints its error message to the original stderr of the
-		# test script, hence the redirection of fd 7, and aborts
-		# with "exit 1", hence the subshell.
-		! ( test_bool_env envvar true ) 7>err &&
-		grep "error: test_bool_env requires bool values" err &&
+		export envvar &&
+		test_bool_env envvar true
+	"
+	test_done
+	EOF
+	grep "error: test_bool_env requires bool values" invalid-bool-value/err
+'
 
+test_expect_success 'test_bool_env invalid default' '
+	run_sub_test_lib_test_err invalid-bool-default "invalid default" <<-\EOF &&
+	test_expect_success "invalid bool" "
 		envvar=true &&
-		! ( test_bool_env envvar invalid ) 7>err &&
-		grep "error: test_bool_env requires bool values" err
-	)
+		export envvar &&
+		test_bool_env envvar default
+	"
+	test_done
+	EOF
+	grep "error: test_bool_env requires bool values" invalid-bool-default/err
 '
 
 ################################################################
@@ -1315,13 +1327,23 @@ test_expect_success 'test_must_fail on a failing git command with env' '
 '
 
 test_expect_success 'test_must_fail rejects a non-git command' '
-	! ( test_must_fail grep ^$ notafile ) 7>err &&
-	grep -F "test_must_fail: only '"'"'git'"'"' is allowed" err
+	run_sub_test_lib_test_err bug-fail-nongit "fail nongit" <<-\EOF &&
+	test_expect_success "non-git command" "
+		test_must_fail grep ^$ notafile
+	"
+	test_done
+	EOF
+	grep "bug.*test_must_fail: only .git. is allowed" bug-fail-nongit/err
 '
 
 test_expect_success 'test_must_fail rejects a non-git command with env' '
-	! ( test_must_fail env var1=a var2=b grep ^$ notafile ) 7>err &&
-	grep -F "test_must_fail: only '"'"'git'"'"' is allowed" err
+	run_sub_test_lib_test_err bug-fail-env "fail nongit with env" <<-\EOF &&
+	test_expect_success "non-git command with env" "
+		test_must_fail env var1=a var2=b grep ^$ notafile
+	"
+	test_done
+	EOF
+	grep "bug.*test_must_fail: only .git. is allowed" bug-fail-env/err
 '
 
 test_done
-- 
2.30.1.1033.gd525307ce1



  reply	other threads:[~2021-02-22 19:16 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-21 19:25 [PATCH 1/2] tests: don't mess with fd 7 of test helper functions SZEDER Gábor
2021-02-21 19:25 ` [PATCH 2/2] test-lib-functions: use BUG() in 'test_must_fail' SZEDER Gábor
2021-02-21 21:58   ` Jeff King
2021-02-22 19:11     ` Jeff King [this message]
2021-02-22 19:17       ` Jeff King
2021-02-22 20:02         ` Junio C Hamano
2021-02-21 21:50 ` [PATCH 1/2] tests: don't mess with fd 7 of test helper functions Jeff King
2021-02-22 17:45   ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YDQBxqTbuYgq1xV8@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=liu.denton@gmail.com \
    --cc=szeder.dev@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).