[PATCH v2 0/5] Skip tests using 3-arg test_*

[PATCH v2 0/5] Skip tests using 3-arg test_*

[Ævar Arnfjörð Bjarmason]

This is just like v1 aside from:
    
    diff --git a/t/t4004-diff-rename-symlink.sh b/t/t4004-diff-rename-symlink.sh
    index 85a77b3..92a65f4 100755
    --- a/t/t4004-diff-rename-symlink.sh
    +++ b/t/t4004-diff-rename-symlink.sh
    @@ -38 +38 @@ test_expect_success SYMLINKS 'setup diff output' "
    -    GIT_DIFF_OPTS=--unified=0 git diff-index -M -p $tree >current
    +    GIT_DIFF_OPTS=--unified=0 git diff-index -M -p $tree >current &&

But I also split the change I made to t/t4004-diff-rename-symlink.sh
from the "tests: implicitly skip SYMLINKS tests using <prereq>" patch
as suggested by Jonathan Nieder.

It was the only one in the aforementioned commit that didn't just nuke
skip_all=* + do 's/success /success SYMLINKS /', so it deserved a
separate patch.
    
Ævar Arnfjörð Bjarmason (5):
  tests: implicitly skip SYMLINKS tests using <prereq>
  t/t4004-diff-rename-symlink.sh: use three-arg <prereq>
  t/t5800-remote-helpers.sh: Skip with prereq on python <2.4
  t/t7800-difftool.sh: Skip with prereq on no PERL
  t/README: Update "Skipping tests" to align with best practices

 t/README                          |   21 +++++++++++++--
 t/t2007-checkout-symlink.sh       |   14 +++-------
 t/t4004-diff-rename-symlink.sh    |   18 +++++--------
 t/t4011-diff-symlink.sh           |   18 ++++---------
 t/t4023-diff-rename-typechange.sh |   14 +++-------
 t/t4114-apply-typechange.sh       |   28 ++++++++-------------
 t/t4115-apply-symlink.sh          |   12 ++-------
 t/t4122-apply-symlink-inside.sh   |   12 ++-------
 t/t5522-pull-symlink.sh           |   14 +++-------
 t/t5800-remote-helpers.sh         |   24 ++++++++----------
 t/t6035-merge-dir-to-symlink.sh   |   20 +++++----------
 t/t7800-difftool.sh               |   49 ++++++++++++++++--------------------
 12 files changed, 100 insertions(+), 144 deletions(-)

[PATCH v2 1/5] tests: implicitly skip SYMLINKS tests using <prereq>

[Ævar Arnfjörð Bjarmason]

Change the tests that skipped due to unavailable SYMLINKS support to
use the three-arg prereq form of test_expect_success.

Now we get an indication of how many tests that need symlinks are
being skipped on platforms that don't support them.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t2007-checkout-symlink.sh       |   14 ++++----------
 t/t4011-diff-symlink.sh           |   18 ++++++------------
 t/t4023-diff-rename-typechange.sh |   14 ++++----------
 t/t4114-apply-typechange.sh       |   28 +++++++++++-----------------
 t/t4115-apply-symlink.sh          |   12 +++---------
 t/t4122-apply-symlink-inside.sh   |   12 +++---------
 t/t5522-pull-symlink.sh           |   14 ++++----------
 t/t6035-merge-dir-to-symlink.sh   |   20 +++++++-------------
 8 files changed, 42 insertions(+), 90 deletions(-)

diff --git a/t/t2007-checkout-symlink.sh b/t/t2007-checkout-symlink.sh
index 05cc8fd..a74ee22 100755
--- a/t/t2007-checkout-symlink.sh
+++ b/t/t2007-checkout-symlink.sh
@@ -6,13 +6,7 @@ test_description='git checkout to switch between branches with symlink<->dir'
 
 . ./test-lib.sh
 
-if ! test_have_prereq SYMLINKS
-then
-	skip_all="symbolic links not supported - skipping tests"
-	test_done
-fi
-
-test_expect_success setup '
+test_expect_success SYMLINKS setup '
 
 	mkdir frotz &&
 	echo hello >frotz/filfre &&
@@ -38,18 +32,18 @@ test_expect_success setup '
 
 '
 
-test_expect_success 'switch from symlink to dir' '
+test_expect_success SYMLINKS 'switch from symlink to dir' '
 
 	git checkout master
 
 '
 
-test_expect_success 'Remove temporary directories & switch to master' '
+test_expect_success SYMLINKS 'Remove temporary directories & switch to master' '
 	rm -fr frotz xyzzy nitfol &&
 	git checkout -f master
 '
 
-test_expect_success 'switch from dir to symlink' '
+test_expect_success SYMLINKS 'switch from dir to symlink' '
 
 	git checkout side
 
diff --git a/t/t4011-diff-symlink.sh b/t/t4011-diff-symlink.sh
index 918a21a..6f69489 100755
--- a/t/t4011-diff-symlink.sh
+++ b/t/t4011-diff-symlink.sh
@@ -9,12 +9,6 @@ test_description='Test diff of symlinks.
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/diff-lib.sh
 
-if ! test_have_prereq SYMLINKS
-then
-	skip_all='Symbolic links not supported, skipping tests.'
-	test_done
-fi
-
 cat > expected << EOF
 diff --git a/frotz b/frotz
 new file mode 120000
@@ -26,7 +20,7 @@ index 0000000..7c465af
 \ No newline at end of file
 EOF
 
-test_expect_success \
+test_expect_success SYMLINKS \
     'diff new symlink' \
     'ln -s xyzzy frotz &&
     git update-index &&
@@ -35,7 +29,7 @@ test_expect_success \
     GIT_DIFF_OPTS=--unified=0 git diff-index -M -p $tree > current &&
     compare_diff_patch current expected'
 
-test_expect_success \
+test_expect_success SYMLINKS \
     'diff unchanged symlink' \
     'tree=$(git write-tree) &&
     git update-index frotz &&
