tests: add a special setup where prerequisites fail
diff mbox series

Message ID 20190513183242.10600-1-avarab@gmail.com
State New
Headers show
Series
  • tests: add a special setup where prerequisites fail
Related show

Commit Message

Ævar Arnfjörð Bjarmason May 13, 2019, 6:32 p.m. UTC
As discussed in [1] there's a regression in the "pu" branch now
because a new test implicitly assumed that a previous test guarded by
a prerequisite had been run. Add a "GIT_TEST_FAIL_PREREQS" special
test setup where we'll skip (nearly) all tests guarded by
prerequisites, allowing us to easily emulate those platform where we
don't run these tests.

As noted in the documentation I'm adding I'm whitelisting the SYMLINKS
prerequisite for now. A lot of tests started failing if we lied about
not supporting symlinks. It's also unlikely that we'll have a failing
test due to a hard dependency on symlinks without that being the
obvious cause, so for now it's not worth the effort to make it work.

1. https://public-inbox.org/git/nycvar.QRO.7.76.6.1905131531000.44@tvgsbejvaqbjf.bet/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Mon, May 13 2019, Johannes Schindelin wrote:

> [...]
> Namely, when test cases 51 and 52 are skipped because of a missing GPG
> prerequisite [*1*], and those two are obviously required to run for the
> `git merge to fail in your test case, as you can very easily verify by
> downloading the artifact containing the `trash directory.t7600-merge`
> directory and re-running the last steps on Linux (where the `git -c
> rerere.enabled=true merge master` *succeeds*).
>
> In fact, you can very, very easily emulate the whole situation on your box
> by running:
>
> 	sh t7600-merge.sh -i -v -x --run=1-50,53-59
>
> And then you can fix your test case so that it does not need to rely on
> test cases that may, or may not, have run previously.

I think it would be better to more pro-actively spot this sort of
thing in the future, so here's a patch to do that. It passes on
"master", but fails on "pu" due to the issue with the one test being
discussed here.

 t/README                   |  9 +++++++++
 t/t0000-basic.sh           | 10 +++++-----
 t/t4202-log.sh             |  2 +-
 t/t7405-submodule-merge.sh |  2 +-
 t/t7810-grep.sh            |  6 +++---
 t/test-lib-functions.sh    | 20 ++++++++++++++++++++
 t/test-lib.sh              |  4 ++++
 7 files changed, 43 insertions(+), 10 deletions(-)

Comments

Johannes Schindelin May 14, 2019, 8:53 a.m. UTC | #1
Hi Ævar,

On Mon, 13 May 2019, Ævar Arnfjörð Bjarmason wrote:

> As discussed in [1] there's a regression in the "pu" branch now
> because a new test implicitly assumed that a previous test guarded by
> a prerequisite had been run. Add a "GIT_TEST_FAIL_PREREQS" special
> test setup where we'll skip (nearly) all tests guarded by
> prerequisites, allowing us to easily emulate those platform where we
> don't run these tests.
>
> As noted in the documentation I'm adding I'm whitelisting the SYMLINKS
> prerequisite for now. A lot of tests started failing if we lied about
> not supporting symlinks. It's also unlikely that we'll have a failing
> test due to a hard dependency on symlinks without that being the
> obvious cause, so for now it's not worth the effort to make it work.

I don't know... In Git for Windows, the SYMLINKS prereq is not met.

(Side note: Windows 10 already supports symlinks for quite some time, even
for non-admin developers, but the fact that Git's test suite is
implemented in shell script bites us yet one more time: MSYS2 has a
completely different idea what symlinks are. It uses the "system file" bit
that only exists on Windows, and if that is set, reads the beginning of
the file, and if that reads "!<symlink>" (interpreted as ASCII), then the
rest of that system file is interpreted as the symlink target.)

So it makes me worry if you say that you had to exclude the SYMLINK
prereq. Maybe all the dependent tests have different prereqs that just so
happen *also* not to be met on Windows?

> 1. https://public-inbox.org/git/nycvar.QRO.7.76.6.1905131531000.44@tvgsbejvaqbjf.bet/
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>
> On Mon, May 13 2019, Johannes Schindelin wrote:
>
> > [...]
> > Namely, when test cases 51 and 52 are skipped because of a missing GPG
> > prerequisite [*1*], and those two are obviously required to run for the
> > `git merge to fail in your test case, as you can very easily verify by
> > downloading the artifact containing the `trash directory.t7600-merge`
> > directory and re-running the last steps on Linux (where the `git -c
> > rerere.enabled=true merge master` *succeeds*).
> >
> > In fact, you can very, very easily emulate the whole situation on your box
> > by running:
> >
> > 	sh t7600-merge.sh -i -v -x --run=1-50,53-59
> >
> > And then you can fix your test case so that it does not need to rely on
> > test cases that may, or may not, have run previously.
>
> I think it would be better to more pro-actively spot this sort of
> thing in the future, so here's a patch to do that. It passes on
> "master", but fails on "pu" due to the issue with the one test being
> discussed here.

