Re: [PATCH 0/4] meson: parse TAP output generated by our tests

From: Patrick Steinhardt <ps@pks.im>
Date: May 28, 2025, 12:27 UTC
Message-ID: <aDcBGIW2dxWxZ2l4@pks.im>
In-Reply-To: <xmqq34cqqbtk.fsf@gitster.g>

On Tue, May 27, 2025 at 09:04:55AM -0700, Junio C Hamano wrote:

Show 17 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
> 
> >> > I don't think it's inherently a bad thing to fail on unexpected passes.
> >> > After all, it shows that our assumption that the test fails is broken,
> >> > and that we should have a look why that is. But I can see arguments both
> >> > ways.
> >> 
> >> As Phillip noted, treating them as ordinary passes undermines the reason
> >> for having them.
> >
> > Yup, and I tend to agree.
> 
> OK.  So perhaps Make-driven CI jobs also follow suit?  In the same
> run that osx-meson job failed, osx-gcc job notices a passed TODO and
> happily declares "All tests successful.".
> 
> https://github.com/git/git/actions/runs/15221271947/job/42817168362#step:4:1933

prove itself doesn't treat unexpected passes as errors, and I couldn't find an option to adapt it. We can do something like the below patch, which makes us fail in case there are any fixed tests. The only downside is that prove will report test failures like this:

    t1400-update-ref.sh ................................ Dubious, test returned 1 (wstat 256, 0x100)
    All 299 subtests passed
        (1 TODO test unexpectedly succeeded)

It's not great because prove reports it as dubious. But it isn't all that bad either because we directly mention the unexpected success for one of the tests.

Patrick

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 16b785f3b91..2b63e1c86ca 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -130,7 +130,7 @@ test_expect_success 'subtest: a failing TODO test' '
 '
 
 test_expect_success 'subtest: a passing TODO test' '
-	write_and_run_sub_test_lib_test passing-todo <<-\EOF &&
+	write_and_run_sub_test_lib_test_err passing-todo <<-\EOF &&
 	test_expect_failure "pretend we have fixed a known breakage" "true"
 	test_done
 	EOF
@@ -142,7 +142,7 @@ test_expect_success 'subtest: a passing TODO test' '
 '
 
 test_expect_success 'subtest: 2 TODO tests, one passin' '
-	write_and_run_sub_test_lib_test partially-passing-todos <<-\EOF &&
+	write_and_run_sub_test_lib_test_err partially-passing-todos <<-\EOF &&
 	test_expect_failure "pretend we have a known breakage" "false"
 	test_expect_success "pretend we have a passing test" "true"
 	test_expect_failure "pretend we have fixed another known breakage" "true"
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 0a124ffad38..5352209d3e4 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1272,7 +1272,14 @@ test_done () {
 
 		check_test_results_san_file_ "$test_failure"
 
-		if test -z "$skip_all" && test -n "$invert_exit_code"
+		if test "$test_fixed" != 0
+		then
+			if test -z "$invert_exit_code"
+			then
+				GIT_EXIT_OK=t
+				exit 1
+			fi
+		elif test -z "$skip_all" && test -n "$invert_exit_code"
 		then
 			say_color warn "# faking up non-zero exit with --invert-exit-code"
 			GIT_EXIT_OK=t