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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Oct 6, 2022, 16:10 UTC
Message-ID: <7306b890-641f-2c45-f610-2aa0361d6066@dunelm.org.uk>
In-Reply-To: <221006.868rltrltu.gmgdl@evledraar.gmail.com>

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>

>>

Show 6 quoted lines

>> 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)

Show 41 quoted lines

> 	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?

> 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

Show 5 quoted lines

> But for the particular case of "test_todo" doing so seems like pointless
> work, if we think we're going to miss this sort of thing in review in
> general, then surely that's not a concern if we're going to very
> prominently mark tests as TODO tests, given how the test of the output
> shows them?

 >[...]

Show 14 quoted lines

>>   test_might_fail () {
>> -	test_must_fail ok=success "$@" 2>&7
>> +	test_must_fail_helper might_fail ok=success "$@" 2>&7
>>   } 7>&2 2>&4
>>   
>>   # Similar to test_must_fail and test_might_fail, but check that a
> 
> I remember finding it annoying that "test_might_fail" is misreported
> from test_must_fail_acceptable as being called "test_must_fail", so this
> refactoring is very welcome.
> 
> But can you please split this into its own commit? I.e. that improvement
> can stand on its own, i.e. just a change that has "test_must_fail" and
> "test_might_fail" reporting their correct name.

> Then this commit can follow, and then you'll just need to add (for this part) >
> 	test_must_fail_helper todo "$@" 2>&7
> 
> And it'll make the resulting diff much smaller & easier to follow.

Sure, I should also improve the error message in

 >> +		echo >&7 "test_$test_must_fail_name_: only 'git' is allowed: $*"

for test_todo.

Best Wishes

Phillip