Tests failed with GIT_TEST_FAIL_PREREQS and/or GIT_TEST_PROTOCOL_VERSION

Tests failed with GIT_TEST_FAIL_PREREQS and/or GIT_TEST_PROTOCOL_VERSION

[Son Luong Ngoc]

Hi folks,

Running the test suit with GIT_TEST_FAIL_PREREQS=1 on master (and
next) seem to result in some failures:

  Test Summary Report
 -------------------
  t5300-pack-object.sh (Wstat: 256 Tests: 46 Failed: 2)
  Failed tests: 35-36
  Non-zero exit status: 1
  t7810-grep.sh (Wstat: 256 Tests: 229 Failed: 1)
  Failed test: 160
  Non-zero exit status: 1
  Files=924, Tests=22400, 422 wallclock secs (12.57 usr 2.52 sys +
601.02 cusr 1047.02 csys = 1663.13 CPU)
  Result: FAIL

A quick git-bisect run seems to point back to this commit:

3b1ca60f8f317b483c8c1805ab500ff2b014cbec is the first bad commit
commit 3b1ca60f8f317b483c8c1805ab500ff2b014cbec
Author: Taylor Blau <me@ttaylorr.com>
Date:   Tue Dec 8 17:03:14 2020 -0500

    ewah/ewah_bitmap.c: avoid open-coding ALLOC_GROW()

    'ewah/ewah_bitmap.c:buffer_grow()' is responsible for growing the buffer
    used to store the bits of an EWAH bitmap. It is essentially doing the
    same task as the 'ALLOC_GROW()' macro, so use that instead.

    This simplifies the callers of 'buffer_grow()', who no longer have to
    ask for a specific size, but rather specify how much of the buffer they
    need. They also no longer need to guard 'buffer_grow()' behind an if
    statement, since 'ALLOC_GROW()' (and, by extension, 'buffer_grow()') is
    a noop if the buffer is already large enough.

    But, the most significant change is that this fixes a bug when calling
    buffer_grow() with both 'alloc_size' and 'new_size' set to 1. In this
    case, truncating integer math will leave the new size set to 1, causing
    the buffer to never grow.

    Instead, let alloc_nr() handle this, which asks for '(new_size + 16) * 3
    / 2' instead of 'new_size * 3 / 2'.

    Signed-off-by: Taylor Blau <me@ttaylorr.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

 ewah/ewah_bitmap.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

I also found that a test is failing with protocol V1 set
(GIT_TEST_PROTOCOL_VERSION=1)

  Test Summary Report
  -------------------
  t5606-clone-options.sh (Wstat: 256 Tests: 15 Failed: 1)
  Failed test: 14
  Non-zero exit status: 1
  Files=924, Tests=22852, 568 wallclock secs (12.69 usr 2.73 sys +
842.87 cusr 1322.91 csys = 2181.20 CPU)
  Result: FAIL

Which git-bisect is telling me that was caused by the same commit.

Regards,
Son Luong.

Re: Tests failed with GIT_TEST_FAIL_PREREQS and/or GIT_TEST_PROTOCOL_VERSION

[Taylor Blau]

Hi,

On Tue, Mar 16, 2021 at 10:45:20AM +0100, Son Luong Ngoc wrote:
> Hi folks,
>
> Running the test suit with GIT_TEST_FAIL_PREREQS=1 on master (and
> next) seem to result in some failures:

Ack, thanks for reporting.

> A quick git-bisect run seems to point back to this commit:
>
> 3b1ca60f8f317b483c8c1805ab500ff2b014cbec is the first bad commit
> commit 3b1ca60f8f317b483c8c1805ab500ff2b014cbec
> Author: Taylor Blau <me@ttaylorr.com>
> Date:   Tue Dec 8 17:03:14 2020 -0500

Hmm. I get a different result.

    $ cat run.sh
    #!/bin/sh
    make -j20 DEVELOPER=1 O=0 && cd t && GIT_TEST_FAIL_PREREQS=1 ./t5300-*.sh -i
    $ git bisect start
    $ git bisect bad
    $ git bisect good v2.30.0
    Bisecting: 453 revisions left to test after this (roughly 9 steps)
    $ git bisect run $PWD/run.sh
    [snip]
    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

>     ewah/ewah_bitmap.c: avoid open-coding ALLOC_GROW()

