[2/3,RFC] test_todo: allow [!] grep as the command

Commit Message

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

From: Phillip Wood <[e-mail]>

Many failing tests use grep, this commit converts a sample to use
test_todo(). As POSIX specifies that a return code greater than one
indicates an error rather than a failing match we take care not the
hide that.

Signed-off-by: Phillip Wood <[e-mail]>
---
 t/t0000-basic.sh                | 20 ++++++++++--
 t/t3510-cherry-pick-sequence.sh | 12 +++----
 t/t4014-format-patch.sh         | 20 ++++++------
 t/test-lib-functions.sh         | 56 ++++++++++++++++++++++++++++-----
 4 files changed, 82 insertions(+), 26 deletions(-)

Comments

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

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

> From: Phillip Wood <[e-mail]>
>
> Many failing tests use grep, this commit converts a sample to use
> test_todo(). As POSIX specifies that a return code greater than one
> indicates an error rather than a failing match we take care not the
> hide that.

Ah, so on the one hand this gives me second thoughts about my stance
in[1], i.e. if we just allowed any command we wouldn't be forced to add
these sorts of special-cases.

Although, we could also allow any command, and just add smartness for
ones we know about, e.g. "grep".

But I do find doing this to be weirdly inconsistent, i.e. caring about
the difference between these two:

	$ grep blah README.md; echo $?
	1
	$ grep blah README.mdx; echo $?
	grep: README.mdx: No such file or directory
	2

Is basically why I took the approach I did in my [2], i.e. to force us
to positively assert *what* the bad behavior should be.

Which is why I ended up doing my verison of this sort of thing as [3],
i.e. you'd need to assert what specific exit code you expected, or
equivalent.

But at this point in the series:
	
	diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
	index fa7831c0674..086eaf91351 100755
	--- a/t/t3600-rm.sh
	+++ b/t/t3600-rm.sh
	@@ -801,7 +801,7 @@ test_expect_success SYMLINKS 'rm across a symlinked leading path (w/ index)' '
	 	test_todo test_must_fail git rm d/f &&
	 	test_todo git rev-parse --verify :d/f &&
	 	test -h d &&
	-	test_todo test_path_is_file e/f
	+	test_todo test_path_is_file blah
	 '
	 
	 test_expect_success 'setup for testing rm messages' '

So, for our own test_path_* helpers we're not going to care at all, and
any failure will do (including a missing file), but we will care for
grep?

I'm obviously more on the "let's care" side, I just find it odd that
you've picked this halfway point here, but not for other things you're
wrapping.

1. https://lore.kernel.org/git/[e-mail]/
2. https://lore.kernel.org/git/[e-mail]/
3. https://lore.kernel.org/git/[e-mail]/

Phillip Wood Oct. 6, 2022, 4:42 p.m. UTC | #2

On 06/10/2022 16:56, Ævar Arnfjörð Bjarmason wrote:
> 
> On Thu, Oct 06 2022, Phillip Wood via GitGitGadget wrote:
> 
>> From: Phillip Wood <[e-mail]>
>>
>> Many failing tests use grep, this commit converts a sample to use
>> test_todo(). As POSIX specifies that a return code greater than one
>> indicates an error rather than a failing match we take care not the
>> hide that.
> 
> Ah, so on the one hand this gives me second thoughts about my stance
> in[1], i.e. if we just allowed any command we wouldn't be forced to add
> these sorts of special-cases.
> 
> Although, we could also allow any command, and just add smartness for
> ones we know about, e.g. "grep".
> 
> But I do find doing this to be weirdly inconsistent, i.e. caring about
> the difference between these two:
> 
> 	$ grep blah README.md; echo $?
> 	1
> 	$ grep blah README.mdx; echo $?
> 	grep: README.mdx: No such file or directory
> 	2

The intent was to catch bad options, not missing files (i.e. we don't 
want test_todo to hide a failure from "grep --invalid-option"). We could 
check the file exists and skip running grep if it does not (hopefully 
the test wont be grepping multiple files in a single command)

> Is basically why I took the approach I did in my [2], i.e. to force us
> to positively assert *what* the bad behavior should be.

That is what made the end result so hard to use though

	test_todo \
		--want "test_must_fail git" \
		--reset "git reset --hard" \
		--expect git \
		-- \
		rm d/f &&

is not exactly readable.

Best Wishes

Phillip

