[3/3,RFC] test_todo: allow [verbose] test as the command

Commit Message

Phillip Wood Oct. 6, 2022, 3:01 p.m. UTC

From: Phillip Wood <[e-mail]>

For simplicity test_todo() allows verbose to precede any valid
command.  As POSIX specifies that a return code greater than one is an
error rather than a failed test we take care not to hide that.

I'm in two minds about this patch. Generally it is better to use one
of our test helpers such as test_cmp() rather than calling test
directly.  There are so few instances of test being used within
test_expect_failure() (the conversions here are not exhaustive but
there are not many more) that it would probably be better to convert
the tests by using the appropriate helper rather than supporting
calling test as the command to test_todo().

Signed-off-by: Phillip Wood <[e-mail]>
---
 t/t0000-basic.sh        | 20 +++++++++++++++++---
 t/t0050-filesystem.sh   |  4 ++--
 t/t3903-stash.sh        | 12 ++++++------
 t/test-lib-functions.sh | 17 +++++++++++------
 4 files changed, 36 insertions(+), 17 deletions(-)

Comments

Ævar Arnfjörð Bjarmason Oct. 6, 2022, 4:02 p.m. UTC | #1

On Thu, Oct 06 2022, Phillip Wood via GitGitGadget wrote:

> From: Phillip Wood <[e-mail]>
>
> For simplicity test_todo() allows verbose to precede any valid
> command.  As POSIX specifies that a return code greater than one is an
> error rather than a failed test we take care not to hide that.
>
> I'm in two minds about this patch. Generally it is better to use one
> of our test helpers such as test_cmp() rather than calling test
> directly.  There are so few instances of test being used within
> test_expect_failure() (the conversions here are not exhaustive but
> there are not many more) that it would probably be better to convert
> the tests by using the appropriate helper rather than supporting
> calling test as the command to test_todo().

I think that we might want to salvage parts of this, but we really
shouldn't be spending review time on carrying forward a bad pattern that
hides segfaults.

I.e. whatever we do about "test_todo"'s interaction with "test" let's
first change things like...

> -test_expect_failure CASE_INSENSITIVE_FS 'add (with different case)' '
> +test_expect_success CASE_INSENSITIVE_FS 'add (with different case)' '
>  	git reset --hard initial &&
>  	rm camelcase &&
>  	echo 1 >CamelCase &&
> @@ -108,7 +108,7 @@ test_expect_failure CASE_INSENSITIVE_FS 'add (with different case)' '
>  	git ls-files >tmp &&
>  	camel=$(grep -i camelcase tmp) &&
>  	test $(echo "$camel" | wc -l) = 1 &&
> -	test "z$(git cat-file blob :$camel)" = z1
> +	test_todo test "z$(git cat-file blob :$camel)" = z1

...this to e.g.:

	echo z1 >expect &&
	git cat-file blob :$camel >actual &&
	test_cmp expect actual

Or whatever, then let's see if migrating "verbose" is worthwhile, in the
post-image you end up with no real users of it, only your tests.

I've wanted to just remove it for a while, all its users seem to be
either bad uses like that, or we'd get much better bang for the buck out
of it by having a t/verbose-bin/ or whatever, which would just wrap
arbitrary commands like "grep" and the like (i.e. ones where we could
provide useful context).

Patch

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index db5c2059eb5..93d3930d9f6 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -198,6 +198,14 @@  test_expect_success 'subtest: test_todo allowed arguments' '
 		"echo a >a && test_todo grep --invalid-option b <a"
 	test_expect_success "test_todo detects ! grep errors" \
 		"echo a >a && test_todo ! grep --invalid-option -v b <a"
+	test_expect_success "test_todo accepts test" \
+		"test_todo test -z a"
+	test_expect_success "test_todo detects test errors" \
+		"test_todo test a xxx b"
+	test_expect_success "test_todo skips verbose and accepts good command" \
+		"test_todo verbose test -z a"
+	test_expect_success "test_todo skips verbose and rejects bad command" \
+		"test_todo verbose false"
 	test_done
 	EOF
 	check_sub_test_lib_test acceptable-test-todo <<-\EOF
@@ -213,9 +221,15 @@  test_expect_success 'subtest: test_todo allowed arguments' '
 	> #	echo a >a && test_todo grep --invalid-option b <a
 	> not ok 8 - test_todo detects ! grep errors
 	> #	echo a >a && test_todo ! grep --invalid-option -v b <a
