[PATCH 0/7] A few minor test-prereq updates

[PATCH 0/7] A few minor test-prereq updates

[Junio C Hamano]

While reviewing somebody's patch, I noticed that individual test
scripts set EXPENSIVE test prerequisite with copied-and-pasted
lines.  Here is a bit to update them, while fixing styles in old
test scripts that had these copied-and-pasted lines.

The last step discards support for GIT_{NOTES,PATCHID}_TIMING_TESTS
environment variables to enable EXPENSIVE tests in two test scripts,
which *is* a backward incompatible change and people may want to
argue against it (but I of course thought it is a good change and
that is why it is included).

Junio C Hamano (7):
  test: turn EXPENSIVE into a lazy prerequisite
  test: turn USR_BIN_TIME into a lazy prerequisite
  t3302: coding style updates
  t3302: do not chdir around in the primary test process
  t3302: drop unnecessary NOT_EXPENSIVE pseudo-prerequisite
  t3419: drop unnecessary NOT_EXPENSIVE pseudo-prerequisite
  tests: drop GIT_*_TIMING_TESTS environment variable support

 t/t0021-conversion.sh            |   2 -
 t/t3302-notes-index-expensive.sh | 130 +++++++++++++++++++++------------------
 t/t3419-rebase-patch-id.sh       |  25 +++-----
 t/t5551-http-fetch.sh            |   2 -
 t/test-lib.sh                    |   8 +++
 5 files changed, 85 insertions(+), 82 deletions(-)

-- 
2.0.0-435-g307a092

[PATCH 1/7] test: turn EXPENSIVE into a lazy prerequisite

[Junio C Hamano]

Two test scripts (t0021 and t5551) had copy & paste code to set
EXPENSIVE prerequisite.  Use the test_lazy_prereq helper to define
them in the common t/test-lib.sh.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t0021-conversion.sh | 2 --
 t/t5551-http-fetch.sh | 2 --
 t/test-lib.sh         | 4 ++++
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index b92e6cb..f890c54 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -190,8 +190,6 @@ test_expect_success 'required filter clean failure' '
 	test_must_fail git add test.fc
 '
 
-test -n "$GIT_TEST_LONG" && test_set_prereq EXPENSIVE
-
 test_expect_success EXPENSIVE 'filter large file' '
 	git config filter.largefile.smudge cat &&
 	git config filter.largefile.clean cat &&
diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh
index afb439e..d697393 100755
--- a/t/t5551-http-fetch.sh
+++ b/t/t5551-http-fetch.sh
@@ -214,8 +214,6 @@ test_expect_success 'cookies stored in http.cookiefile when http.savecookies set
 	test_cmp expect_cookies.txt cookies_tail.txt
 '
 
