Re: Tests failed with GIT_TEST_FAIL_PREREQS and/or GIT_TEST_PROTOCOL_VERSION

[Jeff King <peff@peff.net>]

On Wed, Mar 17, 2021 at 02:38:18PM +0100, Son Luong Ngoc wrote:

> 1. For t7810 and t5300 failing when GIT_TEST_FAIL_PREREQS=1:
> 
>     a926c4b904bdc339568c2898af955cdc61b31542 is the first bad commit
>     commit a926c4b904bdc339568c2898af955cdc61b31542
>     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>     Date:   Thu Feb 11 02:53:51 2021 +0100
> 
>         tests: remove most uses of C_LOCALE_OUTPUT
> 
>         As a follow-up to d162b25f956 (tests: remove support for
>         GIT_TEST_GETTEXT_POISON, 2021-01-20) remove those uses of the now
>         always true C_LOCALE_OUTPUT prerequisite from those tests which
>         declare it as an argument to test_expect_{success,failure}.
> 
>         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>         Signed-off-by: Junio C Hamano <gitster@pobox.com>

I looked at the one in t5300, and I don't think it _ever_ worked, nor
can it be made to work in this mode.

It is expecting that running:

  git index-pack --threads=2

will issue a warning in a build without pthread support. But in the fake
"pretend the pthread prereq is not satisfied" mode, it will of course
not do that, because the build itself is not aware that it's supposed to
be pretending that pthreads aren't supported!

Before the patch mentioned above, its prereqs were:

  !PTHREADS,C_LOCALE_OUTPUT

which would never be satisfied under the "pretend" mode, because
it would _also_ disable C_LOCALE_OUTPUT, and we'd always skip it.

So I think something like this creates a similar situation;

diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index d586fdc7a9..87d26bb70c 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -427,7 +427,8 @@ test_expect_success 'index-pack --strict <pack> works in non-repo' '
 	test_path_is_file foo.idx
 '
 
-test_expect_success !PTHREADS 'index-pack --threads=N or pack.threads=N warns when no pthreads' '
+test_expect_success !PTHREADS,IGNORE_FAIL_PREREQS \
+	'index-pack --threads=N or pack.threads=N warns when no pthreads' '
 	test_must_fail git index-pack --threads=2 2>err &&
 	grep ^warning: err >warnings &&
 	test_line_count = 1 warnings &&
@@ -445,7 +446,8 @@ test_expect_success !PTHREADS 'index-pack --threads=N or pack.threads=N warns wh
 	grep -F "no threads support, ignoring pack.threads" err
 '
 
-test_expect_success !PTHREADS 'pack-objects --threads=N or pack.threads=N warns when no pthreads' '
+test_expect_success !PTHREADS,IGNORE_FAIL_PREREQS \
+	'pack-objects --threads=N or pack.threads=N warns when no pthreads' '
 	git pack-objects --threads=2 --stdout --all </dev/null >/dev/null 2>err &&
 	grep ^warning: err >warnings &&
 	test_line_count = 1 warnings &&

but I think this points to a failing of the FAIL_PREREQS mode. It is
generally OK to say "skip this test by pretending you do not have a
prereq satisfied" (and that is the point: to see if skipping a test
confuses later tests). But given a negated prereq here, it is not OK to
say "run this test that we usually wouldn't", because it is almost
certainly going to be mismatched with the actual build.

So I think the FAIL_PREREQS mode should probably be treating negated
prereqs differently (and always pretending that yes, we have them).

I hadn't investigated the t7810 case yet, but looking at it now, it
seems to be the exact same thing.

-Peff