git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v2 09/14] test-lib: simplify by removing test_external

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Jul 20, 2022, 21:21 UTC
Message-ID
<patch-v2-09.14-20bd31615e4-20220720T211221Z-avarab@gmail.com>
In-Reply-To
<cover-v2-00.14-00000000000-20220720T211221Z-avarab@gmail.com>

Remove the "test_external" function added in [1]. This arguably makes the output of t9700-perl-git.sh and friends worse. But as we'll argue below the trade-off is worth it, since "chaining" to another TAP emitter in test-lib.sh is more trouble than it's worth.

The new output of t9700-perl-git.sh is now:
	$ ./t9700-perl-git.sh
	ok 1 - set up test repository
	ok 2 - use t9700/test.pl to test Git.pm
	# passed all 2 test(s)
	1..2
Whereas before this change it would be:
	$ ./t9700-perl-git.sh
	ok 1 - set up test repository
	# run 1: Perl API (perl /home/avar/g/git/t/t9700/test.pl)
	ok 2 - use Git;
	[... omitting tests 3..46 from t/t9700/test.pl ...]
	ok 47 - unquote escape sequences
	1..47
	# test_external test Perl API was ok
	# test_external_without_stderr test no stderr: Perl API was ok

At the time of its addition supporting "test_external" was easy, but when test-lib.sh itself started to emit TAP in [2] we needed to make everything surrounding the emission of the plan consider "test_external". I added that support in [2] so that we could run:

	prove ./t9700-perl-git.sh :: -v

But since then in [3] the door has been closed on combining $HARNESS_ACTIVE and -v, we'll now just die:

	$ prove ./t9700-perl-git.sh :: -v
	Bailout called.  Further testing stopped:  verbose mode forbidden under TAP harness; try --verbose-log
	FAILED--Further testing stopped: verbose mode forbidden under TAP harness; try --verbose-log

So the only use of this has been that *if* we had failure in one of these tests we could e.g. in CI see which test failed based on the test number. Now we'll need to look at the full verbose logs to get that same information.

I think this trade-off is acceptable given the reduction in complexity, and it brings these tests in line with other similar tests, e.g. the reftable tests added in [4] will be condensed down to just one test, which invokes the C helper:

	$ ./t0032-reftable-unittest.sh
	ok 1 - unittests
	# passed all 1 test(s)
	1..1

It would still be nice to have that ":: -v" form work again, it never *really* worked, but even though we've had edge cases test output screwing up the TAP it mostly worked between d998bd4ab67 and [3], so we may have been overzealous in forbidding it outright.

I have local patches which I'm planning to submit sooner than later that get us to that goal, and in a way that isn't buggy. In the meantime getting rid of this special case makes hacking on this area of test-lib.sh easier, as we'll do in subsequent commits.

The switch from "perl" to "$PERL_PATH" here is because "perl" is defined as a shell function in the test suite, see a5bf824f3b4 (t: prevent '-x' tracing from interfering with test helpers' stderr, 2018-02-25). On e.g. the OSX CI the "command perl"... will be part of the emitted stderr.

1. fb32c410087 (t/test-lib.sh: add test_external and
   test_external_without_stderr, 2008-06-19)
2. d998bd4ab67 (test-lib: Make the test_external_* functions
   TAP-aware, 2010-06-24)
3. 614fe015212 (test-lib: bail out when "-v" used under
   "prove", 2016-10-22)
4. ef8a6c62687 (reftable: utility functions, 2021-10-07)
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 .../netrc/t-git-credential-netrc.sh           |  7 +-
 t/README                                      | 26 ------
 t/t0202-gettext-perl.sh                       | 10 +--
 t/t9700-perl-git.sh                           | 10 +--
 t/test-lib-functions.sh                       | 89 +------------------
 t/test-lib.sh                                 | 40 ++++-----
 6 files changed, 28 insertions(+), 154 deletions(-)
diff --git a/contrib/credential/netrc/t-git-credential-netrc.sh b/contrib/credential/netrc/t-git-credential-netrc.sh
index ff17a9460cd..bf2777308a5 100755
--- a/contrib/credential/netrc/t-git-credential-netrc.sh
+++ b/contrib/credential/netrc/t-git-credential-netrc.sh
@@ -13,13 +13,10 @@
 		'set up test repository' \
 		'git config --add gpg.program test.git-config-gpg'
 
-	# The external test will outputs its own plan
-	test_external_has_tap=1
-
 	export PERL5LIB="$GITPERLLIB"
-	test_external \
-		'git-credential-netrc' \
+	test_expect_success 'git-credential-netrc' '
 		perl "$GIT_BUILD_DIR"/contrib/credential/netrc/test.pl
+	'
 
 	test_done
 )