It does drive me nuts that the `--run=<N>` option exists (thank you,
Slavica, for teaching me!) and is so poorly supported by our test suite.

For example, if t7600.59 fails, it would make a ton of sense to run

	sh t7600-merge.sh -i -v -x --run=59

right?

Except that we frequently have at least one "test case" whose only purpose
is to set things up.

But we never declare explicitly "test case 59 requires test case 51 to run
first".

We do not even declare the test cases: we execute them immediately. So we
would not even be able to juggle them about, e.g. run them in reverse
(which would otherwise be the easiest way to get rid of almost all side
effects).

I think in the long run, we'll have to drag Git's test suite into the 21st
century (kicking and screaming, I'm sure), to have a more declarative
style, with those features that one might know from Mocha, Jest, JUnit,
xUnit.NET, etc.

Back to your patch: it only catches prereq problems. But the `--run=59`
thing would still not be addressed.

What would you think about a mode where random test cases are skipped? It
would have to make sure to provide a way to recreate the problem, e.g.
giving a string that defines exactly which test cases were skipped.

I am *sure* that tons of test scripts would fail with that, and we would
probably have to special-case the `setup` "test cases", and we would have
to clean up quite a few scripts to *not* execute random stuff outside of
`test_expect_*`...

> diff --git a/t/README b/t/README
> index 6404f33e19..9747971d58 100644
> --- a/t/README
> +++ b/t/README
> @@ -334,6 +334,15 @@ that cannot be easily covered by a few specific test cases. These
>  could be enabled by running the test suite with correct GIT_TEST_
>  environment set.
>
> +GIT_TEST_FAIL_PREREQS<non-empty?> fails all prerequisites. This is

Did you mean to insert `=` after `GIT_TEST_FAIL_PREREQS`?

> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 819c24d10e..c20209324c 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -352,7 +352,7 @@ test_expect_success 'log with grep.patternType configuration and command line' '
>  	test_cmp expect actual
>  '
>
> -test_expect_success 'log with various grep.patternType configurations & command-lines' '
> +test_expect_success !FAIL_PREREQS 'log with various grep.patternType configurations & command-lines' '

Is this an indication of a bug in this test case?

>  	git init pattern-type &&
>  	(
>  		cd pattern-type &&
> diff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh
> index 7855bd8648..aa33978ed2 100755
> --- a/t/t7405-submodule-merge.sh
> +++ b/t/t7405-submodule-merge.sh
> @@ -417,7 +417,7 @@ test_expect_failure 'directory/submodule conflict; keep submodule clean' '
>  	)
>  '
>
> -test_expect_failure 'directory/submodule conflict; should not treat submodule files as untracked or in the way' '
> +test_expect_failure !FAIL_PREREQS 'directory/submodule conflict; should not treat submodule files as untracked or in the way' '

Same here?

>  	test_when_finished "git -C directory-submodule/path reset --hard" &&
>  	test_when_finished "git -C directory-submodule reset --hard" &&
>  	(
> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> index 2e1bb61b41..7d7b396c23 100755
> --- a/t/t7810-grep.sh
> +++ b/t/t7810-grep.sh
> @@ -412,7 +412,7 @@ do
>  		test_cmp expected actual
>  	'
>
> -	test_expect_success !PCRE "grep $L with grep.patterntype=perl errors without PCRE" '
> +	test_expect_success !FAIL_PREREQS,!PCRE "grep $L with grep.patterntype=perl errors without PCRE" '

And here?

>  		test_must_fail git -c grep.patterntype=perl grep "foo.*bar"
>  	'
>
> @@ -1234,7 +1234,7 @@ test_expect_success PCRE 'grep --perl-regexp pattern' '
>  	test_cmp expected actual
>  '
>
> -test_expect_success !PCRE 'grep --perl-regexp pattern errors without PCRE' '
> +test_expect_success !FAIL_PREREQS,!PCRE 'grep --perl-regexp pattern errors without PCRE' '

And here?

>  	test_must_fail git grep --perl-regexp "foo.*bar"
>  '
>
> @@ -1249,7 +1249,7 @@ test_expect_success LIBPCRE2 "grep -P with (*NO_JIT) doesn't error out" '
>
>  '
>
> -test_expect_success !PCRE 'grep -P pattern errors without PCRE' '
> +test_expect_success !FAIL_PREREQS,!PCRE 'grep -P pattern errors without PCRE' '

And here?

>  	test_must_fail git grep -P "foo.*bar"
>  '
>
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 8270de74be..0367cec5fd 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -309,6 +309,26 @@ test_unset_prereq () {
>  }
>
>  test_set_prereq () {
> +	if test -n "$GIT_TEST_FAIL_PREREQS"
> +	then
> +		case "$1" in
> +		# The "!" case is handled below with
> +		# test_unset_prereq()
> +		!*)
> +			;;
> +		# (Temporary?) whitelist of things we can't easily
> +		# pretend not to support
> +		SYMLINKS)
> +			;;
> +		# Inspecting whether GIT_TEST_FAIL_PREREQS is on
> +		# should be unaffected.
> +		FAIL_PREREQS)
> +			;;
> +		*)
> +			return
> +		esac
> +	fi
> +