@@ -52,7 +46,7 @@ index 7c465af..0000000
 \ No newline at end of file
 EOF
 
-test_expect_success \
+test_expect_success SYMLINKS \
     'diff removed symlink' \
     'mv frotz frotz2 &&
     git diff-index -M -p $tree > current &&
@@ -62,7 +56,7 @@ cat > expected << EOF
 diff --git a/frotz b/frotz
 EOF
 
-test_expect_success \
+test_expect_success SYMLINKS \
     'diff identical, but newly created symlink' \
     'ln -s xyzzy frotz &&
     git diff-index -M -p $tree > current &&
@@ -80,14 +74,14 @@ index 7c465af..df1db54 120000
 \ No newline at end of file
 EOF
 
-test_expect_success \
+test_expect_success SYMLINKS \
     'diff different symlink' \
     'rm frotz &&
     ln -s yxyyz frotz &&
     git diff-index -M -p $tree > current &&
     compare_diff_patch current expected'
 
-test_expect_success \
+test_expect_success SYMLINKS \
     'diff symlinks with non-existing targets' \
     'ln -s narf pinky &&
     ln -s take\ over brain &&
diff --git a/t/t4023-diff-rename-typechange.sh b/t/t4023-diff-rename-typechange.sh
index 40a95a1..5d20acf 100755
--- a/t/t4023-diff-rename-typechange.sh
+++ b/t/t4023-diff-rename-typechange.sh
@@ -4,13 +4,7 @@ test_description='typechange rename detection'
 
 . ./test-lib.sh
 
-if ! test_have_prereq SYMLINKS
-then
-	skip_all='Symbolic links not supported, skipping tests.'
-	test_done
-fi
-
-test_expect_success setup '
+test_expect_success SYMLINKS setup '
 
 	rm -f foo bar &&
 	cat "$TEST_DIRECTORY"/../COPYING >foo &&
@@ -56,7 +50,7 @@ test_expect_success setup '
 
 '
 
