Skip to content

Commit 5beca49

Browse files
avargitster
authored andcommitted
test-lib: simplify by removing test_external
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 d998bd4 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 a5bf824 (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. fb32c41 (t/test-lib.sh: add test_external and test_external_without_stderr, 2008-06-19) 2. d998bd4 (test-lib: Make the test_external_* functions TAP-aware, 2010-06-24) 3. 614fe01 (test-lib: bail out when "-v" used under "prove", 2016-10-22) 4. ef8a6c6 (reftable: utility functions, 2021-10-07) Signed-off-by: Ævar Arnfjörð Bjarmason <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
1 parent 64f3f5a commit 5beca49

File tree

6 files changed

+28
-154
lines changed

6 files changed

+28
-154
lines changed

contrib/credential/netrc/t-git-credential-netrc.sh

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,13 +13,10 @@
1313
'set up test repository' \
1414
'git config --add gpg.program test.git-config-gpg'
1515

16-
# The external test will outputs its own plan
17-
test_external_has_tap=1
18-
1916
export PERL5LIB="$GITPERLLIB"
20-
test_external \
21-
'git-credential-netrc' \
17+
test_expect_success 'git-credential-netrc' '
2218
perl "$GIT_BUILD_DIR"/contrib/credential/netrc/test.pl
19+
'
2320

2421
test_done
2522
)

t/README

Lines changed: 0 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -938,32 +938,6 @@ see test-lib-functions.sh for the full list and their options.
938938
test_done
939939
fi
940940

941-
- test_external [<prereq>] <message> <external> <script>
942-
943-
Execute a <script> with an <external> interpreter (like perl). This
944-
was added for tests like t9700-perl-git.sh which do most of their
945-
work in an external test script.
946-
947-
test_external \
948-
'GitwebCache::*FileCache*' \
949-
perl "$TEST_DIRECTORY"/t9503/test_cache_interface.pl
950-
951-
If the test is outputting its own TAP you should set the
952-
test_external_has_tap variable somewhere before calling the first
953-
test_external* function. See t9700-perl-git.sh for an example.
954-
955-
# The external test will outputs its own plan
956-
test_external_has_tap=1
957-
958-
- test_external_without_stderr [<prereq>] <message> <external> <script>
959-
960-
Like test_external but fail if there's any output on stderr,
961-
instead of checking the exit code.
962-
963-
test_external_without_stderr \
964-
'Perl API' \
965-
perl "$TEST_DIRECTORY"/t9700/test.pl
966-
967941
- test_expect_code <exit-code> <command>
968942

969943
Run a command and ensure that it exits with the given exit code.

t/t0202-gettext-perl.sh

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,9 @@ TEST_PASSES_SANITIZE_LEAK=true
1010
. "$TEST_DIRECTORY"/lib-perl.sh
1111
skip_all_if_no_Test_More
1212

13-
# The external test will outputs its own plan
14-
test_external_has_tap=1
15-
16-
test_external_without_stderr \
17-
'Perl Git::I18N API' \
18-
perl "$TEST_DIRECTORY"/t0202/test.pl
13+
test_expect_success 'run t0202/test.pl to test Git::I18N.pm' '
14+
"$PERL_PATH" "$TEST_DIRECTORY"/t0202/test.pl 2>stderr &&
15+
test_must_be_empty stderr
16+
'
1917

2018
test_done

t/t9700-perl-git.sh

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -43,11 +43,9 @@ test_expect_success \
4343
git config --add test.pathmulti bar
4444
'
4545

46-
# The external test will outputs its own plan
47-
test_external_has_tap=1
48-
49-
test_external_without_stderr \
50-
'Perl API' \
51-
perl "$TEST_DIRECTORY"/t9700/test.pl
46+
test_expect_success 'use t9700/test.pl to test Git.pm' '
47+
"$PERL_PATH" "$TEST_DIRECTORY"/t9700/test.pl 2>stderr &&
48+
test_must_be_empty stderr
49+
'
5250

5351
test_done

t/test-lib-functions.sh