diff --git a/t/README b/t/README
index 0664aee7ed9..98f69ed13d2 100644
--- a/t/README
+++ b/t/README
@@ -938,32 +938,6 @@ see test-lib-functions.sh for the full list and their options.
 	    test_done
 	fi
 
- - test_external [<prereq>] <message> <external> <script>
-
-   Execute a <script> with an <external> interpreter (like perl). This
-   was added for tests like t9700-perl-git.sh which do most of their
-   work in an external test script.
-
-	test_external \
-	    'GitwebCache::*FileCache*' \
-	    perl "$TEST_DIRECTORY"/t9503/test_cache_interface.pl
-
-   If the test is outputting its own TAP you should set the
-   test_external_has_tap variable somewhere before calling the first
-   test_external* function. See t9700-perl-git.sh for an example.
-
-	# The external test will outputs its own plan
-	test_external_has_tap=1
-
- - test_external_without_stderr [<prereq>] <message> <external> <script>
-
-   Like test_external but fail if there's any output on stderr,
-   instead of checking the exit code.
-
-	test_external_without_stderr \
-	    'Perl API' \
-	    perl "$TEST_DIRECTORY"/t9700/test.pl
-
  - test_expect_code <exit-code> <command>
 
    Run a command and ensure that it exits with the given exit code.
diff --git a/t/t0202-gettext-perl.sh b/t/t0202-gettext-perl.sh
index 043b190626c..5a6f28051bd 100755
--- a/t/t0202-gettext-perl.sh
+++ b/t/t0202-gettext-perl.sh
@@ -10,11 +10,9 @@ TEST_PASSES_SANITIZE_LEAK=true
 . "$TEST_DIRECTORY"/lib-perl.sh
 skip_all_if_no_Test_More
 
-# The external test will outputs its own plan
-test_external_has_tap=1
-
-test_external_without_stderr \
-    'Perl Git::I18N API' \
-    perl "$TEST_DIRECTORY"/t0202/test.pl
+test_expect_success 'run t0202/test.pl to test Git::I18N.pm' '
+	"$PERL_PATH" "$TEST_DIRECTORY"/t0202/test.pl 2>stderr &&
+	test_must_be_empty stderr
+'
 
 test_done
diff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh
index 17fc43f6e57..a48fac6f07a 100755
--- a/t/t9700-perl-git.sh
+++ b/t/t9700-perl-git.sh
@@ -43,11 +43,9 @@ test_expect_success \
      git config --add test.pathmulti bar
      '
 