> Which is why I ended up doing my verison of this sort of thing as [3],
> i.e. you'd need to assert what specific exit code you expected, or
> equivalent.
> 
> But at this point in the series:
> 	
> 	diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
> 	index fa7831c0674..086eaf91351 100755
> 	--- a/t/t3600-rm.sh
> 	+++ b/t/t3600-rm.sh
> 	@@ -801,7 +801,7 @@ test_expect_success SYMLINKS 'rm across a symlinked leading path (w/ index)' '
> 	 	test_todo test_must_fail git rm d/f &&
> 	 	test_todo git rev-parse --verify :d/f &&
> 	 	test -h d &&
> 	-	test_todo test_path_is_file e/f
> 	+	test_todo test_path_is_file blah
> 	 '
> 	
> 	 test_expect_success 'setup for testing rm messages' '
> 
> So, for our own test_path_* helpers we're not going to care at all, and
> any failure will do (including a missing file), but we will care for
> grep?
> 
> I'm obviously more on the "let's care" side, I just find it odd that
> you've picked this halfway point here, but not for other things you're
> wrapping.
> 
> 1. https://lore.kernel.org/git/[e-mail]/
> 2. https://lore.kernel.org/git/[e-mail]/
> 3. https://lore.kernel.org/git/[e-mail]/

Ævar Arnfjörð Bjarmason Oct. 6, 2022, 8:26 p.m. UTC | #3

On Thu, Oct 06 2022, Phillip Wood wrote:

> On 06/10/2022 16:56, Ævar Arnfjörð Bjarmason wrote:
>> On Thu, Oct 06 2022, Phillip Wood via GitGitGadget wrote:
>> 
>>> From: Phillip Wood <[e-mail]>
>>>
>>> Many failing tests use grep, this commit converts a sample to use
>>> test_todo(). As POSIX specifies that a return code greater than one
>>> indicates an error rather than a failing match we take care not the
>>> hide that.
>> Ah, so on the one hand this gives me second thoughts about my stance
>> in[1], i.e. if we just allowed any command we wouldn't be forced to add
>> these sorts of special-cases.
>> Although, we could also allow any command, and just add smartness
>> for
>> ones we know about, e.g. "grep".
>> But I do find doing this to be weirdly inconsistent, i.e. caring
>> about
>> the difference between these two:
>> 	$ grep blah README.md; echo $?
>> 	1
>> 	$ grep blah README.mdx; echo $?
>> 	grep: README.mdx: No such file or directory
>> 	2
>
> The intent was to catch bad options, not missing files (i.e. we don't
> want test_todo to hide a failure from "grep --invalid-option"). We
> could check the file exists and skip running grep if it does not
> (hopefully the test wont be grepping multiple files in a single
> command)

It returns the same exit code for missing files and bad options, so I
don't think this plan will work.

I.e. I (in my initial series) wanted to have something where we declared
what the behavior was right now, *and* what it should be.

But some of our tests are wishy-washing "I wish this worked", so:

	test_todo git some-new-cmd && # should write "unicorn" to a new foo.txt
	test_todo grep unicorn foo.txt

Won't do what you expect?

>> Is basically why I took the approach I did in my [2], i.e. to force us
>> to positively assert *what* the bad behavior should be.
>
> That is what made the end result so hard to use though
>
> 	test_todo \
> 		--want "test_must_fail git" \
> 		--reset "git reset --hard" \
> 		--expect git \
> 		-- \
> 		rm d/f &&
>
> is not exactly readable.

Yes, indeed:) Anyway, my just-sent
https://lore.kernel.org/git/[e-mail]/
goes into that.

I think a "test_todo" should either be a "strictly declare stuff", or a
"YOLO this" where we just detect segfaults.

But per the above having it be some mix of the two is just confusing,
i.e. to extend the example above:

	test_todo git some-new-cmd &&
	test_todo test_path_exists foo.txt &&
	test_todo grep unicorn foo.txt

Won't "work", because the "test_path_exists" isn't strict, but your
"grep" is.

So I think whatever "test_todo" does it should be picking one or the
other.

Patch

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 52362ad3dd3..db5c2059eb5 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -190,6 +190,14 @@  test_expect_success 'subtest: test_todo allowed arguments' '
 		"test_todo test_must_fail git --version"
 	test_expect_success "test_todo test_must_fail rejects bad command" \
 		"test_todo test_must_fail test_true"
+	test_expect_success "test_todo accepts grep" \
+		"echo a >a && test_todo grep b <a"
+	test_expect_success "test_todo accepts ! grep" \
+		"echo a >a && test_todo ! grep -v b <a"
+	test_expect_success "test_todo detects grep errors" \
+		"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_done
 	EOF
 	check_sub_test_lib_test acceptable-test-todo <<-\EOF
@@ -199,9 +207,15 @@  test_expect_success 'subtest: test_todo allowed arguments' '
 	> not ok 3 - test_todo test_must_fail accepts good command # TODO known breakage
 	> not ok 4 - test_todo test_must_fail rejects bad command
 	> #	test_todo test_must_fail test_true