-	> # still have 4 known breakage(s)
-	> # failed 4 among remaining 4 test(s)
-	> 1..8
+	> not ok 9 - test_todo accepts test # TODO known breakage
+	> not ok 10 - test_todo detects test errors
+	> #	test_todo test a xxx b
+	> not ok 11 - test_todo skips verbose and accepts good command # TODO known breakage
+	> not ok 12 - test_todo skips verbose and rejects bad command
+	> #	test_todo verbose false
+	> # still have 6 known breakage(s)
+	> # failed 6 among remaining 6 test(s)
+	> 1..12
 	EOF
 '
 
diff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh
index 325eb1c3cd0..16b933a4c8d 100755
--- a/t/t0050-filesystem.sh
+++ b/t/t0050-filesystem.sh
@@ -100,7 +100,7 @@  test_expect_success CASE_INSENSITIVE_FS 'add directory (with different case)' '
 	test_cmp expected actual
 '
 
-test_expect_failure CASE_INSENSITIVE_FS 'add (with different case)' '
+test_expect_success CASE_INSENSITIVE_FS 'add (with different case)' '
 	git reset --hard initial &&
 	rm camelcase &&
 	echo 1 >CamelCase &&
@@ -108,7 +108,7 @@  test_expect_failure CASE_INSENSITIVE_FS 'add (with different case)' '
 	git ls-files >tmp &&
 	camel=$(grep -i camelcase tmp) &&
 	test $(echo "$camel" | wc -l) = 1 &&
-	test "z$(git cat-file blob :$camel)" = z1
+	test_todo test "z$(git cat-file blob :$camel)" = z1
 '
 
 test_expect_success "setup unicode normalization tests" '
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 376cc8f4ab8..afc5b4126f3 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -556,7 +556,7 @@  test_expect_success 'unstash must re-create the file' '
 	test bar = "$(cat file)"
 '
 
-test_expect_failure 'stash directory to file' '
+test_expect_success 'stash directory to file' '
 	git reset --hard &&
 	mkdir dir &&
 	echo foo >dir/file &&
@@ -567,12 +567,12 @@  test_expect_failure 'stash directory to file' '
 	git stash save "directory to file" &&
 	test_path_is_dir dir &&
 	test foo = "$(cat dir/file)" &&
-	test_must_fail git stash apply &&
-	test bar = "$(cat dir)" &&
+	test_todo test_must_fail git stash apply &&
+	test_todo test bar = "$(cat dir)" &&
 	git reset --soft HEAD^
 '
 
-test_expect_failure 'stash file to directory' '
+test_expect_success 'stash file to directory' '
 	git reset --hard &&
 	rm file &&
 	mkdir file &&
@@ -581,8 +581,8 @@  test_expect_failure 'stash file to directory' '
 	test_path_is_file file &&
 	test bar = "$(cat file)" &&
 	git stash apply &&
-	test_path_is_file file/file &&
-	test foo = "$(cat file/file)"
+	test_todo test_path_is_file file/file &&
+	test_todo test foo = "$(cat file/file)"
 '
 
 test_expect_success 'giving too many ref arguments does not modify files' '
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index aee10bd0706..068a0702809 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1010,8 +1010,8 @@  list_contains () {
 # with env, the env and its corresponding variable settings will be
 # stripped before we we test the command being run.
 #
-# test_todo() allows "grep" or any of the assertions beginning test_
-# such as test_cmp in addition the commands allowed by
+# test_todo() allows "grep", "test" or any of the assertions beginning
+# test_ such as test_cmp in addition the commands allowed by
 # test_must_fail().
 
 test_must_fail_acceptable () {
@@ -1019,6 +1019,11 @@  test_must_fail_acceptable () {
 	name="$1"
 	shift
 
+	if test "$name" = "todo" && test "$1" = "verbose"
+	then
+		shift
+	fi
+
 	if test "$1" = "env"
 	then
 		shift
@@ -1051,7 +1056,7 @@  test_must_fail_acceptable () {
 		fi
 		return 1
 		;;
-	grep|test_*)
+	grep|test|test_*)
 		if test "$name" = "todo"
 		then
 			test_todo_command_="$1"
@@ -1133,7 +1138,7 @@  test_must_fail_helper () {
 		return 1
 	else
 		case "$test_todo_command_" in
-		grep)
+		grep|test)
 			if test $exit_code -ne 1
 			then
 			       echo >&4 "test_todo: $test_todo_command_ error: $*"
@@ -1158,8 +1163,8 @@  test_must_fail_helper () {
 #	'
 #
 # This test will fail if "git foo" fails or err is unexpectedly empty.
-# test_todo can be used with "git", "grep" or any of the "test_*"
-# assertions such as test_cmp().
+# test_todo can be used with "git", "grep", "test" or any of the
+# "test_*" assertions such as test_cmp().
 
 test_todo () {
 	if test "$test_todo_" = "test_expect_failure"