-# The external test will outputs its own plan
-test_external_has_tap=1
-
-test_external_without_stderr \
-    'Perl API' \
-    perl "$TEST_DIRECTORY"/t9700/test.pl
+test_expect_success 'use t9700/test.pl to test Git.pm' '
+	"$PERL_PATH" "$TEST_DIRECTORY"/t9700/test.pl 2>stderr &&
+	test_must_be_empty stderr
+'
 
 test_done
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 6da7273f1d5..869595d4cee 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -633,7 +633,7 @@ test_hook () {
 # - Explicitly using test_have_prereq.
 #
 # - Implicitly by specifying the prerequisite tag in the calls to
-#   test_expect_{success,failure} and test_external{,_without_stderr}.
+#   test_expect_{success,failure}
 #
 # The single parameter is the prerequisite tag (a simple word, in all
 # capital letters by convention).
@@ -836,93 +836,6 @@ test_expect_success () {
 	test_finish_
 }
 
-# test_external runs external test scripts that provide continuous
-# test output about their progress, and succeeds/fails on
-# zero/non-zero exit code.  It outputs the test output on stdout even
-# in non-verbose mode, and announces the external script with "# run
-# <n>: ..." before running it.  When providing relative paths, keep in
-# mind that all scripts run in "trash directory".
-# Usage: test_external description command arguments...
-# Example: test_external 'Perl API' perl ../path/to/test.pl
-test_external () {
-	test "$#" = 4 && { test_prereq=$1; shift; } || test_prereq=
-	test "$#" = 3 ||
-	BUG "not 3 or 4 parameters to test_external"
-	descr="$1"
-	shift
-	test_verify_prereq
-	export test_prereq
-	if ! test_skip "$descr" "$@"
-	then
-		# Announce the script to reduce confusion about the
-		# test output that follows.
-		say_color "" "# run $test_count: $descr ($*)"
-		# Export TEST_DIRECTORY, TRASH_DIRECTORY and GIT_TEST_LONG
-		# to be able to use them in script
-		export TEST_DIRECTORY TRASH_DIRECTORY GIT_TEST_LONG
-		# Run command; redirect its stderr to &4 as in
-		# test_run_, but keep its stdout on our stdout even in
-		# non-verbose mode.
-		"$@" 2>&4
-		if test "$?" = 0
-		then
-			if test $test_external_has_tap -eq 0; then
-				test_ok_ "$descr"
-			else
-				say_color "" "# test_external test $descr was ok"
-				test_success=$(($test_success + 1))
-			fi
-		else
-			if test $test_external_has_tap -eq 0; then
-				test_failure_ "$descr" "$@"
-			else
-				say_color error "# test_external test $descr failed: $@"
-				test_failure=$(($test_failure + 1))
-			fi
-		fi
-	fi
-}
-
-# Like test_external, but in addition tests that the command generated
-# no output on stderr.
-test_external_without_stderr () {
-	# The temporary file has no (and must have no) security
-	# implications.
-	tmp=${TMPDIR:-/tmp}
-	stderr="$tmp/git-external-stderr.$$.tmp"
-	test_external "$@" 4> "$stderr"
-	test -f "$stderr" || error "Internal error: $stderr disappeared."
-	descr="no stderr: $1"
-	shift
-	say >&3 "# expecting no stderr from previous command"
-	if test ! -s "$stderr"
-	then
-		rm "$stderr"
-
-		if test $test_external_has_tap -eq 0; then
-			test_ok_ "$descr"
-		else
-			say_color "" "# test_external_without_stderr test $descr was ok"
-			test_success=$(($test_success + 1))
-		fi
-	else
-		if test "$verbose" = t
-		then
-			output=$(echo; echo "# Stderr is:"; cat "$stderr")
-		else
-			output=
-		fi
-		# rm first in case test_failure exits.
-		rm "$stderr"
-		if test $test_external_has_tap -eq 0; then
-			test_failure_ "$descr" "$@" "$output"
-		else
-			say_color error "# test_external_without_stderr test $descr failed: $@: $output"
-			test_failure=$(($test_failure + 1))
-		fi
-	fi
-}
-
 # debugging-friendly alternatives to "test [-f|-d|-e]"
 # The commands test the existence or non-existence of $1
 test_path_is_file () {
diff --git a/t/test-lib.sh b/t/test-lib.sh
index f5150400c14..453d980bcf7 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1231,27 +1231,24 @@ test_done () {
 	fi
 	case "$test_failure" in
 	0)
-		if test $test_external_has_tap -eq 0
+		if test $test_remaining -gt 0
 		then
-			if test $test_remaining -gt 0
-			then
-				say_color pass "# passed all $msg"
-			fi
-
-			# Maybe print SKIP message
-			test -z "$skip_all" || skip_all="# SKIP $skip_all"
-			case "$test_count" in
-			0)
-				say "1..$test_count${skip_all:+ $skip_all}"
-				;;
-			*)
-				test -z "$skip_all" ||
-				say_color warn "$skip_all"
-				say "1..$test_count"
-				;;
-			esac
+			say_color pass "# passed all $msg"
 		fi
 
+		# Maybe print SKIP message
+		test -z "$skip_all" || skip_all="# SKIP $skip_all"
+		case "$test_count" in
+		0)
+			say "1..$test_count${skip_all:+ $skip_all}"
+			;;
+		*)
+			test -z "$skip_all" ||
+			say_color warn "$skip_all"
+			say "1..$test_count"
+			;;
+		esac
+
 		if test -n "$stress" && test -n "$invert_exit_code"
 		then
 			# We're about to move our "$TRASH_DIRECTORY"