I would probably have done that on the reading side rather than the
writing side ;-)

Thanks for starting this!
Dscho

>  	case "$1" in
>  	!*)
>  		test_unset_prereq "${1#!}"
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 908ddb9c46..6fabafebb3 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1607,3 +1607,7 @@ test_lazy_prereq SHA1 '
>  test_lazy_prereq REBASE_P '
>  	test -z "$GIT_TEST_SKIP_REBASE_P"
>  '
> +
> +test_lazy_prereq FAIL_PREREQS '
> +	test -n "$GIT_TEST_FAIL_PREREQS"
> +'
> --
> 2.21.0.1020.gf2820cf01a
>
>
Ævar Arnfjörð Bjarmason May 14, 2019, 9:41 a.m. UTC | #2
On Tue, May 14 2019, Johannes Schindelin wrote:

> Hi Ævar,
>
> On Mon, 13 May 2019, Ævar Arnfjörð Bjarmason wrote:
>
>> As discussed in [1] there's a regression in the "pu" branch now
>> because a new test implicitly assumed that a previous test guarded by
>> a prerequisite had been run. Add a "GIT_TEST_FAIL_PREREQS" special
>> test setup where we'll skip (nearly) all tests guarded by
>> prerequisites, allowing us to easily emulate those platform where we
>> don't run these tests.
>>
>> As noted in the documentation I'm adding I'm whitelisting the SYMLINKS
>> prerequisite for now. A lot of tests started failing if we lied about
>> not supporting symlinks. It's also unlikely that we'll have a failing
>> test due to a hard dependency on symlinks without that being the
>> obvious cause, so for now it's not worth the effort to make it work.
>
> I don't know... In Git for Windows, the SYMLINKS prereq is not met.
>
> (Side note: Windows 10 already supports symlinks for quite some time, even
> for non-admin developers, but the fact that Git's test suite is
> implemented in shell script bites us yet one more time: MSYS2 has a
> completely different idea what symlinks are. It uses the "system file" bit
> that only exists on Windows, and if that is set, reads the beginning of
> the file, and if that reads "!<symlink>" (interpreted as ASCII), then the
> rest of that system file is interpreted as the symlink target.)
>
> So it makes me worry if you say that you had to exclude the SYMLINK
> prereq. Maybe all the dependent tests have different prereqs that just so
> happen *also* not to be met on Windows?