-	> # still have 2 known breakage(s)
-	> # failed 2 among remaining 2 test(s)
-	> 1..4
+	> not ok 5 - test_todo accepts grep # TODO known breakage
+	> not ok 6 - test_todo accepts ! grep # TODO known breakage
+	> not ok 7 - test_todo detects grep errors
+	> #	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
 	EOF
 '
 
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 3b0fa66c33d..cd869255a2c 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -577,7 +577,7 @@  test_expect_success '--continue respects -x in first commit in multi-pick' '
 	grep "cherry picked from.*$picked" msg
 '
 
-test_expect_failure '--signoff is automatically propagated to resolved conflict' '
+test_expect_success '--signoff is automatically propagated to resolved conflict' '
 	pristine_detach initial &&
 	test_expect_code 1 git cherry-pick --signoff base..anotherpick &&
 	echo "c" >foo &&
@@ -591,11 +591,11 @@  test_expect_failure '--signoff is automatically propagated to resolved conflict'
 	git cat-file commit HEAD~3 >initial_msg &&
 	! grep "Signed-off-by:" initial_msg &&
 	grep "Signed-off-by:" unrelatedpick_msg &&
-	! grep "Signed-off-by:" picked_msg &&
+	test_todo ! grep "Signed-off-by:" picked_msg &&
 	grep "Signed-off-by:" anotherpick_msg
 '
 
-test_expect_failure '--signoff dropped for implicit commit of resolution, multi-pick case' '
+test_expect_success '--signoff dropped for implicit commit of resolution, multi-pick case' '
 	pristine_detach initial &&
 	test_must_fail git cherry-pick -s picked anotherpick &&
 	echo c >foo &&
@@ -605,10 +605,10 @@  test_expect_failure '--signoff dropped for implicit commit of resolution, multi-
 	git diff --exit-code HEAD &&
 	test_cmp_rev initial HEAD^^ &&
 	git cat-file commit HEAD^ >msg &&
-	! grep Signed-off-by: msg
+	test_todo ! grep Signed-off-by: msg
 '
 
-test_expect_failure 'sign-off needs to be reaffirmed after conflict resolution, single-pick case' '
+test_expect_success 'sign-off needs to be reaffirmed after conflict resolution, single-pick case' '
 	pristine_detach initial &&
 	test_must_fail git cherry-pick -s picked &&
 	echo c >foo &&
@@ -618,7 +618,7 @@  test_expect_failure 'sign-off needs to be reaffirmed after conflict resolution,
 	git diff --exit-code HEAD &&
 	test_cmp_rev initial HEAD^ &&
 	git cat-file commit HEAD >msg &&
-	! grep Signed-off-by: msg
+	test_todo ! grep Signed-off-by: msg
 '
 
 test_expect_success 'malformed instruction sheet 1' '
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index ad5c0292794..ab649bf27e5 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -165,12 +165,12 @@  test_expect_success 'additional command line cc (ascii)' '
 	grep "^ *S E Cipient <[e-mail]>\$" hdrs5
 '
 
-test_expect_failure 'additional command line cc (rfc822)' '
+test_expect_success 'additional command line cc (rfc822)' '
 	git config --replace-all format.headers "Cc: R E Cipient <[e-mail]>" &&
 	git format-patch --cc="S. E. Cipient <[e-mail]>" --stdout main..side >patch5 &&
 	sed -e "/^\$/q" patch5 >hdrs5 &&
 	grep "^Cc: R E Cipient <[e-mail]>,\$" hdrs5 &&
-	grep "^ *\"S. E. Cipient\" <[e-mail]>\$" hdrs5
+	test_todo grep "^ *\"S. E. Cipient\" <[e-mail]>\$" hdrs5
 '
 
 test_expect_success 'command line headers' '
@@ -195,16 +195,16 @@  test_expect_success 'command line To: header (ascii)' '
 	grep "^To: R E Cipient <[e-mail]>\$" hdrs8
 '
 
-test_expect_failure 'command line To: header (rfc822)' '
+test_expect_success 'command line To: header (rfc822)' '
 	git format-patch --to="R. E. Cipient <[e-mail]>" --stdout main..side >patch8 &&
 	sed -e "/^\$/q" patch8 >hdrs8 &&
-	grep "^To: \"R. E. Cipient\" <[e-mail]>\$" hdrs8
+	test_todo grep "^To: \"R. E. Cipient\" <[e-mail]>\$" hdrs8
 '
 
-test_expect_failure 'command line To: header (rfc2047)' '
+test_expect_success 'command line To: header (rfc2047)' '
 	git format-patch --to="R Ä Cipient <[e-mail]>" --stdout main..side >patch8 &&
 	sed -e "/^\$/q" patch8 >hdrs8 &&