@@ -1286,11 +1283,8 @@ test_done () {
 		exit 0 ;;
 
 	*)
-		if test $test_external_has_tap -eq 0
-		then
-			say_color error "# failed $test_failure among $msg"
-			say "1..$test_count"
-		fi
+		say_color error "# failed $test_failure among $msg"
+		say "1..$test_count"
 
 		if test -n "$invert_exit_code"
 		then

-- 
2.37.1.1064.gc96144cf387
Previous: Ævar Arnfjörð BjarmasonNext: Ævar Arnfjörð Bjarmason
Message 27 of 57 in “leak test: add "check" test mode, mark leak-free tests”
  1. 00/10 leak test: add "check" test mode, mark leak-free testsÆvar Arnfjörð Bjarmason, Jul 19, 2022
  2. 01/10 test-lib.sh: use $1, not $@ in test_known_broken_{ok,failure}_Ævar Arnfjörð Bjarmason, Jul 19, 2022
  3. 02/10 test-lib.sh: don't set GIT_EXIT_OK before calling test_atexit_handlerÆvar Arnfjörð Bjarmason, Jul 19, 2022
  4. 03/10 test-lib.sh: fix GIT_EXIT_OK logic errors, use BAIL_OUTÆvar Arnfjörð Bjarmason, Jul 19, 2022
  5. 04/10 test-lib.sh: add a --invert-exit-code switchÆvar Arnfjörð Bjarmason, Jul 19, 2022
  6. 07/10 test-lib.sh: add a GIT_TEST_PASSING_SANITIZE_LEAK=check modeÆvar Arnfjörð Bjarmason, Jul 19, 2022
  7. Derrick StoleeJul 20, 2022
  8. 06/10 test-lib: add a SANITIZE=leak logging modeÆvar Arnfjörð Bjarmason, Jul 19, 2022
  9. 05/10 t/README: reword the "GIT_TEST_PASSING_SANITIZE_LEAK" descriptionÆvar Arnfjörð Bjarmason, Jul 19, 2022
  10. Derrick StoleeJul 20, 2022
  11. 09/10 leak tests: mark passing SANITIZE=leak tests as leak-freeÆvar Arnfjörð Bjarmason, Jul 19, 2022
  12. Derrick StoleeJul 20, 2022
  13. 08/10 test-lib: have the "check" mode for SANITIZE=leak consider leak logsÆvar Arnfjörð Bjarmason, Jul 19, 2022
  14. Derrick StoleeJul 20, 2022
  15. 10/10 log tests: don't use "exit 1" outside a sub-shellÆvar Arnfjörð Bjarmason, Jul 19, 2022
  16. Junio C HamanoJul 20, 2022
  17. 00/14 leak test: add "check" test mode, mark leak-free testsÆvar Arnfjörð Bjarmason, Jul 20, 2022
  18. 01/14 test-lib: use $1, not $@ in test_known_broken_{ok,failure}_Ævar Arnfjörð Bjarmason, Jul 20, 2022
  19. 02/14 test-lib: don't set GIT_EXIT_OK before calling test_atexit_handlerÆvar Arnfjörð Bjarmason, Jul 20, 2022
  20. 03/14 test-lib: fix GIT_EXIT_OK logic errors, use BAIL_OUTÆvar Arnfjörð Bjarmason, Jul 20, 2022
  21. 04/14 test-lib: add a --invert-exit-code switchÆvar Arnfjörð Bjarmason, Jul 20, 2022
  22. 05/14 t/README: reword the "GIT_TEST_PASSING_SANITIZE_LEAK" descriptionÆvar Arnfjörð Bjarmason, Jul 20, 2022
  23. 06/14 test-lib: add a SANITIZE=leak logging modeÆvar Arnfjörð Bjarmason, Jul 20, 2022
  24. 07/14 t/Makefile: don't remove test-results in "clean-except-prove-cache"Ævar Arnfjörð Bjarmason, Jul 20, 2022
  25. 08/14 tests: move copy/pasted PERL + Test::More checks to a lib-perl.shÆvar Arnfjörð Bjarmason, Jul 20, 2022
  26. 10/14 test-lib: add a GIT_TEST_PASSING_SANITIZE_LEAK=check modeÆvar Arnfjörð Bjarmason, Jul 20, 2022
  27. 09/14 test-lib: simplify by removing test_externalÆvar Arnfjörð Bjarmason, Jul 20, 2022
  28. 11/14 test-lib: have the "check" mode for SANITIZE=leak consider leak logsÆvar Arnfjörð Bjarmason, Jul 20, 2022
  29. 12/14 leak tests: mark passing SANITIZE=leak tests as leak-freeÆvar Arnfjörð Bjarmason, Jul 20, 2022
  30. 13/14 upload-pack: fix a memory leak in create_pack_file()Ævar Arnfjörð Bjarmason, Jul 20, 2022
  31. 14/14 CI: use "GIT_TEST_SANITIZE_LEAK_LOG=true" in linux-leaksÆvar Arnfjörð Bjarmason, Jul 20, 2022
  32. 00/15 leak test: add "check" test mode, mark leak-free testsÆvar Arnfjörð Bjarmason, Jul 27, 2022
  33. 01/15 test-lib: use $1, not $@ in test_known_broken_{ok,failure}_Ævar Arnfjörð Bjarmason, Jul 27, 2022
  34. 02/15 test-lib: don't set GIT_EXIT_OK before calling test_atexit_handlerÆvar Arnfjörð Bjarmason, Jul 27, 2022
  35. 03/15 test-lib: fix GIT_EXIT_OK logic errors, use BAIL_OUTÆvar Arnfjörð Bjarmason, Jul 27, 2022
  36. 05/15 t/README: reword the "GIT_TEST_PASSING_SANITIZE_LEAK" descriptionÆvar Arnfjörð Bjarmason, Jul 27, 2022
  37. 04/15 test-lib: add a --invert-exit-code switchÆvar Arnfjörð Bjarmason, Jul 27, 2022
  38. 06/15 test-lib: add a SANITIZE=leak logging modeÆvar Arnfjörð Bjarmason, Jul 27, 2022
  39. 07/15 t/Makefile: don't remove test-results in "clean-except-prove-cache"Ævar Arnfjörð Bjarmason, Jul 27, 2022
  40. t/Makefile: remove 'test-results' on 'make clean'SZEDER Gábor, Sep 20, 2022
  41. Jeff KingSep 20, 2022
  42. SZEDER GáborSep 20, 2022
  43. Jeff KingSep 20, 2022
  44. t/Makefile: remove 'test-results' on 'make clean'SZEDER Gábor, Sep 20, 2022
  45. Ævar Arnfjörð BjarmasonSep 21, 2022
  46. Junio C HamanoSep 21, 2022
  47. Junio C HamanoSep 21, 2022
  48. Ævar Arnfjörð BjarmasonSep 26, 2022
  49. Junio C HamanoSep 26, 2022
  50. 08/15 tests: move copy/pasted PERL + Test::More checks to a lib-perl.shÆvar Arnfjörð Bjarmason, Jul 27, 2022
  51. 09/15 test-lib: simplify by removing test_externalÆvar Arnfjörð Bjarmason, Jul 27, 2022
  52. 10/15 test-lib: add a GIT_TEST_PASSING_SANITIZE_LEAK=check modeÆvar Arnfjörð Bjarmason, Jul 27, 2022
  53. 12/15 leak tests: don't skip some tests under SANITIZE=leakÆvar Arnfjörð Bjarmason, Jul 27, 2022
  54. 11/15 test-lib: have the "check" mode for SANITIZE=leak consider leak logsÆvar Arnfjörð Bjarmason, Jul 27, 2022
  55. 13/15 leak tests: mark passing SANITIZE=leak tests as leak-freeÆvar Arnfjörð Bjarmason, Jul 27, 2022
  56. 15/15 CI: use "GIT_TEST_SANITIZE_LEAK_LOG=true" in linux-leaksÆvar Arnfjörð Bjarmason, Jul 27, 2022
  57. 14/15 upload-pack: fix a memory leak in create_pack_file()Ævar Arnfjörð Bjarmason, Jul 27, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.