Re: [PATCH 1/3] [RFC] tests: add test_todo() to mark known breakages

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: phillip.wood@dunelm.org.uk
Cc: Phillip Wood via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, Derrick Stolee <stolee@gmail.com>
Subject: Re: [PATCH 1/3] [RFC] tests: add test_todo() to mark known breakages
Date: Thu, 06 Oct 2022 22:33:04 +0200	[thread overview]
Message-ID: <221006.86mta8r860.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <7306b890-641f-2c45-f610-2aa0361d6066@dunelm.org.uk>

On Thu, Oct 06 2022, Phillip Wood wrote:

> Hi Ævar
>
> On 06/10/2022 16:36, Ævar Arnfjörð Bjarmason wrote:
>> On Thu, Oct 06 2022, Phillip Wood via GitGitGadget wrote:
>> 
>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>>
>>> test_todo() is built upon test_expect_failure() but accepts commands
>>> starting with test_* in addition to git. As our test_* assertions use
>>> BUG() to signal usage errors any such error will not be hidden by
>>> test_todo().
>> I think they will, unless I'm missing something. E.g. try out:
>
> It's talking about BUG() in test-lib.sh, not the C function. For example
>
> test_path_is_file () {
> 	test "$#" -ne 1 && BUG "1 param"
> 	if ! test -f "$1"
> 	then
> 		echo "File $1 doesn't exist"
> 		false
> 	fi
> }
>
> So a test containing "test_todo test_path_is_file a b" should fail as
> BUG calls exit rather than returning non-zero (I should probably test 
> that in 0000-basic.sh)

Ah, anyway, I think getting that to behave correctly is *the* thing any
less sucky test_expect_failure replacement needs to get right, i.e. to
handle BUG() (in C code), abort(), SANITIZE=undefined (and friends, all
of whom abort()), segfaults etc.

>> 	diff --git a/t/t0210-trace2-normal.sh b/t/t0210-trace2-normal.sh
>> 	index 80e76a4695e..1be895abba6 100755
>> 	--- a/t/t0210-trace2-normal.sh
>> 	+++ b/t/t0210-trace2-normal.sh
>> 	@@ -170,7 +170,7 @@ test_expect_success 'BUG messages are written to trace2' '
>> 	
>> 	 test_expect_success 'bug messages with BUG_if_bug() are written to trace2' '
>> 	 	test_when_finished "rm trace.normal actual expect" &&
>> 	-	test_expect_code 99 env GIT_TRACE2="$(pwd)/trace.normal" \
>> 	+	test_todo env GIT_TRACE2="$(pwd)/trace.normal" \
>> 	 		test-tool trace2 008bug 2>err &&
>> 	 	cat >expect <<-\EOF &&
>> 	 	a bug message
>> I.e. in our tests you need to look out for exit code 99, not the
>> usual
>> abort().
>> I have local patches to fix this, previously submitted as an RFC
>> here:
>> https://lore.kernel.org/git/RFC-cover-0.3-00000000000-20220525T234908Z-avarab@gmail.com/
>> I just rebased that & CI is currently running, I'll see how it does:
>> https://github.com/avar/git/tree/avar/usage-do-not-abort-on-BUG-to-get-trace2-event-2
>> When I merged your patches here with that topic yours started doing
>> the
>> right thing in this case, i.e. a "test_todo" that would get a BUG()"
>> would be reported as a failure.
>> 
>>> +	test_true () {
>>> +		true
>>> +	}
>>> +	test_expect_success "pretend we have fixed a test_todo breakage" \
>>> +		"test_todo test_true"
>> "Why the indirection", until I realized that it's because you're
>> working
>> around the whitelist of commands that we have, i.e. we allow 'git' and
>> 'test-tool', but not 'grep' or whatever.
>> I'm of the opinion that we should just drop that limitation
>> altogether,
>> which is shown to be pretty silly in this case. I.e. at some point we
>> listed "test_*" as "this invokes a git binary", but a lot of our test_*
>> commands don't, including this one.
>
> test_expect_failure does not allow test_* are you thinking of test-tool?

I'm talking about test_todo, and the reason for that "test_true" being
needed is because test_must_fail_acceptable is picky, but we could also
(I just adjusted that one test):

	diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
	index 52362ad3dd3..921e0401eb5 100755
	--- a/t/t0000-basic.sh
	+++ b/t/t0000-basic.sh
	@@ -143,11 +143,8 @@ test_expect_success 'subtest: a passing TODO test' '

	 test_expect_success 'subtest: a failing test_todo' '
	 	write_and_run_sub_test_lib_test failing-test-todo <<-\EOF &&
	-	test_false () {
	-		false
	-	}
	 	test_expect_success "passing test" "true"
	-	test_expect_success "known todo" "test_todo test_false"
	+	test_expect_success "known todo" "test_todo false"
	 	test_done
	 	EOF
	 	check_sub_test_lib_test failing-test-todo <<-\EOF
	diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
	index 8978709b231..9300eaa2c62 100644
	--- a/t/test-lib-functions.sh
	+++ b/t/test-lib-functions.sh
	@@ -1034,6 +1034,11 @@ test_must_fail_acceptable () {
	 		done
	 	fi

	+	if test "$name" = "todo"
	+	then
	+		return 0
	+	fi
	+
	 	case "$1" in
	 	git|__git*|test-tool|test_terminal)
	 		return 0
	@@ -1050,10 +1055,6 @@ test_must_fail_acceptable () {
	 		fi
	 		return 1
	 		;;
	-	test_*)
	-		test "$name" = "todo"
	-		return $?
	-		;;
	 	*)
	 		return 1
	 		;;

>> So in general I think we should just allow any command in
>> "test_must_fail" et al, and just catch in code review if someone uses
>> "test_must_fail grep" as opposed to "! grep".
>
> That is not going to scale well

Well, you're throwing the scaling out the window by whitelisting test_*
in your 1/3 :)

I don't think we really need it, but *the* reason it's there is to
distinguish things that run our own C code from things that don't, and a
lot of test_* helpers don't.

So if you want to pursue making this correct per the current intent it
should really use the current whitelist to decide whether or not to pass
things through the "should we change the exit code if it's a signal,
segfault etc?" part.

Otherwise you should just negate it or whatever, as the "test_todo" I
showed in
https://lore.kernel.org/git/221006.86v8owr986.gmgdl@evledraar.gmail.com/
does. I.e. we shouldn't be detecting an abort() in /bin/false and the
like.