My edc23840b0b (test-lib: bring $remove_trash out of retirement,
2021-05-10) caused a regression where we'd fail to handle
GIT_SKIP_TESTS variable content like 't????', since we were calling
match_pattern_list() from the t/ directory not the trash
directory.
We'd thus glob match in the directory containing our tests, whereas
before we'd do the match in our newly setup (and empty, aside from the
.git) trash directory.
This bug reveals a more general problem though, which I'm attempting
to solve here: We have other users of match_pattern_list() that have
always been broken, namely the --verbose-only=* and --valgrind-only=*
options added in ff09af3fb8f (test-lib: verbose mode for only tests
matching a pattern, 2013-06-23) and 5dfc368f5ec (test-lib: valgrind
for only tests matching a pattern, 2013-06-23).
Those options will try to match an argument like --verbose-only=1*
against a test number like "10", but since we run the match in our own
trash directory an earlier test creating a file like "1one.txt" will
break that option.
We cannot simply quote the $GIT_SKIP_TESTS" being passed to
match_pattern_list(), since we are relying on the $IFS semantics.
Let's instead setup a .test-lib-trash subdirectory under the trash
directory, and an "empty-dir" directory under that. Then let's run the
match_pattern_list() in a sub-shell in that directory.
This fixes the regression in my edc23840b0b, as well as the bugs we've
had in the --{verbose,valgrind}-only=* options. I have tests for this,
but they're a pain to support without my in-flight lib-subtest.sh
changes, I'll submit them once those land.
This has the added benefit of providing a new clean work area for
tests in general, so functions in test-lib-functions.sh can use it for
their own scratch files, instead of potentially tripping up the test
logic itself.
The drawback is that any tests that cares about the complete
cleanliness of its test are might need to be adjusted, as one ls-files
test here does. I think it's still worth it because that'll be one
special case (and a dotfile), as opposed to every test potentially
tripping over the likes of "expected.config" on a glob match of "*".
1. https://lore.kernel.org/git/1d003cac-83fa-0b63-f60e-55513ac45cf9@xxxxxxxxx/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
---
On Wed, Jun 16 2021, Junio C Hamano wrote:
> Interestingly enough, edc23840b0 (test-lib: bring $remove_trash out
> of retirement, 2021-05-10) cleanly reverts without being depended on
> by anything else in the series.
I think reverting edc23840b0 might be the best short-term fix to avoid
dealing with the breakage + get a quick fix down. I can confirm that
nothing else in the series relies on it.
This patch is an attempt at a more general solution to the problem,
which depending on what you think you might want to take instead. It
fixes not only the immediate regression, but as noted other existing
bugs in match_pattern_list() usage.
t/t3000-ls-files-others.sh | 8 ++++++--
t/test-lib-functions.sh | 8 +++++---
t/test-lib.sh | 40 +++++++++++++++++++++++---------------
3 files changed, 35 insertions(+), 21 deletions(-)
diff --git a/t/t3000-ls-files-others.sh b/t/t3000-ls-files-others.sh
index 740ce56eab5..8c62da502df 100755
--- a/t/t3000-ls-files-others.sh
+++ b/t/t3000-ls-files-others.sh
@@ -37,6 +37,7 @@ test_expect_success 'setup: expected output' '
cat >expected1 <<-\EOF &&
expected1
expected2
+ expected2.noempty
expected3
output
path0
@@ -47,7 +48,10 @@ test_expect_success 'setup: expected output' '
sed -e "s|path2/file2|path2/|" <expected1 >expected2 &&
cp expected2 expected3 &&
- echo path4/ >>expected2
+ echo path4/ >>expected2 &&
+
+ echo .test-lib-trash/ >expected2.noempty &&
+ cat expected2 >>expected2.noempty
'
test_expect_success 'ls-files --others' '
@@ -57,7 +61,7 @@ test_expect_success 'ls-files --others' '
test_expect_success 'ls-files --others --directory' '
git ls-files --others --directory >output &&
- test_cmp expected2 output
+ test_cmp expected2.noempty output
'
test_expect_success '--no-empty-directory hides empty directory' '
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index f0448daa74b..33697b0df81 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1040,10 +1040,12 @@ test_cmp_config () {
GD="-C $1" &&
shift
fi &&
- printf "%s\n" "$1" >expect.config &&
+ printf "%s\n" "$1" >"$TRASH_DIRECTORY_TEST_LIB"/expect.config &&
shift &&
- git $GD config "$@" >actual.config &&
- test_cmp expect.config actual.config
+ git $GD config "$@" >"$TRASH_DIRECTORY_TEST_LIB"/actual.config &&
+ test_cmp \
+ "$TRASH_DIRECTORY_TEST_LIB"/expect.config \
+ "$TRASH_DIRECTORY_TEST_LIB"/actual.config
}
# test_cmp_bin - helper to compare binary files
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 54938c64279..7f284f56b1d 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -260,6 +260,8 @@ case "$TRASH_DIRECTORY" in
/*) ;; # absolute path is good
*) TRASH_DIRECTORY="$TEST_OUTPUT_DIRECTORY/$TRASH_DIRECTORY" ;;
esac
+TRASH_DIRECTORY_TEST_LIB="$TRASH_DIRECTORY/.test-lib-trash"
+TRASH_DIRECTORY_TEST_LIB_EMPTY="$TRASH_DIRECTORY_TEST_LIB/empty-dir"
# If --stress was passed, run this test repeatedly in several parallel loops.
if test "$GIT_TEST_STRESS_STARTED" = "done"
@@ -848,7 +850,8 @@ maybe_teardown_verbose () {
last_verbose=t
maybe_setup_verbose () {
test -z "$verbose_only" && return
- if match_pattern_list $test_count $verbose_only
+ if (cd "$TRASH_DIRECTORY_TEST_LIB_EMPTY" &&
+ match_pattern_list $test_count $verbose_only)
then
exec 4>&2 3>&1
# Emit a delimiting blank line when going from
@@ -878,7 +881,8 @@ maybe_setup_valgrind () {
return
fi
GIT_VALGRIND_ENABLED=
- if match_pattern_list $test_count $valgrind_only
+ if (cd "$TRASH_DIRECTORY_TEST_LIB_EMPTY" &&
+ match_pattern_list $test_count $valgrind_only)
then
GIT_VALGRIND_ENABLED=t
fi
@@ -1006,7 +1010,8 @@ test_finish_ () {
test_skip () {
to_skip=
skipped_reason=
- if match_pattern_list $this_test.$test_count $GIT_SKIP_TESTS
+ if (cd "$TRASH_DIRECTORY_TEST_LIB_EMPTY" &&
+ match_pattern_list $this_test.$test_count $GIT_SKIP_TESTS)
then
to_skip=t
skipped_reason="GIT_SKIP_TESTS"
@@ -1177,7 +1182,7 @@ test_done () {
esac
fi
- if test -z "$debug" && test -n "$remove_trash"
+ if test -z "$debug"
then
test -d "$TRASH_DIRECTORY" ||
error "Tests passed but trash directory already removed before test cleanup; aborting"
@@ -1342,11 +1347,22 @@ then
exit 1
fi
+# Test repository
+rm -fr "$TRASH_DIRECTORY" || {
+ GIT_EXIT_OK=t
+ echo >&5 "FATAL: Cannot prepare test area"
+ exit 1
+}
+
+# Set up an early work area for the test code itself
+mkdir -p "$TRASH_DIRECTORY_TEST_LIB_EMPTY" >&3 2>&4 ||
+ error "cannot create test-lib.sh trash directory"
+
# Are we running this test at all?
-remove_trash=
this_test=${0##*/}
this_test=${this_test%%-*}
-if match_pattern_list "$this_test" $GIT_SKIP_TESTS
+if (cd "$TRASH_DIRECTORY_TEST_LIB_EMPTY" &&
+ match_pattern_list "$this_test" $GIT_SKIP_TESTS)
then
say_color info >&3 "skipping test $this_test altogether"
skip_all="skip all tests in $this_test"
@@ -1358,20 +1374,12 @@ HOME="$TRASH_DIRECTORY"
GNUPGHOME="$HOME/gnupg-home-not-used"
export HOME GNUPGHOME
-# Test repository
-rm -fr "$TRASH_DIRECTORY" || {
- GIT_EXIT_OK=t
- echo >&5 "FATAL: Cannot prepare test area"
- exit 1
-}
-
-remove_trash=t
+# "We have the $TRASH_DIRECTORY already, but let's create a
+# $TRASH_DIRECTORY/.git
if test -z "$TEST_NO_CREATE_REPO"
then
git init "$TRASH_DIRECTORY" >&3 2>&4 ||
error "cannot run git init"
-else
- mkdir -p "$TRASH_DIRECTORY"
fi
# Use -P to resolve symlinks in our working directory so that the cwd
--
2.32.0.555.g0268d380f7b
