Re: [PATCH] t9301-fast-export: move unset of config variable into its own test function

Junio C Hamano [off-list ref] writes:

For this particular case, what we are interested in testing is not that
"config --unset" exits with 0 status.  We are however interested in making
sure that i18n.commitencoding is not set when the body of #12 runs.

So I think a more appropriate change would be something like this for this
particular case.

Having said that, we may want to have an easier way to exclude certain
classes of pieces, and also encourage test writers to group pieces that
are related to these classes together.

For example, this introduces a new environment you can set,
GIT_SKIP_TEST_CLASS, which is a space separated list of classes of
features that you would want to exclude from the test.
test_expect_success/failure can now take an optional "class token" as the
first parameter (they traditionally took only two parameters, but with
class token, they take three).

This example defines I18N class, and lets you exclude the one you were
manually excluding with "GIT_SKIP_TESTS=t9301.4"


 t/t9301-fast-export.sh |    2 +-
 t/test-lib.sh          |   50 ++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 45 insertions(+), 7 deletions(-)

diff --git i/t/t9301-fast-export.sh w/t/t9301-fast-export.sh
index 2ce2aff..361e8dc 100755
--- i/t/t9301-fast-export.sh
+++ w/t/t9301-fast-export.sh

@@ -63,7 +63,7 @@ test_expect_success 'fast-export master~2..master' '
 
 '
 
-test_expect_success 'iso-8859-1' '
+test_expect_success I18N 'iso-8859-1' '
 
 	git config i18n.commitencoding ISO-8859-1 &&
 	# use author and committer name in ISO-8859-1 to match it.

diff --git i/t/test-lib.sh w/t/test-lib.sh
index e2b106c..88d6d50 100644
--- i/t/test-lib.sh
+++ w/t/test-lib.sh

@@ -232,22 +232,44 @@ test_run_ () {
 	return 0
 }
 
+# space sparated list of skippable test classes
+GIT_SKIPPABLE_TEST_CLASSES='I18N'
+
 test_skip () {
 	this_test=$(expr "./$0" : '.*/\(t[0-9]*\)-[^/]*$')
 	this_test="$this_test.$(expr "$test_count" + 1)"
+
 	to_skip=
-	for skp in $GIT_SKIP_TESTS
-	do
-		case "$this_test" in
-		$skp)
+	if test -n "$test_class"
+	then
+		case " $GIT_SKIPPABLE_TEST_CLASSES " in
+		*" $test_class "*) ;; # ok
+		*)
+			say_color error "'$test_class' is not a skippable test class"
+			error "Skippable are $GIT_SKIPPABLE_TEST_CLASSES"
+		esac
+		case " $GIT_SKIP_TEST_CLASS " in
+		*" $test_class "*)
 			to_skip=t
+			test_class="($test_class) "
 		esac
-	done
+	fi
+	if test -z "$to_skip"
+	then
+		for skp in $GIT_SKIP_TESTS
+		do
+			case "$this_test" in
+			$skp)
+				to_skip=t
+				break
+			esac
+		done
+	fi
 	case "$to_skip" in
 	t)
 		say_color skip >&3 "skipping test: $@"
 		test_count=$(expr "$test_count" + 1)
-		say_color skip "skip $test_count: $1"
+		say_color skip "skip $test_count: $test_class$1"
 		: true
 		;;
 	*)

@@ -257,6 +279,10 @@ test_skip () {
 }
 
 test_expect_failure () {
+	case $# in
+	2)	test_class= ;;
+	3)	test_class=$1; shift ;;
+	esac
 	test "$#" = 2 ||
 	error "bug in the test script: not 2 parameters to test-expect-failure"
 	if ! test_skip "$@"

@@ -274,6 +300,10 @@ test_expect_failure () {
 }
 
 test_expect_success () {
+	case $# in
+	2)	test_class= ;;
+	3)	test_class=$1; shift ;;
+	esac
 	test "$#" = 2 ||
 	error "bug in the test script: not 2 parameters to test-expect-success"
 	if ! test_skip "$@"

@@ -291,6 +321,10 @@ test_expect_success () {
 }
 
 test_expect_code () {
+	case $# in
+	3)	test_class= ;;
+	4)	test_class=$1; shift ;;
+	esac
 	test "$#" = 3 ||
 	error "bug in the test script: not 3 parameters to test-expect-code"
 	if ! test_skip "$@"

@@ -316,6 +350,10 @@ test_expect_code () {
 # Usage: test_external description command arguments...
 # Example: test_external 'Perl API' perl ../path/to/test.pl
 test_external () {
+	case $# in
+	3)	test_class= ;;
+	4)	test_class=$1; shift ;;
+	esac
 	test "$#" -eq 3 ||
 	error >&5 "bug in the test script: not 3 parameters to test_external"
 	descr="$1"

Junio C Hamano schrieb:

Junio C Hamano [off-list ref] writes:

quoted

For this particular case, what we are interested in testing is not that
"config --unset" exits with 0 status.  We are however interested in making
sure that i18n.commitencoding is not set when the body of #12 runs.

So I think a more appropriate change would be something like this for this
particular case.

Having said that, we may want to have an easier way to exclude certain
classes of pieces, and also encourage test writers to group pieces that
are related to these classes together.

For example, this introduces a new environment you can set,
GIT_SKIP_TEST_CLASS, which is a space separated list of classes of
features that you would want to exclude from the test.
test_expect_success/failure can now take an optional "class token" as the
first parameter (they traditionally took only two parameters, but with
class token, they take three).

Nice idea. Another class would be the tests that depend on that the
filesystem supports symbolic links.

-test_expect_success 'iso-8859-1' '
+test_expect_success I18N 'iso-8859-1' '

How do the tests look like if this token is the *last* argument?

To continue the idea, please look into t5302-pack-index.sh: We skip some
tests if we don't have support for 64bit file offsets. Making these tests
a "static" class would not be appropriate because the condition whether
64bit support is present is derived dynamically by the testsuite. What if
we could write tests like this:

test_expect_success \
    'index v2: verify a pack with some 64-bit offsets' \
    'git verify-pack -v "test-3-${pack3}.pack"' \
    'test "$have_64bits"'

i.e. the 3rd argument is a condition that tells whether the test should be
run. And in other cases the 3rd argument is the token that you propose:

test_expect_success 'iso-8859-1' '

     ...test goes here...

' I18N

Hm?

-- Hannes

Johannes Sixt [off-list ref] writes:

Nice idea. Another class would be the tests that depend on that the
filesystem supports symbolic links.

quoted

-test_expect_success 'iso-8859-1' '
+test_expect_success I18N 'iso-8859-1' '

How do the tests look like if this token is the *last* argument?

I thought about it but rejected it because it is much easier to spot class
tokens if it comes immediately after test_expect_xyzzy.

I suspect that certain classes of tests that need to be skipped can be
autodetected inside test-lib.sh; it would be an independent topic to build
on top of this.  Your example of 64-bit may be one of them.