I'll take a look at what's going on here, but Ævar (cc'd) will probably
beat me to it.

Is it possible that your bisection script doesn't report success
properly? Bisecting the same range (v2.30.0..v2.31.0) with

    $ cat run.sh
    #!/bin/sh
    false

does say that my 3b1ca60f8f (ewah/ewah_bitmap.c: avoid open-coding
ALLOC_GROW(), 2020-12-08) is the first bad commit.

Thanks,
Taylor

Re: Tests failed with GIT_TEST_FAIL_PREREQS and/or GIT_TEST_PROTOCOL_VERSION

[Son Luong Ngoc]

Hi Taylor,

On Tue, Mar 16, 2021 at 09:52:47AM -0400, Taylor Blau wrote:
> Hi,
> 
> Is it possible that your bisection script doesn't report success
> properly? Bisecting the same range (v2.30.0..v2.31.0) with
> 
>     $ cat run.sh
>     #!/bin/sh
>     false
> 
> does say that my 3b1ca60f8f (ewah/ewah_bitmap.c: avoid open-coding
> ALLOC_GROW(), 2020-12-08) is the first bad commit.

You are spot on.  It was a busy day and I only had a few minutes to
look at our internal pipeline of the test suite.  I guess I was doing
something along the line of.

      $ git bisect start HEAD v2.30.0
      $ git bisect run 'cd t && GIT_TEST_PROTOCOL_VERSION=1 ./t5606-clone-options.sh'

Which does indeed errored out and pointed to your commit.

> 
> Thanks,
> Taylor

I have properly re-run the bisection in a './test.sh' bash script and
here are the suspicious commits:

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>

2. For failing t5606 while 'GIT_TEST_PROTOCOL_VERSION=1' was used:

    4f37d45706514a4b3d0259d26f719678a0cf3521 is the first bad commit
    commit 4f37d45706514a4b3d0259d26f719678a0cf3521
    Author: Jonathan Tan <jonathantanmy@google.com>
    Date:   Fri Feb 5 12:48:49 2021 -0800

        clone: respect remote unborn HEAD

        Teach Git to use the "unborn" feature introduced in a previous patch as
        follows: Git will always send the "unborn" argument if it is supported
        by the server. During "git clone", if cloning an empty repository, Git
        will use the new information to determine the local branch to create. In
        all other cases, Git will ignore it.

        Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
        Signed-off-by: Junio C Hamano <gitster@pobox.com>

     Documentation/config/init.txt |  2 +-
     builtin/clone.c               | 16 ++++++++++++++--
     connect.c                     | 28 ++++++++++++++++++++++++++--
     t/t5606-clone-options.sh      |  8 +++++---
     t/t5702-protocol-v2.sh        | 25 +++++++++++++++++++++++++
     transport.h                   |  8 ++++++++
     6 files changed, 79 insertions(+), 8 deletions(-)


Thanks,
Son Luong.

[PATCH] t5606: run clone branch name test with protocol v2

[Jonathan Tan]

4f37d45706 ("clone: respect remote unborn HEAD", 2021-02-05) introduces
a new feature (if the remote has an unborn HEAD, e.g. when the remote
repository is empty, use it as the name of the branch) that only works
in protocol v2, but did not ensure that one of its tests always uses
protocol v2, and thus that test would fail if
GIT_TEST_PROTOCOL_VERSION=0 (or 1) is used. Therefore, add "-c
protocol.version=2" to the appropriate test.

(The rest of the tests from that commit have "-c protocol.version=2"
already added.)

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
Thanks, Son Luong, for noticing this. Here's a fix for the
GIT_TEST_PROTOCOL_VERSION part. This was built on 4f37d45706 but also
applies cleanly on master.

 t/t5606-clone-options.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
index ca6339a5fb..5e30772735 100755
--- a/t/t5606-clone-options.sh
+++ b/t/t5606-clone-options.sh
@@ -106,7 +106,7 @@ test_expect_success 'chooses correct default initial branch name' '
 	git -c init.defaultBranch=foo init --bare empty &&
 	test_config -C empty lsrefs.unborn advertise &&
 	GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \
-	git -c init.defaultBranch=up clone empty whats-up &&
+	git -c init.defaultBranch=up -c protocol.version=2 clone empty whats-up &&
 	test refs/heads/foo = $(git -C whats-up symbolic-ref HEAD) &&
 	test refs/heads/foo = $(git -C whats-up config branch.foo.merge)
 '
-- 
2.31.0.rc2.261.g7f71774620-goog

Re: [PATCH] t5606: run clone branch name test with protocol v2

[Jeff King]

On Wed, Mar 17, 2021 at 08:42:00AM -0700, Jonathan Tan wrote:

> 4f37d45706 ("clone: respect remote unborn HEAD", 2021-02-05) introduces
> a new feature (if the remote has an unborn HEAD, e.g. when the remote
> repository is empty, use it as the name of the branch) that only works
> in protocol v2, but did not ensure that one of its tests always uses
> protocol v2, and thus that test would fail if
> GIT_TEST_PROTOCOL_VERSION=0 (or 1) is used. Therefore, add "-c
> protocol.version=2" to the appropriate test.
> 
> (The rest of the tests from that commit have "-c protocol.version=2"
> already added.)

Thanks, this looks like the obvious and correct fix (and clearly makes
the test pass ;) ).