I should have clarified this. What I mean is that it leaves SYMLINK
alone, i.e. what was breaking is that on Linux we don't deal with
pretending that we don't support symlinks, these tests should still work
just fine on Windows (or anywhere SYMLINKS is genuinely false), since
there we actually back up our promise not to support them.

>> 1. https://public-inbox.org/git/nycvar.QRO.7.76.6.1905131531000.44@tvgsbejvaqbjf.bet/
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>
>> On Mon, May 13 2019, Johannes Schindelin wrote:
>>
>> > [...]
>> > Namely, when test cases 51 and 52 are skipped because of a missing GPG
>> > prerequisite [*1*], and those two are obviously required to run for the
>> > `git merge to fail in your test case, as you can very easily verify by
>> > downloading the artifact containing the `trash directory.t7600-merge`
>> > directory and re-running the last steps on Linux (where the `git -c
>> > rerere.enabled=true merge master` *succeeds*).
>> >
>> > In fact, you can very, very easily emulate the whole situation on your box
>> > by running:
>> >
>> > 	sh t7600-merge.sh -i -v -x --run=1-50,53-59
>> >
>> > And then you can fix your test case so that it does not need to rely on
>> > test cases that may, or may not, have run previously.
>>
>> I think it would be better to more pro-actively spot this sort of
>> thing in the future, so here's a patch to do that. It passes on
>> "master", but fails on "pu" due to the issue with the one test being
>> discussed here.
>
> It does drive me nuts that the `--run=<N>` option exists (thank you,
> Slavica, for teaching me!) and is so poorly supported by our test suite.
>
> For example, if t7600.59 fails, it would make a ton of sense to run
>
> 	sh t7600-merge.sh -i -v -x --run=59
>
> right?
>
> Except that we frequently have at least one "test case" whose only purpose
> is to set things up.
>
> But we never declare explicitly "test case 59 requires test case 51 to run
> first".
>
> We do not even declare the test cases: we execute them immediately. So we
> would not even be able to juggle them about, e.g. run them in reverse
> (which would otherwise be the easiest way to get rid of almost all side
> effects).
>
> I think in the long run, we'll have to drag Git's test suite into the 21st
> century (kicking and screaming, I'm sure), to have a more declarative
> style, with those features that one might know from Mocha, Jest, JUnit,
> xUnit.NET, etc.
>
> Back to your patch: it only catches prereq problems. But the `--run=59`
> thing would still not be addressed.

Yeah that would be a nice-to-have but unrelated thing (and a much bigger
task). We'd need to split up "this is setup code" v.s. "this is a test",
or make each individual test declare its dependency graph, which could
get quite verbose.

> What would you think about a mode where random test cases are skipped? It
> would have to make sure to provide a way to recreate the problem, e.g.
> giving a string that defines exactly which test cases were skipped.
>
> I am *sure* that tons of test scripts would fail with that, and we would
> probably have to special-case the `setup` "test cases", and we would have
> to clean up quite a few scripts to *not* execute random stuff outside of
> `test_expect_*`...

I think it would be neat, but unrelated to and overkill for spotting the
practical problem we have now, which is that we *know* we skip some of
this now on some platforms/setups due to prereqs.

>> diff --git a/t/README b/t/README
>> index 6404f33e19..9747971d58 100644
>> --- a/t/README
>> +++ b/t/README
>> @@ -334,6 +334,15 @@ that cannot be easily covered by a few specific test cases. These
>>  could be enabled by running the test suite with correct GIT_TEST_
>>  environment set.
>>
>> +GIT_TEST_FAIL_PREREQS<non-empty?> fails all prerequisites. This is
>
> Did you mean to insert `=` after `GIT_TEST_FAIL_PREREQS`?

Yeah, will fix.

>> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
>> index 819c24d10e..c20209324c 100755
>> --- a/t/t4202-log.sh
>> +++ b/t/t4202-log.sh
>> @@ -352,7 +352,7 @@ test_expect_success 'log with grep.patternType configuration and command line' '
>>  	test_cmp expect actual
>>  '
>>
>> -test_expect_success 'log with various grep.patternType configurations & command-lines' '
>> +test_expect_success !FAIL_PREREQS 'log with various grep.patternType configurations & command-lines' '
>
> Is this an indication of a bug in this test case?
>
>>  	git init pattern-type &&
>>  	(
>>  		cd pattern-type &&
>> diff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh
>> index 7855bd8648..aa33978ed2 100755
>> --- a/t/t7405-submodule-merge.sh
>> +++ b/t/t7405-submodule-merge.sh
>> @@ -417,7 +417,7 @@ test_expect_failure 'directory/submodule conflict; keep submodule clean' '
>>  	)
>>  '
>>
>> -test_expect_failure 'directory/submodule conflict; should not treat submodule files as untracked or in the way' '
>> +test_expect_failure !FAIL_PREREQS 'directory/submodule conflict; should not treat submodule files as untracked or in the way' '
>
> Same here?
>
>>  	test_when_finished "git -C directory-submodule/path reset --hard" &&
>>  	test_when_finished "git -C directory-submodule reset --hard" &&
>>  	(
>> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
>> index 2e1bb61b41..7d7b396c23 100755
>> --- a/t/t7810-grep.sh
>> +++ b/t/t7810-grep.sh
>> @@ -412,7 +412,7 @@ do
>>  		test_cmp expected actual
>>  	'
>>
>> -	test_expect_success !PCRE "grep $L with grep.patterntype=perl errors without PCRE" '
>> +	test_expect_success !FAIL_PREREQS,!PCRE "grep $L with grep.patterntype=perl errors without PCRE" '
>
> And here?
>
>>  		test_must_fail git -c grep.patterntype=perl grep "foo.*bar"
>>  	'
>>
>> @@ -1234,7 +1234,7 @@ test_expect_success PCRE 'grep --perl-regexp pattern' '
>>  	test_cmp expected actual
>>  '
>>
>> -test_expect_success !PCRE 'grep --perl-regexp pattern errors without PCRE' '
>> +test_expect_success !FAIL_PREREQS,!PCRE 'grep --perl-regexp pattern errors without PCRE' '
>
> And here?
>
>>  	test_must_fail git grep --perl-regexp "foo.*bar"
>>  '
>>
>> @@ -1249,7 +1249,7 @@ test_expect_success LIBPCRE2 "grep -P with (*NO_JIT) doesn't error out" '
>>
>>  '
>>
>> -test_expect_success !PCRE 'grep -P pattern errors without PCRE' '
>> +test_expect_success !FAIL_PREREQS,!PCRE 'grep -P pattern errors without PCRE' '
>
> And here?

To all of the above: Not a bug, I guess it could be refactored as an
unrelated fix, but it's tests of the form "without this prereq we should
die", so of course if I lie about not having it where I *do* have it it
won't die.

>>  	test_must_fail git grep -P "foo.*bar"
>>  '
>>
>> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
>> index 8270de74be..0367cec5fd 100644
>> --- a/t/test-lib-functions.sh
>> +++ b/t/test-lib-functions.sh
>> @@ -309,6 +309,26 @@ test_unset_prereq () {
>>  }
>>
>>  test_set_prereq () {
>> +	if test -n "$GIT_TEST_FAIL_PREREQS"
>> +	then
>> +		case "$1" in
>> +		# The "!" case is handled below with
>> +		# test_unset_prereq()
>> +		!*)
>> +			;;
>> +		# (Temporary?) whitelist of things we can't easily
>> +		# pretend not to support
>> +		SYMLINKS)
>> +			;;
>> +		# Inspecting whether GIT_TEST_FAIL_PREREQS is on
>> +		# should be unaffected.
>> +		FAIL_PREREQS)
>> +			;;
>> +		*)
>> +			return
>> +		esac
>> +	fi
>> +
>
> I would probably have done that on the reading side rather than the
> writing side ;-)

I started out by doing that (I'll note so in the v2 commit message), but
found it much harier, in that function we need to deal with the lazy
prereqs v.s. non-lazy, so I'd need to change multiple branches of that
or refactor it, all that stuff calls test_set_prereq() after it
discovers what the prereq is, so doing it here is much easier &
straightforward.

>>  	case "$1" in
>>  	!*)
>>  		test_unset_prereq "${1#!}"
>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>> index 908ddb9c46..6fabafebb3 100644
>> --- a/t/test-lib.sh
>> +++ b/t/test-lib.sh
>> @@ -1607,3 +1607,7 @@ test_lazy_prereq SHA1 '
>>  test_lazy_prereq REBASE_P '
>>  	test -z "$GIT_TEST_SKIP_REBASE_P"
>>  '
>> +
>> +test_lazy_prereq FAIL_PREREQS '
>> +	test -n "$GIT_TEST_FAIL_PREREQS"
>> +'
>> --
>> 2.21.0.1020.gf2820cf01a
>>
>>
Johannes Schindelin May 14, 2019, 12:37 p.m. UTC | #3
Hi Ævar,

On Tue, 14 May 2019, Ævar Arnfjörð Bjarmason wrote:

> On Tue, May 14 2019, Johannes Schindelin wrote:
>
> > What would you think about a mode where random test cases are skipped?
> > It would have to make sure to provide a way to recreate the problem,
> > e.g. giving a string that defines exactly which test cases were
> > skipped.
> >
> > I am *sure* that tons of test scripts would fail with that, and we
> > would probably have to special-case the `setup` "test cases", and we
> > would have to clean up quite a few scripts to *not* execute random
> > stuff outside of `test_expect_*`...
>
> I think it would be neat, but unrelated to and overkill for spotting the
> practical problem we have now, which is that we *know* we skip some of
> this now on some platforms/setups due to prereqs.

I understand, but I am still worried that this is a lot of work for an
incomplete fix.

For example, the t7600-merge.sh test script that set off this conversation
has two prereqs that are unmet on Windows: GPG and EXECKEEPSPID. On Azure
Pipelines' macOS agents, it is only GPG that is unmet. So switching off
all prereqs would not help macOS with e.g. a bug where the GPG test cases
are skipped but the EXECKEEPSPID test case is not.

Ciao,
Dscho
Ævar Arnfjörð Bjarmason May 14, 2019, 1:39 p.m. UTC | #4
On Tue, May 14 2019, Johannes Schindelin wrote:

> Hi Ævar,
>
> On Tue, 14 May 2019, Ævar Arnfjörð Bjarmason wrote:
>
>> On Tue, May 14 2019, Johannes Schindelin wrote:
>>
>> > What would you think about a mode where random test cases are skipped?
>> > It would have to make sure to provide a way to recreate the problem,
>> > e.g. giving a string that defines exactly which test cases were
>> > skipped.
>> >
>> > I am *sure* that tons of test scripts would fail with that, and we
>> > would probably have to special-case the `setup` "test cases", and we
>> > would have to clean up quite a few scripts to *not* execute random
>> > stuff outside of `test_expect_*`...
>>
>> I think it would be neat, but unrelated to and overkill for spotting the
>> practical problem we have now, which is that we *know* we skip some of
>> this now on some platforms/setups due to prereqs.
>
> I understand, but I am still worried that this is a lot of work for an
> incomplete fix.
>
> For example, the t7600-merge.sh test script that set off this conversation
> has two prereqs that are unmet on Windows: GPG and EXECKEEPSPID. On Azure
> Pipelines' macOS agents, it is only GPG that is unmet. So switching off
> all prereqs would not help macOS with e.g. a bug where the GPG test cases
> are skipped but the EXECKEEPSPID test case is not.

It won't catch such cases, but will catch cases where a later new test
assumes that whatever the state of the test repo it gets is what's
always going to be there. In practice I think that'll catch most such
issues.

The other GIT_TEST_* modes assume similar non-combinatorial explosion
failure scenarios.

I haven't gone back through the test suite's commit history to try to
dig for other cases, so perhaps this mode is premature etc.
Johannes Schindelin May 14, 2019, 2:04 p.m. UTC | #5
Hi Ævar,

On Tue, 14 May 2019, Ævar Arnfjörð Bjarmason wrote:

> On Tue, May 14 2019, Johannes Schindelin wrote:
>
> > On Tue, 14 May 2019, Ævar Arnfjörð Bjarmason wrote:
> >
> >> On Tue, May 14 2019, Johannes Schindelin wrote:
> >>
> >> > What would you think about a mode where random test cases are
> >> > skipped? It would have to make sure to provide a way to recreate
> >> > the problem, e.g. giving a string that defines exactly which test
> >> > cases were skipped.
> >> >
> >> > I am *sure* that tons of test scripts would fail with that, and we
> >> > would probably have to special-case the `setup` "test cases", and
> >> > we would have to clean up quite a few scripts to *not* execute
> >> > random stuff outside of `test_expect_*`...
> >>
> >> I think it would be neat, but unrelated to and overkill for spotting
> >> the practical problem we have now, which is that we *know* we skip
> >> some of this now on some platforms/setups due to prereqs.
> >
> > I understand, but I am still worried that this is a lot of work for an
> > incomplete fix.
> >
> > For example, the t7600-merge.sh test script that set off this
> > conversation has two prereqs that are unmet on Windows: GPG and
> > EXECKEEPSPID. On Azure Pipelines' macOS agents, it is only GPG that is
> > unmet. So switching off all prereqs would not help macOS with e.g. a
> > bug where the GPG test cases are skipped but the EXECKEEPSPID test
> > case is not.
>
> It won't catch such cases, but will catch cases where a later new test
> assumes that whatever the state of the test repo it gets is what's
> always going to be there. In practice I think that'll catch most such
> issues.

That's fair. It will also raise awareness of these issues, which should
also have a quite beneficial effect.

Given that your patch is not even large, I think you're right, it *is* a
lot of bang for the buck.

Ciao,
Dscho

Patch
diff mbox series

diff --git a/t/README b/t/README
index 6404f33e19..9747971d58 100644
--- a/t/README
+++ b/t/README
@@ -334,6 +334,15 @@  that cannot be easily covered by a few specific test cases. These
 could be enabled by running the test suite with correct GIT_TEST_
 environment set.
 
+GIT_TEST_FAIL_PREREQS<non-empty?> fails all prerequisites. This is
+useful for discovering issues with the tests where say a later test
+implicitly depends on an optional earlier test.
+
+There's a "FAIL_PREREQS" prerequisite that can be used to test for
+whether this mode is active, and e.g. skip some tests that are hard to
+refactor to deal with it. The "SYMLINKS" prerequisite is currently
+excluded as so much relies on it, but this might change in the future.
+
 GIT_TEST_GETTEXT_POISON=<non-empty?> turns all strings marked for
 translation into gibberish if non-empty (think "test -n"). Used for
 spotting those tests that need to be marked with a C_LOCALE_OUTPUT
diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index c03054c538..31de7e90f3 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -726,7 +726,7 @@  donthaveit=yes
 test_expect_success DONTHAVEIT 'unmet prerequisite causes test to be skipped' '
 	donthaveit=no
 '
-if test $haveit$donthaveit != yesyes
+if test -z "$GIT_TEST_FAIL_PREREQS" -a $haveit$donthaveit != yesyes
 then
 	say "bug in test framework: prerequisite tags do not work reliably"
 	exit 1
@@ -747,7 +747,7 @@  donthaveiteither=yes
 test_expect_success DONTHAVEIT,HAVEIT 'unmet prerequisites causes test to be skipped' '
 	donthaveiteither=no
 '
-if test $haveit$donthaveit$donthaveiteither != yesyesyes
+if test -z "$GIT_TEST_FAIL_PREREQS" -a $haveit$donthaveit$donthaveiteither != yesyesyes
 then
 	say "bug in test framework: multiple prerequisite tags do not work reliably"
 	exit 1
@@ -763,7 +763,7 @@  test_expect_success !LAZY_TRUE 'missing lazy prereqs skip tests' '
 	donthavetrue=no
 '
 
-if test "$havetrue$donthavetrue" != yesyes
+if test -z "$GIT_TEST_FAIL_PREREQS" -a "$havetrue$donthavetrue" != yesyes
 then
 	say 'bug in test framework: lazy prerequisites do not work'
 	exit 1
@@ -779,7 +779,7 @@  test_expect_success LAZY_FALSE 'missing negative lazy prereqs will skip' '
 	havefalse=no
 '
 
-if test "$nothavefalse$havefalse" != yesyes
+if test -z "$GIT_TEST_FAIL_PREREQS" -a "$nothavefalse$havefalse" != yesyes
 then
 	say 'bug in test framework: negative lazy prerequisites do not work'
 	exit 1
@@ -790,7 +790,7 @@  test_expect_success 'tests clean up after themselves' '
 	test_when_finished clean=yes
 '
 
-if test $clean != yes
+if test -z "$GIT_TEST_FAIL_PREREQS" -a $clean != yes
 then
 	say "bug in test framework: basic cleanup command does not work reliably"
 	exit 1
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 819c24d10e..c20209324c 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -352,7 +352,7 @@  test_expect_success 'log with grep.patternType configuration and command line' '
 	test_cmp expect actual
 '
 
-test_expect_success 'log with various grep.patternType configurations & command-lines' '
+test_expect_success !FAIL_PREREQS 'log with various grep.patternType configurations & command-lines' '
 	git init pattern-type &&
 	(
 		cd pattern-type &&
diff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh
index 7855bd8648..aa33978ed2 100755
--- a/t/t7405-submodule-merge.sh
+++ b/t/t7405-submodule-merge.sh
@@ -417,7 +417,7 @@  test_expect_failure 'directory/submodule conflict; keep submodule clean' '
 	)
 '
 
-test_expect_failure 'directory/submodule conflict; should not treat submodule files as untracked or in the way' '
+test_expect_failure !FAIL_PREREQS 'directory/submodule conflict; should not treat submodule files as untracked or in the way' '
 	test_when_finished "git -C directory-submodule/path reset --hard" &&
 	test_when_finished "git -C directory-submodule reset --hard" &&
 	(
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 2e1bb61b41..7d7b396c23 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -412,7 +412,7 @@  do
 		test_cmp expected actual
 	'
 
-	test_expect_success !PCRE "grep $L with grep.patterntype=perl errors without PCRE" '
+	test_expect_success !FAIL_PREREQS,!PCRE "grep $L with grep.patterntype=perl errors without PCRE" '
 		test_must_fail git -c grep.patterntype=perl grep "foo.*bar"
 	'
 
@@ -1234,7 +1234,7 @@  test_expect_success PCRE 'grep --perl-regexp pattern' '
 	test_cmp expected actual
 '
 
-test_expect_success !PCRE 'grep --perl-regexp pattern errors without PCRE' '
+test_expect_success !FAIL_PREREQS,!PCRE 'grep --perl-regexp pattern errors without PCRE' '
 	test_must_fail git grep --perl-regexp "foo.*bar"
 '
 
@@ -1249,7 +1249,7 @@  test_expect_success LIBPCRE2 "grep -P with (*NO_JIT) doesn't error out" '
 
 '
 
-test_expect_success !PCRE 'grep -P pattern errors without PCRE' '
+test_expect_success !FAIL_PREREQS,!PCRE 'grep -P pattern errors without PCRE' '
 	test_must_fail git grep -P "foo.*bar"
 '
 
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 8270de74be..0367cec5fd 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -309,6 +309,26 @@  test_unset_prereq () {
 }
 
 test_set_prereq () {
+	if test -n "$GIT_TEST_FAIL_PREREQS"
+	then
+		case "$1" in
+		# The "!" case is handled below with
+		# test_unset_prereq()
+		!*)
+			;;
+		# (Temporary?) whitelist of things we can't easily
+		# pretend not to support
+		SYMLINKS)
+			;;
+		# Inspecting whether GIT_TEST_FAIL_PREREQS is on
+		# should be unaffected.
+		FAIL_PREREQS)
+			;;
+		*)
+			return
+		esac
+	fi
+
 	case "$1" in
 	!*)
 		test_unset_prereq "${1#!}"
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 908ddb9c46..6fabafebb3 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1607,3 +1607,7 @@  test_lazy_prereq SHA1 '
 test_lazy_prereq REBASE_P '
 	test -z "$GIT_TEST_SKIP_REBASE_P"
 '
+
+test_lazy_prereq FAIL_PREREQS '
+	test -n "$GIT_TEST_FAIL_PREREQS"
+'