-test -n "$GIT_TEST_LONG" && test_set_prereq EXPENSIVE
-
 test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '
 	(
 	cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index b25249e..d70d05e 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -855,6 +855,10 @@ test_lazy_prereq AUTOIDENT '
 	git var GIT_AUTHOR_IDENT
 '
 
+test_lazy_prereq EXPENSIVE '
+	test -n "$GIT_TEST_LONG"
+'
+
 # When the tests are run as root, permission tests will report that
 # things are writable when they shouldn't be.
 test -w / || test_set_prereq SANITY
-- 
2.0.0-435-g307a092

[PATCH 2/7] test: turn USR_BIN_TIME into a lazy prerequisite

[Junio C Hamano]

Two test scripts (t3302 and t3419) had copy & paste code to set
USR_BIN_TIME prerequisite.  Use the test_lazy_prereq helper to define
them in the common t/test-lib.sh.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t3302-notes-index-expensive.sh | 1 -
 t/t3419-rebase-patch-id.sh       | 1 -
 t/test-lib.sh                    | 4 ++++
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/t3302-notes-index-expensive.sh b/t/t3302-notes-index-expensive.sh
index e35d781..dc706ab 100755
--- a/t/t3302-notes-index-expensive.sh
+++ b/t/t3302-notes-index-expensive.sh
@@ -9,7 +9,6 @@ test_description='Test commit notes index (expensive!)'
 
 test_set_prereq NOT_EXPENSIVE
 test -n "$GIT_NOTES_TIMING_TESTS" && test_set_prereq EXPENSIVE
-test -x /usr/bin/time && test_set_prereq USR_BIN_TIME
 
 create_repo () {
 	number_of_commits=$1
diff --git a/t/t3419-rebase-patch-id.sh b/t/t3419-rebase-patch-id.sh
index e70ac10..08e30b3 100755
--- a/t/t3419-rebase-patch-id.sh
+++ b/t/t3419-rebase-patch-id.sh
@@ -6,7 +6,6 @@ test_description='git rebase - test patch id computation'
 
 test_set_prereq NOT_EXPENSIVE
 test -n "$GIT_PATCHID_TIMING_TESTS" && test_set_prereq EXPENSIVE
-test -x /usr/bin/time && test_set_prereq USR_BIN_TIME
 
 count()
 {
diff --git a/t/test-lib.sh b/t/test-lib.sh
index d70d05e..884c57c 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -859,6 +859,10 @@ test_lazy_prereq EXPENSIVE '
 	test -n "$GIT_TEST_LONG"
 '
 
+test_lazy_prereq USR_BIN_TIME '
+	test -x /usr/bin/time
+'
+
 # When the tests are run as root, permission tests will report that
 # things are writable when they shouldn't be.
 test -w / || test_set_prereq SANITY
-- 
2.0.0-435-g307a092

[PATCH 3/7] t3302: coding style updates

[Junio C Hamano]

Use "<<-END_OF_HERE_TEXT" to push the contents of here-text to the
right in order to show the loop structure better.

Use write_script when writing a script to be run.

Use "test" (not "[ ... ]") and avoid unnecessary ";" in the middle
of a line.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t3302-notes-index-expensive.sh | 90 +++++++++++++++++++++-------------------
 1 file changed, 47 insertions(+), 43 deletions(-)

diff --git a/t/t3302-notes-index-expensive.sh b/t/t3302-notes-index-expensive.sh
index dc706ab..aa9dbd7 100755
--- a/t/t3302-notes-index-expensive.sh
+++ b/t/t3302-notes-index-expensive.sh
@@ -16,43 +16,43 @@ create_repo () {
 	test -d .git || {
 	git init &&
 	(
-		while [ $nr -lt $number_of_commits ]; do
+		while test $nr -lt $number_of_commits
+		do
 			nr=$(($nr+1))
 			mark=$(($nr+$nr))
 			notemark=$(($mark+1))
 			test_tick &&
-			cat <<INPUT_END &&
-commit refs/heads/master
-mark :$mark
-committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
-data <<COMMIT
-commit #$nr
-COMMIT
-
-M 644 inline file
-data <<EOF
-file in commit #$nr
-EOF
-
-blob
-mark :$notemark
-data <<EOF
-note for commit #$nr
-EOF
-
-INPUT_END
-
-			echo "N :$notemark :$mark" >> note_commit
+			cat <<-INPUT_END &&
+			commit refs/heads/master
+			mark :$mark
+			committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+			data <<COMMIT
+			commit #$nr
+			COMMIT
+
+			M 644 inline file
+			data <<EOF
+			file in commit #$nr
+			EOF
+
+			blob
+			mark :$notemark
+			data <<EOF
+			note for commit #$nr
+			EOF
+
+			INPUT_END
+			echo "N :$notemark :$mark" >>note_commit
 		done &&
 		test_tick &&
-		cat <<INPUT_END &&
-commit refs/notes/commits
-committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
-data <<COMMIT
-notes
-COMMIT
+		cat <<-INPUT_END &&
+		commit refs/notes/commits
+		committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+		data <<COMMIT
+		notes
+		COMMIT
 
-INPUT_END
+		INPUT_END
 
 		cat note_commit
 	) |
@@ -64,38 +64,41 @@ INPUT_END
 test_notes () {
 	count=$1 &&
 	git config core.notesRef refs/notes/commits &&
-	git log | grep "^    " > output &&
+	git log | grep "^    " >output &&
 	i=$count &&
-	while [ $i -gt 0 ]; do
+	while test $i -gt 0
+	do
 		echo "    commit #$i" &&
 		echo "    note for commit #$i" &&
-		i=$(($i-1));
-	done > expect &&
+		i=$(($i-1))
+	done >expect &&
 	test_cmp expect output
 }
 
-cat > time_notes << \EOF
+write_script time_notes <<\EOF
 	mode=$1
 	i=1
-	while [ $i -lt $2 ]; do
+	while test $i -lt $2
+	do
 		case $1 in
 		no-notes)
-			GIT_NOTES_REF=non-existing; export GIT_NOTES_REF
-		;;
+			GIT_NOTES_REF=non-existing
+			export GIT_NOTES_REF
+			;;
 		notes)
 			unset GIT_NOTES_REF
-		;;
+			;;
 		esac
-		git log >/dev/null
+		git log
 		i=$(($i+1))
-	done
+	done >/dev/null
 EOF
 
 time_notes () {
 	for mode in no-notes notes
 	do
 		echo $mode
-		/usr/bin/time "$SHELL_PATH" ../time_notes $mode $1
+		/usr/bin/time ../time_notes $mode $1
 	done
 }
 
@@ -118,7 +121,8 @@ do_tests () {
 }
 
 do_tests NOT_EXPENSIVE 10
-for count in 100 1000 10000; do
+for count in 100 1000 10000
+do
 	do_tests EXPENSIVE $count
 done
 
-- 
2.0.0-435-g307a092

[PATCH 4/7] t3302: do not chdir around in the primary test process

[Junio C Hamano]

These days^Wyears we strive to do stuff in subdirectories inside
subshells to avoid mistakes.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t3302-notes-index-expensive.sh | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/t/t3302-notes-index-expensive.sh b/t/t3302-notes-index-expensive.sh
index aa9dbd7..7712cf3 100755
--- a/t/t3302-notes-index-expensive.sh
+++ b/t/t3302-notes-index-expensive.sh
@@ -106,18 +106,27 @@ do_tests () {
 	pr=$1
 	count=$2
 
-	test_expect_success $pr 'setup / mkdir' '
-		mkdir $count &&
-		cd $count
+	test_expect_success $pr "setup $count" '
+		mkdir "$count" &&
+		(
+			cd "$count" &&
+			create_repo "$count"
+		)
 	'
 
-	test_expect_success $pr "setup $count" "create_repo $count"
-
-	test_expect_success $pr 'notes work' "test_notes $count"
-
-	test_expect_success USR_BIN_TIME,$pr 'notes timing with /usr/bin/time' "time_notes 100"
+	test_expect_success $pr 'notes work' '
+		(
+			cd "$count" &&
+			test_notes "$count"
+		)
+	'
 
-	test_expect_success $pr 'teardown / cd ..' 'cd ..'
+	test_expect_success USR_BIN_TIME,$pr 'notes timing with /usr/bin/time' '
+		(
+			cd "$count" &&
+			time_notes 100
+		)
+	'
 }
 
 do_tests NOT_EXPENSIVE 10
-- 
2.0.0-435-g307a092

[PATCH 5/7] t3302: drop unnecessary NOT_EXPENSIVE pseudo-prerequisite

[Junio C Hamano]

This was only necessary because do_tests helper the script defines
took its parameters in a wrong order.  Just pass an empty string (or
not passing the optional EXPENSIVE prerequisite) when running the
test with a light-weight set of parameters and have the shell do the
right thing when parsing test_expect_success helper.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t3302-notes-index-expensive.sh | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/t/t3302-notes-index-expensive.sh b/t/t3302-notes-index-expensive.sh
index 7712cf3..8d44e04 100755
--- a/t/t3302-notes-index-expensive.sh
+++ b/t/t3302-notes-index-expensive.sh
@@ -7,7 +7,6 @@ test_description='Test commit notes index (expensive!)'
 
 . ./test-lib.sh
 
-test_set_prereq NOT_EXPENSIVE
 test -n "$GIT_NOTES_TIMING_TESTS" && test_set_prereq EXPENSIVE
 
 create_repo () {
@@ -103,8 +102,7 @@ time_notes () {
 }
 
 do_tests () {
-	pr=$1
-	count=$2
+	count=$1 pr=${2-}
 
 	test_expect_success $pr "setup $count" '
 		mkdir "$count" &&
@@ -121,7 +119,7 @@ do_tests () {
 		)
 	'
 
-	test_expect_success USR_BIN_TIME,$pr 'notes timing with /usr/bin/time' '
+	test_expect_success "USR_BIN_TIME${pr:+,$pr}" 'notes timing with /usr/bin/time' '
 		(
 			cd "$count" &&
 			time_notes 100
@@ -129,10 +127,10 @@ do_tests () {
 	'
 }
 
-do_tests NOT_EXPENSIVE 10
+do_tests 10
 for count in 100 1000 10000
 do
-	do_tests EXPENSIVE $count
+	do_tests "$count" EXPENSIVE
 done
 
 test_done
-- 
2.0.0-435-g307a092

[PATCH 6/7] t3419: drop unnecessary NOT_EXPENSIVE pseudo-prerequisite

[Junio C Hamano]

This was only necessary because do_tests helper the script defines
took its parameters in a wrong order.  Just pass an empty string (or
not passing the optional EXPENSIVE prerequisite) when running the
test with a light-weight set of parameters and have the shell do the
right thing when parsing test_expect_success helper.

Also update coding style while we are at it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t3419-rebase-patch-id.sh | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/t/t3419-rebase-patch-id.sh b/t/t3419-rebase-patch-id.sh
index 08e30b3..9292b49 100755
--- a/t/t3419-rebase-patch-id.sh
+++ b/t/t3419-rebase-patch-id.sh
@@ -4,11 +4,9 @@ test_description='git rebase - test patch id computation'
 
 . ./test-lib.sh
 
-test_set_prereq NOT_EXPENSIVE
 test -n "$GIT_PATCHID_TIMING_TESTS" && test_set_prereq EXPENSIVE
 
-count()
-{
+count () {
 	i=0
 	while test $i -lt $1
 	do
@@ -17,8 +15,7 @@ count()
 	done
 }
 
-scramble()
-{
+scramble () {
 	i=0
 	while read x
 	do
@@ -27,12 +24,11 @@ scramble()
 			echo "$x"
 		fi
 		i=$((($i+1) % 10))
-	done < "$1" > "$1.new"
+	done <"$1" >"$1.new"
 	mv -f "$1.new" "$1"
 }
 
-run()
-{
+run () {
 	echo \$ "$@"
 	/usr/bin/time "$@" >/dev/null
 }
@@ -42,10 +38,8 @@ test_expect_success 'setup' '
 	git tag root
 '
 
-do_tests()
-{
-	pr=$1
-	nlines=$2
+do_tests () {
+	nlines=$1 pr=${2-}
 
 	test_expect_success $pr "setup: $nlines lines" "
 		rm -f .gitattributes &&
@@ -102,7 +96,7 @@ do_tests()
 	"
 }
 
-do_tests NOT_EXPENSIVE 500
-do_tests EXPENSIVE 50000
+do_tests 500
+do_tests 50000 EXPENSIVE
 
 test_done
-- 
2.0.0-435-g307a092

[PATCH 7/7] tests: drop GIT_*_TIMING_TESTS environment variable support

[Junio C Hamano]

Two tests (t3302 and t3419) used to have their own environment
variable to trigger expensive tests without enabling expensive
tests in other scripts; a user could set GIT_NOTES_TIMING_TESTS
but not GIT_TEST_LONG and run the whole test suite and trigger
expensive tests only in t3302 but not other tests.  The same for
GIT_PATCHID_TIMING_TESTS in t3419.

While this may have seemed a good flexibility, in reality if you are
concentrating on a single test (e.g. t3302), you can just run that
single test with the GIT_TEST_LONG to trigger expensive tests.  It
does not seem worth forcing other people who may want to come up
with their own expesive tests to invent new environment variables by
keeping this convention.

Drop them.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t3302-notes-index-expensive.sh | 2 --
 t/t3419-rebase-patch-id.sh       | 2 --
 2 files changed, 4 deletions(-)

diff --git a/t/t3302-notes-index-expensive.sh b/t/t3302-notes-index-expensive.sh
index 8d44e04..7217c5e 100755
--- a/t/t3302-notes-index-expensive.sh
+++ b/t/t3302-notes-index-expensive.sh
@@ -7,8 +7,6 @@ test_description='Test commit notes index (expensive!)'
 
 . ./test-lib.sh
 
-test -n "$GIT_NOTES_TIMING_TESTS" && test_set_prereq EXPENSIVE
-
 create_repo () {
 	number_of_commits=$1
 	nr=0
diff --git a/t/t3419-rebase-patch-id.sh b/t/t3419-rebase-patch-id.sh
index 9292b49..217dd79 100755
--- a/t/t3419-rebase-patch-id.sh
+++ b/t/t3419-rebase-patch-id.sh
@@ -4,8 +4,6 @@ test_description='git rebase - test patch id computation'
 
 . ./test-lib.sh
 
-test -n "$GIT_PATCHID_TIMING_TESTS" && test_set_prereq EXPENSIVE
-
 count () {
 	i=0
 	while test $i -lt $1
-- 
2.0.0-435-g307a092

Re: [PATCH 2/7] test: turn USR_BIN_TIME into a lazy prerequisite

[Jeff King]

On Mon, Jun 09, 2014 at 04:22:50PM -0700, Junio C Hamano wrote:

> Two test scripts (t3302 and t3419) had copy & paste code to set
> USR_BIN_TIME prerequisite.  Use the test_lazy_prereq helper to define
> them in the common t/test-lib.sh.

I have to wonder why we even have such a prereq in the first place.

Many tests which use EXPENSIVE are sensible; we used to do something bad
with large values, we fixed it, and want to be able to test the
correctness of the fix. The use in t0021 looks like that.

However, the tests in t3302 and t3419, which use both EXPENSIVE and
USR_BIN_TIME, seem like they are really interested in performance
testing. I think they probably should be in t/perf in the first place
(the reason they are not is presumably because they predate it).

Other than that, your series looks fine.

-Peff

PS s/expesive/expensive/ in the commit message of 7/7