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

[PATCH] test-lib: fix "$remove_trash" regression and match_pattern_list() bugs

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Jun 16, 2021, 08:24 UTC
Message-ID
<patch-1.1-436c723f4f8-20210616T082030Z-avarab@gmail.com>
In-Reply-To
<xmqqa6nqsd2i.fsf@gitster.g>

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@gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
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
Previous: Junio C HamanoNext: Jeff King
Message 7 of 17 in “[BUG] question mark in GIT_SKIP_TESTS is broken in master”
  1. Andrei RybakJun 15, 2021
  2. [BUG] range expressions in GIT_SKIP_TESTS are broken in master (was [BUG] question mark in GIT_SKIP_TESTS is broken in master)Andrei Rybak, Jun 15, 2021
  3. Junio C HamanoJun 16, 2021
  4. Junio C HamanoJun 16, 2021
  5. Jeff KingJun 16, 2021
  6. Junio C HamanoJun 16, 2021
  7. test-lib: fix "$remove_trash" regression and match_pattern_list() bugsÆvar Arnfjörð Bjarmason, Jun 16, 2021
  8. Jeff KingJun 16, 2021
  9. Junio C HamanoJun 16, 2021
  10. Jeff KingJun 16, 2021
  11. Jeff KingJun 16, 2021
  12. Ævar Arnfjörð BjarmasonJun 16, 2021
  13. Jeff KingJun 16, 2021
  14. Junio C HamanoJun 17, 2021
  15. Ævar Arnfjörð BjarmasonJun 16, 2021
  16. Jeff KingJun 16, 2021
  17. Junio C HamanoJun 17, 2021

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.