-Peff

Re: Tests failed with GIT_TEST_FAIL_PREREQS and/or GIT_TEST_PROTOCOL_VERSION

[Jeff King]

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

Re: [PATCH] t5606: run clone branch name test with protocol v2

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> 4f37d45706 ("clone: respect remote unborn HEAD", 2021-02-05) introduces
> a new feature (if the remote has an unborn HEAD, e.g. when the remote
> repository is empty, use it as the name of the branch) that only works
> in protocol v2, but did not ensure that one of its tests always uses
> protocol v2, and thus that test would fail if
> GIT_TEST_PROTOCOL_VERSION=0 (or 1) is used. Therefore, add "-c
> protocol.version=2" to the appropriate test.
>
> (The rest of the tests from that commit have "-c protocol.version=2"
> already added.)
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
> Thanks, Son Luong, for noticing this. Here's a fix for the
> GIT_TEST_PROTOCOL_VERSION part. This was built on 4f37d45706 but also
> applies cleanly on master.

Makes sense.  And I do not see need for any other changes, like
test_expect_failure with protocol 0 (or 1).

Will queue as a candidate for maint-2.31.

Thanks.



>
>  t/t5606-clone-options.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
> index ca6339a5fb..5e30772735 100755
> --- a/t/t5606-clone-options.sh
> +++ b/t/t5606-clone-options.sh
> @@ -106,7 +106,7 @@ test_expect_success 'chooses correct default initial branch name' '
>  	git -c init.defaultBranch=foo init --bare empty &&
>  	test_config -C empty lsrefs.unborn advertise &&
>  	GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \
> -	git -c init.defaultBranch=up clone empty whats-up &&
> +	git -c init.defaultBranch=up -c protocol.version=2 clone empty whats-up &&
>  	test refs/heads/foo = $(git -C whats-up symbolic-ref HEAD) &&
>  	test refs/heads/foo = $(git -C whats-up config branch.foo.merge)
>  '

[PATCH] t: annotate !PTHREADS tests with !FAIL_PREREQS

[Jeff King]

On Wed, Mar 17, 2021 at 01:54:25PM -0400, Jeff King wrote:

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

It looks like the problem is indeed somewhat widespread, and there is a
magic prereq already to skip such tests.

