Re: [PATCH v3 1/2] rev-parse tests: add tests executed from a subdirectory

[Junio C Hamano <gitster@pobox.com>]

Mike Rappazzo <rappazzo@gmail.com> writes:

>> Instead of cleaning things up like this, could you please please
>> please fix these existing tests that chdir around without being in a
>> subshell?  If the "previous tests" failed before going down as this
>> step expects, the "cd .. && rm -r" can make things worse.

> I originally copied the pattern from above this code:
> ...
> but Gábor had an objection to it [2].  So I went with this simple cleanup test.

The "|| exit 1" you see everywhere is a sure sign that these tests
are not designed correctly.  These existing tests that do too many
things outside test_expect_success block needs to be redone, so that
each of them can do what it wants to test even when previous ones
failed.

What I had in mind was more a long the lines of this change. I only
did the first several just for illustration (I added an 'exit' to
mark where I stopped), but I think you get the idea.  The point is
that by doing things in subprocess inside test_expect_success you
can avoid disrupting the main test process even when a test fails as
much as possible, and this illustrates how you would deal with the
"cd" and "export".  What I didn't handle was the updates to
.git/config whose effects by earlier tests are relied on later tests
in this illustration.

As to "table driven" vs "explicitly spelling out scripts, accepting
some code repetition", I tend to favor the latter slightly, unless
the table driven approach is done in such a way that each of its
tests are truly independent from one another, i.e. if a row of the
table represents one test, the tests would not be disrupted by
reordering the rows, inserting a new row in the middle, or removing
an existing row.  From my experience, "table-driven" sets of tests
whose rows have inter-dependency has been much harder to debug when
I had to hunt down a bug that manifests itself only in one test in
the middle.

Hope this helps clarify what I meant.

 t/t1500-rev-parse.sh | 82 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 48 insertions(+), 34 deletions(-)

diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 48ee077..22b52c0 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -5,65 +5,79 @@ test_description='test git rev-parse'
 
 test_rev_parse() {
 	name=$1
-	shift
-
-	test_expect_success "$name: is-bare-repository" \
-	"test '$1' = \"\$(git rev-parse --is-bare-repository)\""
+	prep=$2
+	shift 2
+
+	test_expect_success "$name: is-bare-repository" "
+	(
+		$prep &&
+		test '$1' = \"\$(git rev-parse --is-bare-repository)\"
+	)"
 	shift
 	[ $# -eq 0 ] && return
 
-	test_expect_success "$name: is-inside-git-dir" \
-	"test '$1' = \"\$(git rev-parse --is-inside-git-dir)\""
+	test_expect_success "$name: is-inside-git-dir" "
+	(
+		$prep &&
+		test '$1' = \"\$(git rev-parse --is-inside-git-dir)\"
+	)"
 	shift
 	[ $# -eq 0 ] && return
 
-	test_expect_success "$name: is-inside-work-tree" \
-	"test '$1' = \"\$(git rev-parse --is-inside-work-tree)\""
+	test_expect_success "$name: is-inside-work-tree" "
+	(
+		$prep &&
+		test '$1' = \"\$(git rev-parse --is-inside-work-tree)\"
+	)"
 	shift
 	[ $# -eq 0 ] && return
 
-	test_expect_success "$name: prefix" \
-	"test '$1' = \"\$(git rev-parse --show-prefix)\""
+	test_expect_success "$name: prefix" "
+	(
+		$prep &&
+		test '$1' = \"\$(git rev-parse --show-prefix)\"
+	)"
 	shift
 	[ $# -eq 0 ] && return
 
-	test_expect_success "$name: git-dir" \
-	"test '$1' = \"\$(git rev-parse --git-dir)\""
+	test_expect_success "$name: git-dir" "
+	(
+		$prep &&
+		test '$1' = \"\$(git rev-parse --git-dir)\"
+	)"
 	shift
 	[ $# -eq 0 ] && return
 }
 
-# label is-bare is-inside-git is-inside-work prefix git-dir
+# label prep is-bare is-inside-git is-inside-work prefix git-dir
 
 ROOT=$(pwd)
+mkdir -p sub/dir work
 
-test_rev_parse toplevel false false true '' .git
+test_rev_parse toplevel : false false true '' .git
 
-cd .git || exit 1
-test_rev_parse .git/ false true false '' .
-cd objects || exit 1
-test_rev_parse .git/objects/ false true false '' "$ROOT/.git"
-cd ../.. || exit 1
+test_rev_parse .git 'cd .git' false true false '' .
 
-mkdir -p sub/dir || exit 1
-cd sub/dir || exit 1
-test_rev_parse subdirectory false false true sub/dir/ "$ROOT/.git"
-cd ../.. || exit 1
+test_rev_parse .git/objects/ 'cd .git/objects' false true false '' "$ROOT/.git"
 
-git config core.bare true
-test_rev_parse 'core.bare = true' true false false
+test_rev_parse subdirectory 'cd sub/dir' \
+	false false true sub/dir/ "$ROOT/.git"
 
-git config --unset core.bare
-test_rev_parse 'core.bare undefined' false false true
+test_rev_parse 'core.bare = true' 'git config core.bare true' \
+	true false false
 
-mkdir work || exit 1
-cd work || exit 1
-GIT_DIR=../.git
-GIT_CONFIG="$(pwd)"/../.git/config
-export GIT_DIR GIT_CONFIG
+test_rev_parse 'core.bare undefined' 'git config --unset core.bare || :' \
+	false false true
 
-git config core.bare false
-test_rev_parse 'GIT_DIR=../.git, core.bare = false' false false true ''
+test_rev_parse 'GIT_DIR=../.git, core.bare = false' '
+	cd work && 
+	GIT_DIR=../.git &&
+	GIT_CONFIG="$(pwd)"/../.git/config &&
+	export GIT_DIR GIT_CONFIG &&
+	git config core.bare false' \
+	false false true ''
+
+exit
 
 git config core.bare true
 test_rev_parse 'GIT_DIR=../.git, core.bare = true' true false false ''