Lines changed: 1 addition & 88 deletions
Original file line numberDiff line numberDiff line change
@@ -633,7 +633,7 @@ test_hook () {
633633
# - Explicitly using test_have_prereq.
634634
#
635635
# - Implicitly by specifying the prerequisite tag in the calls to
636-
# test_expect_{success,failure} and test_external{,_without_stderr}.
636+
# test_expect_{success,failure}
637637
#
638638
# The single parameter is the prerequisite tag (a simple word, in all
639639
# capital letters by convention).
@@ -836,93 +836,6 @@ test_expect_success () {
836836
test_finish_
837837
}
838838

839-
# test_external runs external test scripts that provide continuous
840-
# test output about their progress, and succeeds/fails on
841-
# zero/non-zero exit code. It outputs the test output on stdout even
842-
# in non-verbose mode, and announces the external script with "# run
843-
# <n>: ..." before running it. When providing relative paths, keep in
844-
# mind that all scripts run in "trash directory".
845-
# Usage: test_external description command arguments...
846-
# Example: test_external 'Perl API' perl ../path/to/test.pl
847-
test_external () {
848-
test "$#" = 4 && { test_prereq=$1; shift; } || test_prereq=
849-
test "$#" = 3 ||
850-
BUG "not 3 or 4 parameters to test_external"
851-
descr="$1"
852-
shift
853-
test_verify_prereq
854-
export test_prereq
855-
if ! test_skip "$descr" "$@"
856-
then
857-
# Announce the script to reduce confusion about the
858-
# test output that follows.
859-
say_color "" "# run $test_count: $descr ($*)"
860-
# Export TEST_DIRECTORY, TRASH_DIRECTORY and GIT_TEST_LONG
861-
# to be able to use them in script
862-
export TEST_DIRECTORY TRASH_DIRECTORY GIT_TEST_LONG
863-
# Run command; redirect its stderr to &4 as in
864-
# test_run_, but keep its stdout on our stdout even in
865-
# non-verbose mode.
866-
"$@" 2>&4
867-
if test "$?" = 0
868-
then
869-
if test $test_external_has_tap -eq 0; then
870-
test_ok_ "$descr"
871-
else
872-
say_color "" "# test_external test $descr was ok"
873-
test_success=$(($test_success + 1))
874-
fi
875-
else
876-
if test $test_external_has_tap -eq 0; then
877-
test_failure_ "$descr" "$@"
878-
else
879-
say_color error "# test_external test $descr failed: $@"
880-
test_failure=$(($test_failure + 1))
881-
fi
882-
fi
883-
fi
884-
}
885-
886-
# Like test_external, but in addition tests that the command generated
887-
# no output on stderr.
888-
test_external_without_stderr () {
889-
# The temporary file has no (and must have no) security
890-
# implications.
891-
tmp=${TMPDIR:-/tmp}
892-
stderr="$tmp/git-external-stderr.$$.tmp"
893-
test_external "$@" 4> "$stderr"
894-
test -f "$stderr" || error "Internal error: $stderr disappeared."
895-
descr="no stderr: $1"
896-
shift
897-
say >&3 "# expecting no stderr from previous command"
898-
if test ! -s "$stderr"
899-
then
900-
rm "$stderr"
901-
902-
if test $test_external_has_tap -eq 0; then
903-
test_ok_ "$descr"
904-
else
905-
say_color "" "# test_external_without_stderr test $descr was ok"
906-
test_success=$(($test_success + 1))
907-
fi
908-
else
909-
if test "$verbose" = t
910-
then
911-
output=$(echo; echo "# Stderr is:"; cat "$stderr")
912-
else
913-
output=
914-
fi
915-
# rm first in case test_failure exits.
916-
rm "$stderr"
917-
if test $test_external_has_tap -eq 0; then
918-
test_failure_ "$descr" "$@" "$output"
919-
else
920-
say_color error "# test_external_without_stderr test $descr failed: $@: $output"
921-
test_failure=$(($test_failure + 1))
922-
fi
923-
fi
924-
}
925-
926839
# debugging-friendly alternatives to "test [-f|-d|-e]"
927840
# The commands test the existence or non-existence of $1
928841
test_path_is_file () {

t/test-lib.sh

Lines changed: 17 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1231,27 +1231,24 @@ test_done () {
12311231
fi
12321232
case "$test_failure" in
12331233
0)
1234-
if test $test_external_has_tap -eq 0
1234+
if test $test_remaining -gt 0
12351235
then
1236-
if test $test_remaining -gt 0
1237-
then
1238-
say_color pass "# passed all $msg"
1239-
fi
1240-
1241-
# Maybe print SKIP message
1242-
test -z "$skip_all" || skip_all="# SKIP $skip_all"
1243-
case "$test_count" in
1244-
0)
1245-
say "1..$test_count${skip_all:+ $skip_all}"
1246-
;;
1247-
*)
1248-
test -z "$skip_all" ||
1249-
say_color warn "$skip_all"
1250-
say "1..$test_count"
1251-
;;
1252-
esac
1236+
say_color pass "# passed all $msg"
12531237
fi
12541238

1239+
# Maybe print SKIP message
1240+
test -z "$skip_all" || skip_all="# SKIP $skip_all"
1241+
case "$test_count" in
1242+
0)
1243+
say "1..$test_count${skip_all:+ $skip_all}"
1244+
;;
1245+
*)
1246+
test -z "$skip_all" ||
1247+
say_color warn "$skip_all"
1248+
say "1..$test_count"
1249+
;;
1250+
esac
1251+
12551252
if test -n "$stress" && test -n "$invert_exit_code"
12561253
then
12571254
# We're about to move our "$TRASH_DIRECTORY"
@@ -1286,11 +1283,8 @@ test_done () {
12861283
exit 0 ;;
12871284

12881285
*)
1289-
if test $test_external_has_tap -eq 0
1290-
then
1291-
say_color error "# failed $test_failure among $msg"
1292-
say "1..$test_count"
1293-
fi
1286+
say_color error "# failed $test_failure among $msg"
1287+
say "1..$test_count"
12941288

12951289
if test -n "$invert_exit_code"
12961290
then

0 commit comments

Comments
 (0)