I do still think that this is a fundamental failing of the FAIL_PREREQS
mode, but it probably makes sense to annotate these tests in the
meantime (I don't plan on looking further into it myself).

Another rough edge I noticed: if you set GIT_TEST_HTTPD or
GIT_TEST_GIT_DAEMON to "yes" in your config.mak, these play quite badly
with GIT_TEST_FAIL_PREREQS. We think NOT_ROOT is not satisfied, so
refuse to start httpd, and then complain that the setup fails (and the
point of "yes" for those values is to loudly complain when setup fails,
rather than quietly skipping the tests).

-- >8 --
Subject: [PATCH] t: annotate !PTHREADS tests with !FAIL_PREREQS

Some tests in t5300 and t7810 expect us to complain about a "--threads"
argument when Git is compiled without pthread support. Running these
under GIT_TEST_FAIL_PREREQS produces a confusing failure: we pretend to
the tests that there is no pthread support, so they expect the warning,
but of course the actual build is perfectly happy to respect the
--threads argument.

We never noticed before the recent a926c4b904 (tests: remove most uses
of C_LOCALE_OUTPUT, 2021-02-11), because the tests also were marked as
requiring the C_LOCALE_OUTPUT prerequisite. Which means they'd never
have run in FAIL_PREREQS mode, since it would always pretend that the
locale prereq was not satisfied.

These tests can't possibly work in this mode; it is a mismatch between
what the tests expect and what the build was told to do. So let's just
mark them to be skipped, using the special prereq introduced by
dfe1a17df9 (tests: add a special setup where prerequisites fail,
2019-05-13).

Reported-by: Son Luong Ngoc <sluongng@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5300-pack-object.sh | 6 ++++--
 t/t7810-grep.sh        | 3 ++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index d586fdc7a9..e830a37a38 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,!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,!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 &&
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index edfaa9a6d1..5830733f3d 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -969,7 +969,8 @@ do
 	"
 done
 
-test_expect_success !PTHREADS 'grep --threads=N or pack.threads=N warns when no pthreads' '
+test_expect_success !PTHREADS,!FAIL_PREREQS \
+	'grep --threads=N or pack.threads=N warns when no pthreads' '
 	git grep --threads=2 Hello hello_world 2>err &&
 	grep ^warning: err >warnings &&
 	test_line_count = 1 warnings &&
-- 
2.31.0.559.g509d4a088b

Re: [PATCH] t: annotate !PTHREADS tests with !FAIL_PREREQS

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> 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.
>
> It looks like the problem is indeed somewhat widespread, and there is a
> magic prereq already to skip such tests.
>
> I do still think that this is a fundamental failing of the FAIL_PREREQS
> mode, but it probably makes sense to annotate these tests in the
> meantime (I don't plan on looking further into it myself).

The README file in t/ directory claims that this "is useful for
discovering issues with the tests where say a later test implicitly
depends on an optional earlier test." but apparently it does not
work well with these negated prerequisites.  Its implementation
probably should force a safe bypass of the whole test_have_prereq()
etc. done in test_skip by hooking into test_verify_prereq and
overwrite any non-empty test_prereq with a single hardcoded
PRETEND_FAIL_PREREQ prerequisite that is never satisfied, or
something.

> Another rough edge I noticed: if you set GIT_TEST_HTTPD or
> GIT_TEST_GIT_DAEMON to "yes" in your config.mak, these play quite badly
> with GIT_TEST_FAIL_PREREQS. We think NOT_ROOT is not satisfied, so
> refuse to start httpd, and then complain that the setup fails (and the
> point of "yes" for those values is to loudly complain when setup fails,
> rather than quietly skipping the tests).

... and I think this would also be gone, as the NOT_ROOT test is
done with test_have_prereq that we wouldn't be mucking with if we
limit the FAIL_PREREQS only to tweak the test_expect_* prereqs.

In short, the biggest mistake in the current FAIL_PREREQS design is
to hook into test_have_prereq while the stated objective only needs
to futz with the prerequisite given to the test_expect_* functions,
I would think.

> -- >8 --
> Subject: [PATCH] t: annotate !PTHREADS tests with !FAIL_PREREQS
>
> Some tests in t5300 and t7810 expect us to complain about a "--threads"
> argument when Git is compiled without pthread support. Running these
> under GIT_TEST_FAIL_PREREQS produces a confusing failure: we pretend to
> the tests that there is no pthread support, so they expect the warning,
> but of course the actual build is perfectly happy to respect the
> --threads argument.
>
> We never noticed before the recent a926c4b904 (tests: remove most uses
> of C_LOCALE_OUTPUT, 2021-02-11), because the tests also were marked as
> requiring the C_LOCALE_OUTPUT prerequisite. Which means they'd never
> have run in FAIL_PREREQS mode, since it would always pretend that the
> locale prereq was not satisfied.
>
> These tests can't possibly work in this mode; it is a mismatch between
> what the tests expect and what the build was told to do. So let's just
> mark them to be skipped, using the special prereq introduced by
> dfe1a17df9 (tests: add a special setup where prerequisites fail,
> 2019-05-13).
>
> Reported-by: Son Luong Ngoc <sluongng@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/t5300-pack-object.sh | 6 ++++--
>  t/t7810-grep.sh        | 3 ++-
>  2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
> index d586fdc7a9..e830a37a38 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,!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,!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 &&
> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> index edfaa9a6d1..5830733f3d 100755
> --- a/t/t7810-grep.sh
> +++ b/t/t7810-grep.sh
> @@ -969,7 +969,8 @@ do
>  	"
>  done
>  
> -test_expect_success !PTHREADS 'grep --threads=N or pack.threads=N warns when no pthreads' '
> +test_expect_success !PTHREADS,!FAIL_PREREQS \
> +	'grep --threads=N or pack.threads=N warns when no pthreads' '
>  	git grep --threads=2 Hello hello_world 2>err &&
>  	grep ^warning: err >warnings &&
>  	test_line_count = 1 warnings &&

Re: [PATCH] t: annotate !PTHREADS tests with !FAIL_PREREQS

[Jeff King]

On Thu, Mar 18, 2021 at 02:17:19PM -0700, Junio C Hamano wrote:

> In short, the biggest mistake in the current FAIL_PREREQS design is
> to hook into test_have_prereq while the stated objective only needs
> to futz with the prerequisite given to the test_expect_* functions,
> I would think.

Yeah, that matches my intuition of the problem, too.

-Peff