-test_expect_success 'cross renames to be detected for regular files' '
+test_expect_success SYMLINKS 'cross renames to be detected for regular files' '
 
 	git diff-tree five six -r --name-status -B -M | sort >actual &&
 	{
@@ -67,7 +61,7 @@ test_expect_success 'cross renames to be detected for regular files' '
 
 '
 
-test_expect_success 'cross renames to be detected for typechange' '
+test_expect_success SYMLINKS 'cross renames to be detected for typechange' '
 
 	git diff-tree one two -r --name-status -B -M | sort >actual &&
 	{
@@ -78,7 +72,7 @@ test_expect_success 'cross renames to be detected for typechange' '
 
 '
 
-test_expect_success 'moves and renames' '
+test_expect_success SYMLINKS 'moves and renames' '
 
 	git diff-tree three four -r --name-status -B -M | sort >actual &&
 	{
diff --git a/t/t4114-apply-typechange.sh b/t/t4114-apply-typechange.sh
index 164d58c..f12826f 100755
--- a/t/t4114-apply-typechange.sh
+++ b/t/t4114-apply-typechange.sh
@@ -9,13 +9,7 @@ test_description='git apply should not get confused with type changes.
 
 . ./test-lib.sh
 
-if ! test_have_prereq SYMLINKS
-then
-	skip_all='Symbolic links not supported, skipping tests.'
-	test_done
-fi
-
-test_expect_success 'setup repository and commits' '
+test_expect_success SYMLINKS 'setup repository and commits' '
 	echo "hello world" > foo &&
 	echo "hi planet" > bar &&
 	git update-index --add foo bar &&
@@ -48,7 +42,7 @@ test_expect_success 'setup repository and commits' '
 	git branch foo-baz-renamed-from-foo
 	'
 
-test_expect_success 'file renamed from foo to foo/baz' '
+test_expect_success SYMLINKS 'file renamed from foo to foo/baz' '
 	git checkout -f initial &&
 	git diff-tree -M -p HEAD foo-baz-renamed-from-foo > patch &&
 	git apply --index < patch
@@ -56,7 +50,7 @@ test_expect_success 'file renamed from foo to foo/baz' '
 test_debug 'cat patch'
 
 
-test_expect_success 'file renamed from foo/baz to foo' '
+test_expect_success SYMLINKS 'file renamed from foo/baz to foo' '
 	git checkout -f foo-baz-renamed-from-foo &&
 	git diff-tree -M -p HEAD initial > patch &&
 	git apply --index < patch
@@ -64,7 +58,7 @@ test_expect_success 'file renamed from foo/baz to foo' '
 test_debug 'cat patch'
 
 
-test_expect_success 'directory becomes file' '
+test_expect_success SYMLINKS 'directory becomes file' '
 	git checkout -f foo-becomes-a-directory &&
 	git diff-tree -p HEAD initial > patch &&
 	git apply --index < patch
@@ -72,7 +66,7 @@ test_expect_success 'directory becomes file' '
 test_debug 'cat patch'
 
 
-test_expect_success 'file becomes directory' '
+test_expect_success SYMLINKS 'file becomes directory' '
 	git checkout -f initial &&
 	git diff-tree -p HEAD foo-becomes-a-directory > patch &&
 	git apply --index < patch
@@ -80,7 +74,7 @@ test_expect_success 'file becomes directory' '
 test_debug 'cat patch'
 
 
-test_expect_success 'file becomes symlink' '
+test_expect_success SYMLINKS 'file becomes symlink' '
 	git checkout -f initial &&
 	git diff-tree -p HEAD foo-symlinked-to-bar > patch &&
 	git apply --index < patch
@@ -88,21 +82,21 @@ test_expect_success 'file becomes symlink' '
 test_debug 'cat patch'
 
 
-test_expect_success 'symlink becomes file' '
+test_expect_success SYMLINKS 'symlink becomes file' '
 	git checkout -f foo-symlinked-to-bar &&
 	git diff-tree -p HEAD foo-back-to-file > patch &&
 	git apply --index < patch
 	'
 test_debug 'cat patch'
 
-test_expect_success 'binary file becomes symlink' '
+test_expect_success SYMLINKS 'binary file becomes symlink' '
 	git checkout -f foo-becomes-binary &&
 	git diff-tree -p --binary HEAD foo-symlinked-to-bar > patch &&
 	git apply --index < patch
 	'
 test_debug 'cat patch'
 
-test_expect_success 'symlink becomes binary file' '
+test_expect_success SYMLINKS 'symlink becomes binary file' '
 	git checkout -f foo-symlinked-to-bar &&
 	git diff-tree -p --binary HEAD foo-becomes-binary > patch &&
 	git apply --index < patch
@@ -110,7 +104,7 @@ test_expect_success 'symlink becomes binary file' '
 test_debug 'cat patch'
 
 
-test_expect_success 'symlink becomes directory' '
+test_expect_success SYMLINKS 'symlink becomes directory' '
 	git checkout -f foo-symlinked-to-bar &&
 	git diff-tree -p HEAD foo-becomes-a-directory > patch &&
 	git apply --index < patch
@@ -118,7 +112,7 @@ test_expect_success 'symlink becomes directory' '
 test_debug 'cat patch'
 
 
-test_expect_success 'directory becomes symlink' '
+test_expect_success SYMLINKS 'directory becomes symlink' '
 	git checkout -f foo-becomes-a-directory &&
 	git diff-tree -p HEAD foo-symlinked-to-bar > patch &&
 	git apply --index < patch
diff --git a/t/t4115-apply-symlink.sh b/t/t4115-apply-symlink.sh
index aff4348..7674dd2 100755
--- a/t/t4115-apply-symlink.sh
+++ b/t/t4115-apply-symlink.sh
@@ -9,13 +9,7 @@ test_description='git apply symlinks and partial files
 
 . ./test-lib.sh
 
-if ! test_have_prereq SYMLINKS
-then
-	skip_all='Symbolic links not supported, skipping tests.'
-	test_done
-fi
-
-test_expect_success setup '
+test_expect_success SYMLINKS setup '
 
 	ln -s path1/path2/path3/path4/path5 link1 &&
 	git add link? &&
@@ -34,7 +28,7 @@ test_expect_success setup '
 
 '
 
-test_expect_success 'apply symlink patch' '
+test_expect_success SYMLINKS 'apply symlink patch' '
 
 	git checkout side &&
 	git apply patch &&
@@ -43,7 +37,7 @@ test_expect_success 'apply symlink patch' '
 
 '
 
-test_expect_success 'apply --index symlink patch' '
+test_expect_success SYMLINKS 'apply --index symlink patch' '
 
 	git checkout -f side &&
 	git apply --index patch &&
diff --git a/t/t4122-apply-symlink-inside.sh b/t/t4122-apply-symlink-inside.sh
index 923fcab..3940737 100755
--- a/t/t4122-apply-symlink-inside.sh
+++ b/t/t4122-apply-symlink-inside.sh
@@ -3,12 +3,6 @@
 test_description='apply to deeper directory without getting fooled with symlink'
 . ./test-lib.sh
 
-if ! test_have_prereq SYMLINKS
-then
-	skip_all='Symbolic links not supported, skipping tests.'
-	test_done
-fi
-
 lecho () {
 	for l_
 	do
@@ -16,7 +10,7 @@ lecho () {
 	done
 }
 
-test_expect_success setup '
+test_expect_success SYMLINKS setup '
 
 	mkdir -p arch/i386/boot arch/x86_64 &&
 	lecho 1 2 3 4 5 >arch/i386/boot/Makefile &&
@@ -37,7 +31,7 @@ test_expect_success setup '
 
 '
 
-test_expect_success apply '
+test_expect_success SYMLINKS apply '
 
 	git checkout test &&
 	git diff --exit-code test &&
@@ -46,7 +40,7 @@ test_expect_success apply '
 
 '
 
-test_expect_success 'check result' '
+test_expect_success SYMLINKS 'check result' '
 
 	git diff --exit-code master &&
 	git diff --exit-code --cached master &&
diff --git a/t/t5522-pull-symlink.sh b/t/t5522-pull-symlink.sh
index 298200f..8e9b204 100755
--- a/t/t5522-pull-symlink.sh
+++ b/t/t5522-pull-symlink.sh
@@ -4,12 +4,6 @@ test_description='pulling from symlinked subdir'
 
 . ./test-lib.sh
 
-if ! test_have_prereq SYMLINKS
-then
-	skip_all='Symbolic links not supported, skipping tests.'
-	test_done
-fi
-
 # The scenario we are building:
 #
 #   trash\ directory/
@@ -20,7 +14,7 @@ fi
 #
 # The working directory is subdir-link.
 
-test_expect_success setup '
+test_expect_success SYMLINKS setup '
 	mkdir subdir &&
 	echo file >subdir/file &&
 	git add subdir/file &&
@@ -36,7 +30,7 @@ test_expect_success setup '
 
 # Demonstrate that things work if we just avoid the symlink
 #
-test_expect_success 'pulling from real subdir' '
+test_expect_success SYMLINKS 'pulling from real subdir' '
 	(
 		echo real >subdir/file &&
 		git commit -m real subdir/file &&
@@ -64,7 +58,7 @@ test_expect_success 'pulling from real subdir' '
 # directory.  A POSIX shell's "cd" works a little differently
 # than chdir() in C; "cd -P" is much closer to chdir().
 #
-test_expect_success 'pulling from symlinked subdir' '
+test_expect_success SYMLINKS 'pulling from symlinked subdir' '
 	(
 		echo link >subdir/file &&
 		git commit -m link subdir/file &&
@@ -77,7 +71,7 @@ test_expect_success 'pulling from symlinked subdir' '
 # Prove that the remote end really is a repo, and other commands
 # work fine in this context.  It's just that "git pull" breaks.
 #
-test_expect_success 'pushing from symlinked subdir' '
+test_expect_success SYMLINKS 'pushing from symlinked subdir' '
 	(
 		cd subdir-link/ &&
 		echo push >file &&
diff --git a/t/t6035-merge-dir-to-symlink.sh b/t/t6035-merge-dir-to-symlink.sh
index cd3190c..28e8f2a 100755
--- a/t/t6035-merge-dir-to-symlink.sh
+++ b/t/t6035-merge-dir-to-symlink.sh
@@ -3,13 +3,7 @@
 test_description='merging when a directory was replaced with a symlink'
 . ./test-lib.sh
 
-if ! test_have_prereq SYMLINKS
-then
-	skip_all='Symbolic links not supported, skipping tests.'
-	test_done
-fi
-
-test_expect_success 'create a commit where dir a/b changed to symlink' '
+test_expect_success SYMLINKS 'create a commit where dir a/b changed to symlink' '
 	mkdir -p a/b/c a/b-2/c &&
 	> a/b/c/d &&
 	> a/b-2/c/d &&
@@ -23,7 +17,7 @@ test_expect_success 'create a commit where dir a/b changed to symlink' '
 	git commit -m "dir to symlink"
 '
 
-test_expect_success 'keep a/b-2/c/d across checkout' '
+test_expect_success SYMLINKS 'keep a/b-2/c/d across checkout' '
 	git checkout HEAD^0 &&
 	git reset --hard master &&
 	git rm --cached a/b &&
@@ -32,14 +26,14 @@ test_expect_success 'keep a/b-2/c/d across checkout' '
 	 test -f a/b-2/c/d
 '
 
-test_expect_success 'checkout should not have deleted a/b-2/c/d' '
+test_expect_success SYMLINKS 'checkout should not have deleted a/b-2/c/d' '
 	git checkout HEAD^0 &&
 	git reset --hard master &&
 	 git checkout start^0 &&
 	 test -f a/b-2/c/d
 '
 
-test_expect_success 'setup for merge test' '
+test_expect_success SYMLINKS 'setup for merge test' '
 	git reset --hard &&
 	test -f a/b-2/c/d &&
 	echo x > a/x &&
@@ -48,7 +42,7 @@ test_expect_success 'setup for merge test' '
 	git tag baseline
 '
 
-test_expect_success 'do not lose a/b-2/c/d in merge (resolve)' '
+test_expect_success SYMLINKS 'do not lose a/b-2/c/d in merge (resolve)' '
 	git reset --hard &&
 	git checkout baseline^0 &&
 	git merge -s resolve master &&
@@ -64,7 +58,7 @@ test_expect_failure 'do not lose a/b-2/c/d in merge (recursive)' '
 	test -f a/b-2/c/d
 '
 
-test_expect_success 'setup a merge where dir a/b-2 changed to symlink' '
+test_expect_success SYMLINKS 'setup a merge where dir a/b-2 changed to symlink' '
 	git reset --hard &&
 	git checkout start^0 &&
 	rm -rf a/b-2 &&
@@ -74,7 +68,7 @@ test_expect_success 'setup a merge where dir a/b-2 changed to symlink' '
 	git tag test2
 '
 
-test_expect_success 'merge should not have conflicts (resolve)' '
+test_expect_success SYMLINKS 'merge should not have conflicts (resolve)' '
 	git reset --hard &&
 	git checkout baseline^0 &&
 	git merge -s resolve test2 &&
-- 
1.7.0.4

Re: [PATCH v2 1/5] tests: implicitly skip SYMLINKS tests using <prereq>

[Jonathan Nieder]

Ævar Arnfjörð Bjarmason wrote:

>  t/t2007-checkout-symlink.sh       |   14 ++++----------
>  t/t4011-diff-symlink.sh           |   18 ++++++------------
>  t/t4023-diff-rename-typechange.sh |   14 ++++----------
>  t/t4114-apply-typechange.sh       |   28 +++++++++++-----------------
>  t/t4115-apply-symlink.sh          |   12 +++---------
>  t/t4122-apply-symlink-inside.sh   |   12 +++---------
>  t/t5522-pull-symlink.sh           |   14 ++++----------
>  t/t6035-merge-dir-to-symlink.sh   |   20 +++++++-------------
>  8 files changed, 42 insertions(+), 90 deletions(-)

I like it. :)

[PATCH v2 2/5] t/t4004-diff-rename-symlink.sh: use three-arg <prereq>

[Ævar Arnfjörð Bjarmason]

Change the tests that skipped due to unavailable SYMLINKS support to
use the three-arg prereq form of test_expect_success.

This is like the "tests: implicitly skip SYMLINKS tests using
<prereq>" change, but I needed to create an additional test for some
setup code. It's in a separate change as suggested by Jonathan Nieder
for ease of reviewing.

    Date: Tue, 27 Jul 2010 16:17:37 -0500
    From: Jonathan Nieder <jrnieder@gmail.com>
    Message-ID: <20100727211737.GA11768@burratino>
    In-Reply-To: <1280265254-19642-2-git-send-email-avarab@gmail.com>
    Subject: Re: [PATCH 1/4] tests: implicitly skip SYMLINKS tests using <prereq>

    Ævar Arnfjörð Bjarmason wrote:

    > +++ b/t/t4004-diff-rename-symlink.sh
    > @@ -40,8 +34,9 @@ test_expect_success \
    >  # rezrov and nitfol are rename/copy of frotz and bozbar should be
    >  # a new creation.
    >
    > -GIT_DIFF_OPTS=--unified=0 git diff-index -M -p $tree >current
    > -cat >expected <<\EOF
    > +test_expect_success SYMLINKS 'setup diff output' "
    > +    GIT_DIFF_OPTS=--unified=0 git diff-index -M -p $tree >current
    > +    cat >expected <<\EOF
    >  diff --git a/bozbar b/bozbar
    >  new file mode 120000
    >  --- /dev/null

    Probably belongs in a separate patch.  More importantly, it is missing
    &&-chaining (not a regression, but it is best to set a good example).

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t4004-diff-rename-symlink.sh |   18 +++++++-----------
 1 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/t/t4004-diff-rename-symlink.sh b/t/t4004-diff-rename-symlink.sh
index 1a09e8d..92a65f4 100755
--- a/t/t4004-diff-rename-symlink.sh
+++ b/t/t4004-diff-rename-symlink.sh
@@ -12,13 +12,7 @@ by an edit for them.
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/diff-lib.sh
 
-if ! test_have_prereq SYMLINKS
-then
-	skip_all='Symbolic links not supported, skipping tests.'
-	test_done
-fi
-
-test_expect_success \
+test_expect_success SYMLINKS \
     'prepare reference tree' \
     'echo xyzzy | tr -d '\\\\'012 >yomin &&
      ln -s xyzzy frotz &&
@@ -26,7 +20,7 @@ test_expect_success \
     tree=$(git write-tree) &&
     echo $tree'
 
-test_expect_success \
+test_expect_success SYMLINKS \
     'prepare work tree' \
     'mv frotz rezrov &&
      rm -f yomin &&
@@ -40,8 +34,9 @@ test_expect_success \
 # rezrov and nitfol are rename/copy of frotz and bozbar should be
 # a new creation.
 
-GIT_DIFF_OPTS=--unified=0 git diff-index -M -p $tree >current
-cat >expected <<\EOF
+test_expect_success SYMLINKS 'setup diff output' "
+    GIT_DIFF_OPTS=--unified=0 git diff-index -M -p $tree >current &&
+    cat >expected <<\EOF
 diff --git a/bozbar b/bozbar
 new file mode 120000
 --- /dev/null
@@ -65,8 +60,9 @@ deleted file mode 100644
 -xyzzy
 \ No newline at end of file
 EOF
+"
 
-test_expect_success \
+test_expect_success SYMLINKS \
     'validate diff output' \
     'compare_diff_patch current expected'
 
-- 
1.7.0.4

Re: [PATCH v2 2/5] t/t4004-diff-rename-symlink.sh: use three-arg <prereq>

[Jonathan Nieder]

Ævar Arnfjörð Bjarmason wrote:

> Change the tests that skipped due to unavailable SYMLINKS support to
> use the three-arg prereq form of test_expect_success.
> 
> This is like the "tests: implicitly skip SYMLINKS tests using
> <prereq>" change, but I needed to create an additional test for some
> setup code. It's in a separate change as suggested by Jonathan Nieder
> for ease of reviewing.

Hmm, I still don’t understand this.  Do you mean that there is
some setup that needs to be run before these commands, and so
the patch fails if that change is not included?

Or is it a matter of "while at it, fix this other problem
I noticed" (which would be fine, but it is clearer to
present it as such if so)?

> diff --git a/t/t4004-diff-rename-symlink.sh b/t/t4004-diff-rename-symlink.sh
> index 1a09e8d..92a65f4 100755
> --- a/t/t4004-diff-rename-symlink.sh
> +++ b/t/t4004-diff-rename-symlink.sh
> @@ -40,8 +34,9 @@ test_expect_success \
>  # rezrov and nitfol are rename/copy of frotz and bozbar should be
>  # a new creation.
>  
> -GIT_DIFF_OPTS=--unified=0 git diff-index -M -p $tree >current
> -cat >expected <<\EOF
> +test_expect_success SYMLINKS 'setup diff output' "
> +    GIT_DIFF_OPTS=--unified=0 git diff-index -M -p $tree >current &&
> +    cat >expected <<\EOF
>  diff --git a/bozbar b/bozbar
>  new file mode 120000
>  --- /dev/null
> @@ -65,8 +60,9 @@ deleted file mode 100644
>  -xyzzy
>  \ No newline at end of file
>  EOF
> +"

Tip for the future: if you use 'straight quotes', then readers
can avoid carefully scanning through for $ and similar oddities
(and the test script presented with the "expecting success:"
prompt will use the friendlier $tree instead of 76c98ds89).

The patch looks good; my only remaining concern is the log
message as mentioned above.

Thanks,
Jonathan

Re: [PATCH v2 2/5] t/t4004-diff-rename-symlink.sh: use three-arg <prereq>

[Ævar Arnfjörð Bjarmason]

On Thu, Jul 29, 2010 at 01:09, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ævar Arnfjörð Bjarmason wrote:
>
>> Change the tests that skipped due to unavailable SYMLINKS support to
>> use the three-arg prereq form of test_expect_success.
>>
>> This is like the "tests: implicitly skip SYMLINKS tests using
>> <prereq>" change, but I needed to create an additional test for some
>> setup code. It's in a separate change as suggested by Jonathan Nieder
>> for ease of reviewing.
>
> Hmm, I still don’t understand this.  Do you mean that there is
> some setup that needs to be run before these commands, and so
> the patch fails if that change is not included?
>
> Or is it a matter of "while at it, fix this other problem
> I noticed" (which would be fine, but it is clearer to
> present it as such if so)?

The setup code needs to be inside a test so that it'll only run if we
have SYMLINKS support.

I could also have done:

    if test_have_prereq PERL
    then
        ..setup code..
    fi

But setup code should be inside tests, so that we'll get failure
reporting.

>> diff --git a/t/t4004-diff-rename-symlink.sh b/t/t4004-diff-rename-symlink.sh
>> index 1a09e8d..92a65f4 100755
>> --- a/t/t4004-diff-rename-symlink.sh
>> +++ b/t/t4004-diff-rename-symlink.sh
>> @@ -40,8 +34,9 @@ test_expect_success \
>>  # rezrov and nitfol are rename/copy of frotz and bozbar should be
>>  # a new creation.
>>
>> -GIT_DIFF_OPTS=--unified=0 git diff-index -M -p $tree >current
>> -cat >expected <<\EOF
>> +test_expect_success SYMLINKS 'setup diff output' "
>> +    GIT_DIFF_OPTS=--unified=0 git diff-index -M -p $tree >current &&
>> +    cat >expected <<\EOF
>>  diff --git a/bozbar b/bozbar
>>  new file mode 120000
>>  --- /dev/null
>> @@ -65,8 +60,9 @@ deleted file mode 100644
>>  -xyzzy
>>  \ No newline at end of file
>>  EOF
>> +"
>
> Tip for the future: if you use 'straight quotes', then readers
> can avoid carefully scanning through for $ and similar oddities
> (and the test script presented with the "expecting success:"
> prompt will use the friendlier $tree instead of 76c98ds89).

Ah, thanks.

> The patch looks good; my only remaining concern is the log
> message as mentioned above.

Hopefully that's cleared up now.

[PATCH v2 3/5] t/t5800-remote-helpers.sh: Skip with prereq on python <2.4

[Ævar Arnfjörð Bjarmason]

Change the t/t5800-remote-helpers.sh test to skip with the the
three-arg prereq form of test_expect_success instead of bailing out.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5800-remote-helpers.sh |   24 +++++++++++-------------
 1 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh
index 637d8e9..1fb6380 100755
--- a/t/t5800-remote-helpers.sh
+++ b/t/t5800-remote-helpers.sh
@@ -13,13 +13,11 @@ if sys.hexversion < 0x02040000:
     sys.exit(1)
 '
 then
-	:
-else
-	skip_all='skipping git remote-testgit tests: requires Python 2.4 or newer'
-	test_done
+    # Requires Python 2.4 or newer
+	test_set_prereq PYTHON_24
 fi
 
-test_expect_success 'setup repository' '
+test_expect_success PYTHON_24 'setup repository' '
 	git init --bare server/.git &&
 	git clone server public &&
 	(cd public &&
@@ -29,34 +27,34 @@ test_expect_success 'setup repository' '
 	 git push origin master)
 '
 
-test_expect_success 'cloning from local repo' '
+test_expect_success PYTHON_24 'cloning from local repo' '
 	git clone "testgit::${PWD}/server" localclone &&
 	test_cmp public/file localclone/file
 '
 
-test_expect_success 'cloning from remote repo' '
+test_expect_success PYTHON_24 'cloning from remote repo' '
 	git clone "testgit::file://${PWD}/server" clone &&
 	test_cmp public/file clone/file
 '
 
-test_expect_success 'create new commit on remote' '
+test_expect_success PYTHON_24 'create new commit on remote' '
 	(cd public &&
 	 echo content >>file &&
 	 git commit -a -m two &&
 	 git push)
 '
 
-test_expect_success 'pulling from local repo' '
+test_expect_success PYTHON_24 'pulling from local repo' '
 	(cd localclone && git pull) &&
 	test_cmp public/file localclone/file
 '
 
-test_expect_success 'pulling from remote remote' '
+test_expect_success PYTHON_24 'pulling from remote remote' '
 	(cd clone && git pull) &&
 	test_cmp public/file clone/file
 '
 
-test_expect_success 'pushing to local repo' '
+test_expect_success PYTHON_24 'pushing to local repo' '
 	(cd localclone &&
 	echo content >>file &&
 	git commit -a -m three &&
@@ -65,12 +63,12 @@ test_expect_success 'pushing to local repo' '
 	test $HEAD = $(git --git-dir=server/.git rev-parse --verify HEAD)
 '
 
-test_expect_success 'synch with changes from localclone' '
+test_expect_success PYTHON_24 'synch with changes from localclone' '
 	(cd clone &&
 	 git pull)
 '
 
-test_expect_success 'pushing remote local repo' '
+test_expect_success PYTHON_24 'pushing remote local repo' '
 	(cd clone &&
 	echo content >>file &&
 	git commit -a -m four &&
-- 
1.7.0.4

Re: [PATCH v2 3/5] t/t5800-remote-helpers.sh: Skip with prereq on python <2.4

[Jonathan Nieder]

Ævar Arnfjörð Bjarmason wrote:

> Change the t/t5800-remote-helpers.sh test to skip with the the
> three-arg prereq form of test_expect_success instead of bailing out.

Right:

> +++ b/t/t5800-remote-helpers.sh
> @@ -13,13 +13,11 @@ if sys.hexversion < 0x02040000:
>      sys.exit(1)
>  '
>  then
> -	:
> -else
> -	skip_all='skipping git remote-testgit tests: requires Python 2.4 or newer'
> -	test_done
> +    # Requires Python 2.4 or newer
> +	test_set_prereq PYTHON_24
>  fi

And as a bonus, this makes the test clearer.  Nice.

I would suggest dropping the "Requires Python 2.4" comment, or
maybe moving it somewhere else with a change to explain that
it is git-remote-testgit that requires Python ≥ 2.4
(rather than the remote helper facility in general).

The rest of the series looks good to me.

Thanks again,
Jonathan

Re: [PATCH v2 3/5] t/t5800-remote-helpers.sh: Skip with prereq on python <2.4

[Ævar Arnfjörð Bjarmason]

On Thu, Jul 29, 2010 at 01:16, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ævar Arnfjörð Bjarmason wrote:
>
>> Change the t/t5800-remote-helpers.sh test to skip with the the
>> three-arg prereq form of test_expect_success instead of bailing out.
>
> Right:
>
>> +++ b/t/t5800-remote-helpers.sh
>> @@ -13,13 +13,11 @@ if sys.hexversion < 0x02040000:
>>      sys.exit(1)
>>  '
>>  then
>> -     :
>> -else
>> -     skip_all='skipping git remote-testgit tests: requires Python 2.4 or newer'
>> -     test_done
>> +    # Requires Python 2.4 or newer
>> +     test_set_prereq PYTHON_24
>>  fi
>
> And as a bonus, this makes the test clearer.  Nice.

Yup, <prereq> is really nice in all regards.

> I would suggest dropping the "Requires Python 2.4" comment, or
> maybe moving it somewhere else with a change to explain that
> it is git-remote-testgit that requires Python ≥ 2.4
> (rather than the remote helper facility in general).

I put it there to make the connection between 24, 0x02040000, and 2.4.

I think trying to avoid confusion like that isn't a problem in a test
file, it's pretty obvious from the context that the prereq only has to
do with the test. Someone looking up remote helpers is going to do
that in the documentation, not in some t/ test file.

Thanks for your continuing & detailed reviews b.t.w.

[PATCH v2 4/5] t/t7800-difftool.sh: Skip with prereq on no PERL

[Ævar Arnfjörð Bjarmason]

Change t/t7800-difftool.sh to to skip with the the three-arg prereq
form of test_expect_success instead of bailing out.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t7800-difftool.sh |   49 ++++++++++++++++++++++---------------------------
 1 files changed, 22 insertions(+), 27 deletions(-)

diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 196827e..58dc6f6 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -10,11 +10,6 @@ Testing basic diff tool invocation
 
 . ./test-lib.sh
 
-if ! test_have_prereq PERL; then
-	skip_all='skipping difftool tests, perl not available'
-	test_done
-fi
-
 LF='
 '
 
@@ -50,7 +45,7 @@ prompt_given()
 }
 
 # Create a file on master and change it on branch
-test_expect_success 'setup' '
+test_expect_success PERL 'setup' '
 	echo master >file &&
 	git add file &&
 	git commit -m "added file" &&
@@ -62,7 +57,7 @@ test_expect_success 'setup' '
 '
 
 # Configure a custom difftool.<tool>.cmd and use it
-test_expect_success 'custom commands' '
+test_expect_success PERL 'custom commands' '
 	restore_test_defaults &&
 	git config difftool.test-tool.cmd "cat \$REMOTE" &&
 
@@ -75,13 +70,13 @@ test_expect_success 'custom commands' '
 '
 
 # Ensures that git-difftool ignores bogus --tool values
-test_expect_success 'difftool ignores bad --tool values' '
+test_expect_success PERL 'difftool ignores bad --tool values' '
 	diff=$(git difftool --no-prompt --tool=bad-tool branch)
 	test "$?" = 1 &&
 	test "$diff" = ""
 '
 
-test_expect_success 'difftool honors --gui' '
+test_expect_success PERL 'difftool honors --gui' '
 	git config merge.tool bogus-tool &&
 	git config diff.tool bogus-tool &&
 	git config diff.guitool test-tool &&
@@ -92,7 +87,7 @@ test_expect_success 'difftool honors --gui' '
 	restore_test_defaults
 '
 
-test_expect_success 'difftool --gui works without configured diff.guitool' '
+test_expect_success PERL 'difftool --gui works without configured diff.guitool' '
 	git config diff.tool test-tool &&
 
 	diff=$(git difftool --no-prompt --gui branch) &&
@@ -102,7 +97,7 @@ test_expect_success 'difftool --gui works without configured diff.guitool' '
 '
 
 # Specify the diff tool using $GIT_DIFF_TOOL
-test_expect_success 'GIT_DIFF_TOOL variable' '
+test_expect_success PERL 'GIT_DIFF_TOOL variable' '
 	git config --unset diff.tool
 	GIT_DIFF_TOOL=test-tool &&
 	export GIT_DIFF_TOOL &&
@@ -115,7 +110,7 @@ test_expect_success 'GIT_DIFF_TOOL variable' '
 
 # Test the $GIT_*_TOOL variables and ensure
 # that $GIT_DIFF_TOOL always wins unless --tool is specified
-test_expect_success 'GIT_DIFF_TOOL overrides' '
+test_expect_success PERL 'GIT_DIFF_TOOL overrides' '
 	git config diff.tool bogus-tool &&
 	git config merge.tool bogus-tool &&
 
@@ -136,7 +131,7 @@ test_expect_success 'GIT_DIFF_TOOL overrides' '
 
 # Test that we don't have to pass --no-prompt to difftool
 # when $GIT_DIFFTOOL_NO_PROMPT is true
-test_expect_success 'GIT_DIFFTOOL_NO_PROMPT variable' '
+test_expect_success PERL 'GIT_DIFFTOOL_NO_PROMPT variable' '
 	GIT_DIFFTOOL_NO_PROMPT=true &&
 	export GIT_DIFFTOOL_NO_PROMPT &&
 
@@ -148,7 +143,7 @@ test_expect_success 'GIT_DIFFTOOL_NO_PROMPT variable' '
 
 # git-difftool supports the difftool.prompt variable.
 # Test that GIT_DIFFTOOL_PROMPT can override difftool.prompt = false
-test_expect_success 'GIT_DIFFTOOL_PROMPT variable' '
+test_expect_success PERL 'GIT_DIFFTOOL_PROMPT variable' '
 	git config difftool.prompt false &&
 	GIT_DIFFTOOL_PROMPT=true &&
 	export GIT_DIFFTOOL_PROMPT &&
@@ -160,7 +155,7 @@ test_expect_success 'GIT_DIFFTOOL_PROMPT variable' '
 '
 
 # Test that we don't have to pass --no-prompt when difftool.prompt is false
-test_expect_success 'difftool.prompt config variable is false' '
+test_expect_success PERL 'difftool.prompt config variable is false' '
 	git config difftool.prompt false &&
 
 	diff=$(git difftool branch) &&
@@ -170,7 +165,7 @@ test_expect_success 'difftool.prompt config variable is false' '
 '
 
 # Test that we don't have to pass --no-prompt when mergetool.prompt is false
-test_expect_success 'difftool merge.prompt = false' '
+test_expect_success PERL 'difftool merge.prompt = false' '
 	git config --unset difftool.prompt
 	git config mergetool.prompt false &&
 
@@ -181,7 +176,7 @@ test_expect_success 'difftool merge.prompt = false' '
 '
 
 # Test that the -y flag can override difftool.prompt = true
-test_expect_success 'difftool.prompt can overridden with -y' '
+test_expect_success PERL 'difftool.prompt can overridden with -y' '
 	git config difftool.prompt true &&
 
 	diff=$(git difftool -y branch) &&
@@ -191,7 +186,7 @@ test_expect_success 'difftool.prompt can overridden with -y' '
 '
 
 # Test that the --prompt flag can override difftool.prompt = false
-test_expect_success 'difftool.prompt can overridden with --prompt' '
+test_expect_success PERL 'difftool.prompt can overridden with --prompt' '
 	git config difftool.prompt false &&
 
 	prompt=$(echo | git difftool --prompt branch | tail -1) &&
@@ -201,7 +196,7 @@ test_expect_success 'difftool.prompt can overridden with --prompt' '
 '
 
 # Test that the last flag passed on the command-line wins
-test_expect_success 'difftool last flag wins' '
+test_expect_success PERL 'difftool last flag wins' '
 	diff=$(git difftool --prompt --no-prompt branch) &&
 	test "$diff" = "branch" &&
 
@@ -215,7 +210,7 @@ test_expect_success 'difftool last flag wins' '
 
 # git-difftool falls back to git-mergetool config variables
 # so test that behavior here
-test_expect_success 'difftool + mergetool config variables' '
+test_expect_success PERL 'difftool + mergetool config variables' '
 	remove_config_vars
 	git config merge.tool test-tool &&
 	git config mergetool.test-tool.cmd "cat \$LOCAL" &&
@@ -233,7 +228,7 @@ test_expect_success 'difftool + mergetool config variables' '
 	restore_test_defaults
 '
 
-test_expect_success 'difftool.<tool>.path' '
+test_expect_success PERL 'difftool.<tool>.path' '
 	git config difftool.tkdiff.path echo &&
 	diff=$(git difftool --tool=tkdiff --no-prompt branch) &&
 	git config --unset difftool.tkdiff.path &&
@@ -243,32 +238,32 @@ test_expect_success 'difftool.<tool>.path' '
 	restore_test_defaults
 '
 
-test_expect_success 'difftool --extcmd=cat' '
+test_expect_success PERL 'difftool --extcmd=cat' '
 	diff=$(git difftool --no-prompt --extcmd=cat branch) &&
 	test "$diff" = branch"$LF"master
 '
 
-test_expect_success 'difftool --extcmd cat' '
+test_expect_success PERL 'difftool --extcmd cat' '
 	diff=$(git difftool --no-prompt --extcmd cat branch) &&
 	test "$diff" = branch"$LF"master
 '
 
-test_expect_success 'difftool -x cat' '
+test_expect_success PERL 'difftool -x cat' '
 	diff=$(git difftool --no-prompt -x cat branch) &&
 	test "$diff" = branch"$LF"master
 '
 
-test_expect_success 'difftool --extcmd echo arg1' '
+test_expect_success PERL 'difftool --extcmd echo arg1' '
 	diff=$(git difftool --no-prompt --extcmd sh\ -c\ \"echo\ \$1\" branch)
 	test "$diff" = file
 '
 
-test_expect_success 'difftool --extcmd cat arg1' '
+test_expect_success PERL 'difftool --extcmd cat arg1' '
 	diff=$(git difftool --no-prompt --extcmd sh\ -c\ \"cat\ \$1\" branch)
 	test "$diff" = master
 '
 
-test_expect_success 'difftool --extcmd cat arg2' '
+test_expect_success PERL 'difftool --extcmd cat arg2' '
 	diff=$(git difftool --no-prompt --extcmd sh\ -c\ \"cat\ \$2\" branch)
 	test "$diff" = branch
 '
-- 
1.7.0.4

[PATCH v2 5/5] t/README: Update "Skipping tests" to align with best practices

[Ævar Arnfjörð Bjarmason]

The example I initially added to "Skipping tests" wasn't very
good. We'd rather skip tests using the three-arg prereq form to the
test_* functions, not bail out with a skip message.

Change the documentation to reflect that, but retain the bailout
example under a disclaimer which explains that it's probably not a
good idea to use it.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/README |   21 ++++++++++++++++++---
 1 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/t/README b/t/README
index 0d1183c..12b714b 100644
--- a/t/README
+++ b/t/README
@@ -307,9 +307,21 @@ Keep in mind:
 Skipping tests
 --------------
 
-If you need to skip all the remaining tests you should set skip_all
-and immediately call test_done. The string you give to skip_all will
-be used as an explanation for why the test was skipped. for instance:
+If you need to skip tests you should do so be using the three-arg form
+of the test_* functions (see the "Test harness library" section
+below), e.g.:
+
+    test_expect_success PERL 'I need Perl' "
+        '$PERL_PATH' -e 'hlagh() if unf_unf()'
+    "
+
+The advantage of skipping tests like this is that platforms that don't
+have the PERL and other optional dependencies get an indication of how
+many tests they're missing.
+
+If the test code is too hairy for that (i.e. does a lot of setup work
+outside test assertions) you can also skip all remaining tests by
+setting skip_all and immediately call test_done:
 
 	if ! test_have_prereq PERL
 	then
@@ -317,6 +329,9 @@ be used as an explanation for why the test was skipped. for instance:
 	    test_done
 	fi
 
+The string you give to skip_all will be used as an explanation for why
+the test was skipped.
+
 End with test_done
 ------------------
 
-- 
1.7.0.4