Mike Rappazzo <rappazzo@xxxxxxxxx> 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 ''
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html