t: annotate !PTHREADS tests with !FAIL_PREREQS

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

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

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