| Message ID | 20210221192512.3096291-2-szeder.dev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] tests: don't mess with fd 7 of test helper functions | expand |
On Sun, Feb 21, 2021 at 08:25:12PM +0100, SZEDER Gábor wrote: > In many test helper functions we verify that they were invoked with > sensible parameters, and call BUG() to abort the test script when the > parameters are buggy. 6a67c75948 (test-lib-functions: restrict > test_must_fail usage, 2020-07-07) added such a parameter verification > to 'test_must_fail', but it didn't report the error with BUG(), like > we usually do. OK. I do not care all that much between BUG() and not-BUG here, since we are unlikely to have a test where test_must_fail returning 0 yields success. I guess the most interesting outcome is that we would notice a bug in a test_expect_failure block. > The two tests checking that 'test_must_fail' recognizes invalid > parameters need some updates: > > - BUG() calls 'exit 1' to abort the test script, but we don't want > that to happen while testing 'test_must_fail' itself, so in those > tests we must invoke that function in a subshell. > - These tests check that 'test_must_fail' failed with the > appropriate error message, but BUG() sends its error message to a > different file descriptor, so update the redirection accordingly. 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: diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh index b9d5c6c404..b3fd740452 100755 --- a/t/t0000-basic.sh +++ b/t/t0000-basic.sh @@ -1315,13 +1315,25 @@ 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 + cmd="grep ^$ notafile" && + run_sub_test_lib_test_err bug-fail-nongit "fail nongit" <<-EOF && + test_expect_success "non-git command" "test_must_fail $cmd" + EOF + check_sub_test_lib_test_err bug-fail-nongit <<-\EOF_OUT 3<<-EOF_ERR + EOF_OUT + > error: bug in the test script: test_must_fail: only ${SQ}git${SQ} is allowed: $cmd + EOF_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 + cmd="env var1=a var2=b grep ^$ notafile" && + 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 $cmd" + EOF + check_sub_test_lib_test_err bug-fail-env <<-\EOF_OUT 3<<-EOF_ERR + EOF_OUT + > error: bug in the test script: test_must_fail: only ${SQ}git${SQ} is allowed: $cmd + EOF_ERR ' test_done 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 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). > test_expect_success 'test_must_fail rejects a non-git command' ' > - ! test_must_fail grep ^$ notafile 2>err && > + ! ( test_must_fail grep ^$ notafile ) 7>err && > grep -F "test_must_fail: only '"'"'git'"'"' is allowed" err > ' Holy double-quoting batman! I do think using $SQ or just "." (if using grep) to match single-quotes makes things more readable. Obviously not something you're introducing, but perhaps worth addressing as we touch this test. -Peff
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
On Mon, Feb 22, 2021 at 02:11:02PM -0500, Jeff King wrote: > 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). I'm quite tempted to do this, as well (this is on top of what I just sent, but could easily be done independently by removing the hunks touching the instances I just added): -- >8 -- Subject: [PATCH] t0000: automatically add test_done to sub-test snippets Our sub-test helper already handles setting up test_description, including test-lib.sh, etc. Let's also automatically a test_done at the end, since it is easy to forget and essentially every test snippet will want it. The only test which _wouldn't_ want this is one that was specifically trying to check the behavior when test_done is not run. We don't have such a test, but if we wanted one, it could just as easily run "exit 0" as part of its snippet (which is arguably more obvious and readable than leaving out the test_done, anyway). Signed-off-by: Jeff King <peff@peff.net> --- t/t0000-basic.sh | 46 +--------------------------------------------- 1 file changed, 1 insertion(+), 45 deletions(-) diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh index e5c06d055b..5a9592dd10 100755 --- a/t/t0000-basic.sh +++ b/t/t0000-basic.sh @@ -92,6 +92,7 @@ _run_sub_test_lib_test_common () { . "\$TEST_DIRECTORY"/test-lib.sh EOF cat >>"$name.sh" && + echo "test_done" >>"$name.sh" && export TEST_DIRECTORY && TEST_OUTPUT_DIRECTORY=$(pwd) && export TEST_OUTPUT_DIRECTORY && @@ -141,7 +142,6 @@ test_expect_success 'pretend we have a fully passing test suite' ' do test_expect_success "passing test #$i" "true" done - test_done EOF check_sub_test_lib_test full-pass <<-\EOF > ok 1 - passing test #1 @@ -158,7 +158,6 @@ test_expect_success 'pretend we have a partially passing test suite' ' test_expect_success "passing test #1" "true" test_expect_success "failing test #2" "false" test_expect_success "passing test #3" "true" - test_done EOF check_sub_test_lib_test partial-pass <<-\EOF > ok 1 - passing test #1 @@ -174,7 +173,6 @@ test_expect_success 'pretend we have a known breakage' ' run_sub_test_lib_test failing-todo "A failing TODO test" <<-\EOF && test_expect_success "passing test" "true" test_expect_failure "pretend we have a known breakage" "false" - test_done EOF check_sub_test_lib_test failing-todo <<-\EOF > ok 1 - passing test @@ -188,7 +186,6 @@ test_expect_success 'pretend we have a known breakage' ' test_expect_success 'pretend we have fixed a known breakage' ' run_sub_test_lib_test passing-todo "A passing TODO test" <<-\EOF && test_expect_failure "pretend we have fixed a known breakage" "true" - test_done EOF check_sub_test_lib_test passing-todo <<-\EOF > ok 1 - pretend we have fixed a known breakage # TODO known breakage vanished @@ -203,7 +200,6 @@ test_expect_success 'pretend we have fixed one of two known breakages (run in su test_expect_failure "pretend we have a known breakage" "false" test_expect_success "pretend we have a passing test" "true" test_expect_failure "pretend we have fixed another known breakage" "true" - test_done EOF check_sub_test_lib_test partially-passing-todos <<-\EOF > not ok 1 - pretend we have a known breakage # TODO known breakage @@ -222,7 +218,6 @@ test_expect_success 'pretend we have a pass, fail, and known breakage' ' test_expect_success "passing test" "true" test_expect_success "failing test" "false" test_expect_failure "pretend we have a known breakage" "false" - test_done EOF check_sub_test_lib_test mixed-results1 <<-\EOF > ok 1 - passing test @@ -248,7 +243,6 @@ test_expect_success 'pretend we have a mix of all possible results' ' test_expect_failure "pretend we have a known breakage" "false" test_expect_failure "pretend we have a known breakage" "false" test_expect_failure "pretend we have fixed a known breakage" "true" - test_done EOF check_sub_test_lib_test mixed-results2 <<-\EOF > ok 1 - passing test @@ -277,7 +271,6 @@ test_expect_success C_LOCALE_OUTPUT 'test --verbose' ' test_expect_success "passing test" true test_expect_success "test with output" "echo foo" test_expect_success "failing test" false - test_done EOF mv t1234-verbose/out t1234-verbose/out+ && grep -v "^Initialized empty" t1234-verbose/out+ >t1234-verbose/out && @@ -305,7 +298,6 @@ test_expect_success 'test --verbose-only' ' test_expect_success "passing test" true test_expect_success "test with output" "echo foo" test_expect_success "failing test" false - test_done EOF check_sub_test_lib_test t2345-verbose-only-2 <<-\EOF > ok 1 - passing test @@ -330,7 +322,6 @@ test_expect_success 'GIT_SKIP_TESTS' ' do test_expect_success "passing test #$i" "true" done - test_done EOF check_sub_test_lib_test git-skip-tests-basic <<-\EOF > ok 1 - passing test #1 @@ -351,7 +342,6 @@ test_expect_success 'GIT_SKIP_TESTS several tests' ' do test_expect_success "passing test #$i" "true" done - test_done EOF check_sub_test_lib_test git-skip-tests-several <<-\EOF > ok 1 - passing test #1 @@ -375,7 +365,6 @@ test_expect_success 'GIT_SKIP_TESTS sh pattern' ' do test_expect_success "passing test #$i" "true" done - test_done EOF check_sub_test_lib_test git-skip-tests-sh-pattern <<-\EOF > ok 1 - passing test #1 @@ -399,7 +388,6 @@ test_expect_success 'GIT_SKIP_TESTS entire suite' ' do test_expect_success "passing test #$i" "true" done - test_done EOF check_sub_test_lib_test git-skip-tests-entire-suite <<-\EOF > 1..0 # SKIP skip all tests in git @@ -416,7 +404,6 @@ test_expect_success 'GIT_SKIP_TESTS does not skip unmatched suite' ' do test_expect_success "passing test #$i" "true" done - test_done EOF check_sub_test_lib_test git-skip-tests-unmatched-suite <<-\EOF > ok 1 - passing test #1 @@ -435,7 +422,6 @@ test_expect_success '--run basic' ' do test_expect_success "passing test #$i" "true" done - test_done EOF check_sub_test_lib_test run-basic <<-\EOF > ok 1 - passing test #1 @@ -456,7 +442,6 @@ test_expect_success '--run with a range' ' do test_expect_success "passing test #$i" "true" done - test_done EOF check_sub_test_lib_test run-range <<-\EOF > ok 1 - passing test #1 @@ -477,7 +462,6 @@ test_expect_success '--run with two ranges' ' do test_expect_success "passing test #$i" "true" done - test_done EOF check_sub_test_lib_test run-two-ranges <<-\EOF > ok 1 - passing test #1 @@ -498,7 +482,6 @@ test_expect_success '--run with a left open range' ' do test_expect_success "passing test #$i" "true" done - test_done EOF check_sub_test_lib_test run-left-open-range <<-\EOF > ok 1 - passing test #1 @@ -519,7 +502,6 @@ test_expect_success '--run with a right open range' ' do test_expect_success "passing test #$i" "true" done - test_done EOF check_sub_test_lib_test run-right-open-range <<-\EOF > ok 1 # skip passing test #1 (--run) @@ -540,7 +522,6 @@ test_expect_success '--run with basic negation' ' do test_expect_success "passing test #$i" "true" done - test_done EOF check_sub_test_lib_test run-basic-neg <<-\EOF > ok 1 - passing test #1 @@ -561,7 +542,6 @@ test_expect_success '--run with two negations' ' do test_expect_success "passing test #$i" "true" done - test_done EOF check_sub_test_lib_test run-two-neg <<-\EOF > ok 1 - passing test #1 @@ -582,7 +562,6 @@ test_expect_success '--run a range and negation' ' do test_expect_success "passing test #$i" "true" done - test_done EOF check_sub_test_lib_test run-range-and-neg <<-\EOF > ok 1 - passing test #1 @@ -603,7 +582,6 @@ test_expect_success '--run range negation' ' do test_expect_success "passing test #$i" "true" done - test_done EOF check_sub_test_lib_test run-range-neg <<-\EOF > ok 1 # skip passing test #1 (--run) @@ -625,7 +603,6 @@ test_expect_success '--run include, exclude and include' ' do test_expect_success "passing test #$i" "true" done - test_done EOF check_sub_test_lib_test run-inc-neg-inc <<-\EOF > ok 1 # skip passing test #1 (--run) @@ -647,7 +624,6 @@ test_expect_success '--run include, exclude and include, comma separated' ' do test_expect_success "passing test #$i" "true" done - test_done EOF check_sub_test_lib_test run-inc-neg-inc-comma <<-\EOF > ok 1 # skip passing test #1 (--run) @@ -669,7 +645,6 @@ test_expect_success '--run exclude and include' ' do test_expect_success "passing test #$i" "true" done - test_done EOF check_sub_test_lib_test run-neg-inc <<-\EOF > ok 1 - passing test #1 @@ -691,7 +666,6 @@ test_expect_success '--run empty selectors' ' do test_expect_success "passing test #$i" "true" done - test_done EOF check_sub_test_lib_test run-empty-sel <<-\EOF > ok 1 - passing test #1 @@ -714,7 +688,6 @@ test_expect_success '--run substring selector' ' do test_expect_success "other test #$i" "true" done - test_done EOF check_sub_test_lib_test run-substring-selector <<-\EOF > ok 1 - relevant test @@ -734,7 +707,6 @@ test_expect_success '--run keyword selection' ' "--run invalid range start" \ --run="a-5" <<-\EOF && test_expect_success "passing test #1" "true" - test_done EOF check_sub_test_lib_test_err run-inv-range-start \ <<-\EOF_OUT 3<<-EOF_ERR @@ -749,7 +721,6 @@ test_expect_success '--run invalid range end' ' "--run invalid range end" \ --run="1-z" <<-\EOF && test_expect_success "passing test #1" "true" - test_done EOF check_sub_test_lib_test_err run-inv-range-end \ <<-\EOF_OUT 3<<-EOF_ERR @@ -773,8 +744,6 @@ test_expect_success 'tests respect prerequisites' ' test_expect_success HAVETHIS,HAVEIT "multiple prereqs" "true" test_expect_success HAVEIT,DONTHAVEIT "mixed prereqs (yes,no)" "false" test_expect_success DONTHAVEIT,HAVEIT "mixed prereqs (no,yes)" "false" - - test_done EOF check_sub_test_lib_test prereqs <<-\EOF @@ -799,8 +768,6 @@ test_expect_success 'tests respect lazy prerequisites' ' test_lazy_prereq LAZY_FALSE false test_expect_success LAZY_FALSE "lazy prereq not satisfied" "false" test_expect_success !LAZY_FALSE "negative false prereq" "true" - - test_done EOF check_sub_test_lib_test lazy-prereqs <<-\EOF @@ -828,8 +795,6 @@ test_expect_success 'nested lazy prerequisites' ' test_path_is_missing inner " test_expect_success NESTED_PREREQ "evaluate nested prereq" "true" - - test_done EOF check_sub_test_lib_test nested-lazy <<-\EOF @@ -845,8 +810,6 @@ test_expect_success 'lazy prereqs do not turn off tracing' ' test_lazy_prereq LAZY true test_expect_success lazy "test_have_prereq LAZY && echo trace" - - test_done EOF grep "echo trace" lazy-prereq-and-tracing/err @@ -861,7 +824,6 @@ test_expect_success 'tests clean up after themselves' ' test_expect_success "cleanup happened" " test $clean = yes " - test_done EOF check_sub_test_lib_test cleanup <<-\EOF @@ -883,7 +845,6 @@ test_expect_success 'tests clean up even on failures' ' test_expect_success "failure to clean up causes the test to fail" " test_when_finished \"(exit 2)\" " - test_done EOF check_sub_test_lib_test failing-cleanup <<-\EOF > not ok 1 - tests clean up even after a failure @@ -912,7 +873,6 @@ test_expect_success 'test_atexit is run' ' > ../../dont-clean-atexit && (exit 1) " - test_done EOF test_path_is_file dont-clean-atexit && test_path_is_missing clean-atexit && @@ -992,7 +952,6 @@ test_expect_success 'test_bool_env invalid value' ' export envvar && test_bool_env envvar true " - test_done EOF grep "error: test_bool_env requires bool values" invalid-bool-value/err ' @@ -1004,7 +963,6 @@ test_expect_success 'test_bool_env invalid default' ' export envvar && test_bool_env envvar default " - test_done EOF grep "error: test_bool_env requires bool values" invalid-bool-default/err ' @@ -1331,7 +1289,6 @@ test_expect_success 'test_must_fail rejects a non-git command' ' 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 ' @@ -1341,7 +1298,6 @@ test_expect_success 'test_must_fail rejects a non-git command with env' ' 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 '
Jeff King <peff@peff.net> writes: > ... Let's also automatically a test_done at the > end, ... s/a test_done/add &/ probably.
diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh index a6e570d674..b9d5c6c404 100755 --- a/t/t0000-basic.sh +++ b/t/t0000-basic.sh @@ -1315,12 +1315,12 @@ 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 2>err && + ! ( test_must_fail grep ^$ notafile ) 7>err && grep -F "test_must_fail: only '"'"'git'"'"' is allowed" err ' test_expect_success 'test_must_fail rejects a non-git command with env' ' - ! test_must_fail env var1=a var2=b grep ^$ notafile 2>err && + ! ( test_must_fail env var1=a var2=b grep ^$ notafile ) 7>err && grep -F "test_must_fail: only '"'"'git'"'"' is allowed" err ' diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index a40c1c5d83..cdbc59e4f0 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -910,8 +910,7 @@ test_must_fail () { esac if ! test_must_fail_acceptable "$@" then - echo >&6 "test_must_fail: only 'git' is allowed: $*" - return 1 + BUG "test_must_fail: only 'git' is allowed: $*" fi "$@" 2>&6 exit_code=$?
In many test helper functions we verify that they were invoked with sensible parameters, and call BUG() to abort the test script when the parameters are buggy. 6a67c75948 (test-lib-functions: restrict test_must_fail usage, 2020-07-07) added such a parameter verification to 'test_must_fail', but it didn't report the error with BUG(), like we usually do. As discussed in detail in the previous patch, BUG() didn't really work in 'test_must_fail' back then, but it resolved those issues, so let's use BUG() in this case as well. The two tests checking that 'test_must_fail' recognizes invalid parameters need some updates: - BUG() calls 'exit 1' to abort the test script, but we don't want that to happen while testing 'test_must_fail' itself, so in those tests we must invoke that function in a subshell. - These tests check that 'test_must_fail' failed with the appropriate error message, but BUG() sends its error message to a different file descriptor, so update the redirection accordingly. Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> --- t/t0000-basic.sh | 4 ++-- t/test-lib-functions.sh | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-)