-	grep "^To: =?UTF-8?q?R=20=C3=84=20Cipient?= <[e-mail]>\$" hdrs8
+	test_todo grep "^To: =?UTF-8?q?R=20=C3=84=20Cipient?= <[e-mail]>\$" hdrs8
 '
 
 test_expect_success 'configuration To: header (ascii)' '
@@ -214,18 +214,18 @@  test_expect_success 'configuration To: header (ascii)' '
 	grep "^To: R E Cipient <[e-mail]>\$" hdrs9
 '
 
-test_expect_failure 'configuration To: header (rfc822)' '
+test_expect_success 'configuration To: header (rfc822)' '
 	git config format.to "R. E. Cipient <[e-mail]>" &&
 	git format-patch --stdout main..side >patch9 &&
 	sed -e "/^\$/q" patch9 >hdrs9 &&
-	grep "^To: \"R. E. Cipient\" <[e-mail]>\$" hdrs9
+	test_todo grep "^To: \"R. E. Cipient\" <[e-mail]>\$" hdrs9
 '
 
-test_expect_failure 'configuration To: header (rfc2047)' '
+test_expect_success 'configuration To: header (rfc2047)' '
 	git config format.to "R Ä Cipient <[e-mail]>" &&
 	git format-patch --stdout main..side >patch9 &&
 	sed -e "/^\$/q" patch9 >hdrs9 &&
-	grep "^To: =?UTF-8?q?R=20=C3=84=20Cipient?= <[e-mail]>\$" hdrs9
+	test_todo grep "^To: =?UTF-8?q?R=20=C3=84=20Cipient?= <[e-mail]>\$" hdrs9
 '
 
 # check_patch <patch>: Verify that <patch> looks like a half-sane
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 8978709b231..aee10bd0706 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1010,8 +1010,9 @@  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 any of the assertions beginning test_ such as
-# test_cmp in addition the commands allowed by test_must_fail().
+# test_todo() allows "grep" or any of the assertions beginning test_
+# such as test_cmp in addition the commands allowed by
+# test_must_fail().
 
 test_must_fail_acceptable () {
 	local name
@@ -1050,9 +1051,21 @@  test_must_fail_acceptable () {
 		fi
 		return 1
 		;;
-	test_*)
-		test "$name" = "todo"
-		return $?
+	grep|test_*)
+		if test "$name" = "todo"
+		then
+			test_todo_command_="$1"
+			return 0
+		fi
+		return 1
+		;;
+	'!')
+		if test "$name" = "todo" && test "$2" = "grep"
+		then
+			test_todo_command_=grep
+			return 0
+		fi
+		return 1
 		;;
 	*)
 		return 1
@@ -1061,6 +1074,7 @@  test_must_fail_acceptable () {
 }
 
 test_must_fail_helper () {
+	test_todo_command_=
 	test_must_fail_name_="$1"
 	shift
 	case "$1" in
@@ -1077,8 +1091,27 @@  test_must_fail_helper () {
 		echo >&7 "test_$test_must_fail_name_: only 'git' is allowed: $*"
 		return 1
 	fi
+	test_invert_exit_code_=
+	if test "$1" = "!"
+	then
+		test_invert_exit_code_=1
+		shift
+	fi
 	"$@" 2>&7
 	exit_code=$?
+	if test -n "$test_invert_exit_code_"
+	# We only invert the exit code of grep. An exit code greater
+	# than 1 indicates an error rather than a failed match so
+	# we only want to swap zero and one.
+	then
+		if test $exit_code -eq 0
+		then
+			exit_code=1
+		elif test $exit_code -eq 1
+		then
+			exit_code=0
+		fi
+	fi
 	if test $exit_code -eq 0 && ! list_contains "$_test_ok" success
 	then
 		echo >&4 "test_$test_must_fail_name_: command succeeded: $*"
@@ -1098,6 +1131,15 @@  test_must_fail_helper () {
 	then
 		echo >&4 "test_$test_must_fail_name_: valgrind error: $*"
 		return 1
+	else
+		case "$test_todo_command_" in
+		grep)
+			if test $exit_code -ne 1
+			then
+			       echo >&4 "test_todo: $test_todo_command_ error: $*"
+			       return 1
+			fi;;
+		esac
 	fi
 
 	return 0
@@ -1116,8 +1158,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" or any of the "test_*" assertions
-# such as test_cmp().
+# test_todo can be used with "git", "grep" or any of the "test_*"
+# assertions such as test_cmp().
 
 test_todo () {
 	if test "$test_todo_" = "test_expect_failure"