tests: mark two failing tests under FAIL_PREREQS
diff mbox series

Message ID 20190620204227.8947-1-avarab@gmail.com
State New
Headers show
Series
  • tests: mark two failing tests under FAIL_PREREQS
Related show

Commit Message

Ævar Arnfjörð Bjarmason June 20, 2019, 8:42 p.m. UTC
Fix a couple of tests that would potentially fail under
GIT_TEST_FAIL_PREREQS=true.

I missed these when annotating other tests in dfe1a17df9 ("tests: add
a special setup where prerequisites fail", 2019-05-13) because on my
system I can only reproduce this failure when I run the tests as
"root", since the tests happen to depend on whether we can fall back
on GECOS info or not. I.e. they'd usually fail to look up the ident
info anyway, but not always.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t0007-git-var.sh          | 2 +-
 t/t7502-commit-porcelain.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Johannes Schindelin June 21, 2019, 6:04 p.m. UTC | #1
Hi Ævar,

On Thu, 20 Jun 2019, Ævar Arnfjörð Bjarmason wrote:

> Fix a couple of tests that would potentially fail under
> GIT_TEST_FAIL_PREREQS=true.
>
> I missed these when annotating other tests in dfe1a17df9 ("tests: add
> a special setup where prerequisites fail", 2019-05-13) because on my
> system I can only reproduce this failure when I run the tests as
> "root", since the tests happen to depend on whether we can fall back
> on GECOS info or not. I.e. they'd usually fail to look up the ident
> info anyway, but not always.

I had to read the commit message (in particular the oneline) a couple of
times, and I have to admit that I wish it was a bit clearer...

From the explanation, I would have assumed that those two test cases fail
often, anyway, so they shouldn't care whether `FAIL_PREREQS` is in effect.

The only reason why they should be exempt from the `FAIL_PREREQS` mode
that I can think of is that later test cases would depend on them, but how
can they? Those test cases would also have to have the `AUTOIDENT` prereq,
and they would be skipped under `FAIL_PREREQS`, too, no?

In other words, I struggle to understand why this patch is necessary.

Could you help me understand?

Ciao,
Dscho

>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  t/t0007-git-var.sh          | 2 +-
>  t/t7502-commit-porcelain.sh | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
> index 5868a87352..1f600e2cae 100755
> --- a/t/t0007-git-var.sh
> +++ b/t/t0007-git-var.sh
> @@ -17,7 +17,7 @@ test_expect_success 'get GIT_COMMITTER_IDENT' '
>  	test_cmp expect actual
>  '
>
> -test_expect_success !AUTOIDENT 'requested identites are strict' '
> +test_expect_success !FAIL_PREREQS,!AUTOIDENT 'requested identites are strict' '
>  	(
>  		sane_unset GIT_COMMITTER_NAME &&
>  		sane_unset GIT_COMMITTER_EMAIL &&
> diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
> index 5733d9cd34..14c92e4c25 100755
> --- a/t/t7502-commit-porcelain.sh
> +++ b/t/t7502-commit-porcelain.sh
> @@ -402,7 +402,7 @@ echo editor started >"$(pwd)/.git/result"
>  exit 0
>  EOF
>
> -test_expect_success !AUTOIDENT 'do not fire editor when committer is bogus' '
> +test_expect_success !FAIL_PREREQS,!AUTOIDENT 'do not fire editor when committer is bogus' '
>  	>.git/result &&
>
>  	echo >>negative &&
> --
> 2.22.0.455.g172b71a6c5
>
>
Ævar Arnfjörð Bjarmason June 21, 2019, 6:26 p.m. UTC | #2
On Fri, Jun 21 2019, Johannes Schindelin wrote:

> Hi Ævar,
>
> On Thu, 20 Jun 2019, Ævar Arnfjörð Bjarmason wrote:
>
>> Fix a couple of tests that would potentially fail under
>> GIT_TEST_FAIL_PREREQS=true.
>>
>> I missed these when annotating other tests in dfe1a17df9 ("tests: add
>> a special setup where prerequisites fail", 2019-05-13) because on my
>> system I can only reproduce this failure when I run the tests as
>> "root", since the tests happen to depend on whether we can fall back
>> on GECOS info or not. I.e. they'd usually fail to look up the ident
>> info anyway, but not always.
>
> I had to read the commit message (in particular the oneline) a couple of
> times, and I have to admit that I wish it was a bit clearer...
>
> From the explanation, I would have assumed that those two test cases fail
> often, anyway, so they shouldn't care whether `FAIL_PREREQS` is in effect.
>
> The only reason why they should be exempt from the `FAIL_PREREQS` mode
> that I can think of is that later test cases would depend on them, but how
> can they? Those test cases would also have to have the `AUTOIDENT` prereq,
> and they would be skipped under `FAIL_PREREQS`, too, no?

The test doesn't depend on "AUTOIDENT", but "!AUTOIDENT", i.e. the
negated version. The effect of the FAIL_PREREQS mode is to set all
prereqs to false, and therefore "test_have_prereq AUTOIDENT" is false,
but "test_have_prereq !AUTOIDENT" is true.

So this test that would otherwise get skipped gets run.

I honestly didn't think much about these cases when I wrote dfe1a17df9
("tests: add a special setup where prerequisites fail", 2019-05-13), and
now I'm not quite sure whether it should be considered a bug or a
feature, but in the meantime this un-breaks the test suite under this
mode.

> In other words, I struggle to understand why this patch is necessary.
>
> Could you help me understand?
>
> Ciao,
> Dscho
>
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  t/t0007-git-var.sh          | 2 +-
>>  t/t7502-commit-porcelain.sh | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
>> index 5868a87352..1f600e2cae 100755
>> --- a/t/t0007-git-var.sh
>> +++ b/t/t0007-git-var.sh
>> @@ -17,7 +17,7 @@ test_expect_success 'get GIT_COMMITTER_IDENT' '
>>  	test_cmp expect actual
>>  '
>>
>> -test_expect_success !AUTOIDENT 'requested identites are strict' '
>> +test_expect_success !FAIL_PREREQS,!AUTOIDENT 'requested identites are strict' '
>>  	(
>>  		sane_unset GIT_COMMITTER_NAME &&
>>  		sane_unset GIT_COMMITTER_EMAIL &&
>> diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
>> index 5733d9cd34..14c92e4c25 100755
>> --- a/t/t7502-commit-porcelain.sh
>> +++ b/t/t7502-commit-porcelain.sh
>> @@ -402,7 +402,7 @@ echo editor started >"$(pwd)/.git/result"
>>  exit 0
>>  EOF
>>
>> -test_expect_success !AUTOIDENT 'do not fire editor when committer is bogus' '
>> +test_expect_success !FAIL_PREREQS,!AUTOIDENT 'do not fire editor when committer is bogus' '
>>  	>.git/result &&
>>
>>  	echo >>negative &&
>> --
>> 2.22.0.455.g172b71a6c5
>>
>>
Junio C Hamano June 21, 2019, 8:08 p.m. UTC | #3
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> ... The effect of the FAIL_PREREQS mode is to set all
> prereqs to false, and therefore "test_have_prereq AUTOIDENT" is false,
> but "test_have_prereq !AUTOIDENT" is true.
>
> So this test that would otherwise get skipped gets run.
>
> I honestly didn't think much about these cases when I wrote dfe1a17df9
> ("tests: add a special setup where prerequisites fail", 2019-05-13), and
> now I'm not quite sure whether it should be considered a bug or a
> feature, but in the meantime this un-breaks the test suite under this
> mode.

Yeah, reading the above alone, anybody's knee-jerk reaction would be
that fail-prereqs is buggy, but then I am not sure how we can "fix"
that, short of forbidding a prereq like this !AUTOIDENT one from the
test suite.  The test "predicts" how 'git' used in the test body
would behave, and skips it if we know that the predicted behaviour
breaks the test.  Forcing the prereq using FAIL_PREREQS without
actually making a matching change to the behaviour of 'git' has no
chance of producing sane results.

>>> diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
>>> index 5868a87352..1f600e2cae 100755
>>> --- a/t/t0007-git-var.sh
>>> +++ b/t/t0007-git-var.sh
>>> @@ -17,7 +17,7 @@ test_expect_success 'get GIT_COMMITTER_IDENT' '
>>>  	test_cmp expect actual
>>>  '
>>>
>>> -test_expect_success !AUTOIDENT 'requested identites are strict' '
>>> +test_expect_success !FAIL_PREREQS,!AUTOIDENT 'requested identites are strict' '
>>>  	(
>>>  		sane_unset GIT_COMMITTER_NAME &&
>>>  		sane_unset GIT_COMMITTER_EMAIL &&

For this particular one, given the test used to set/unset AUTOIDENT
lazily, I am not sure if it even makes sense to keep this test.  If
we break the underlying ident machinery that would be caught by this
particular test, it seems to me that it is very likely that the test
used to lazily set up AUTOIDENT prereq would fail the same way, so
I am not sure protecting it with !AUTOIDENT would still keep this
one useful.

>>> diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
>>> index 5733d9cd34..14c92e4c25 100755
>>> --- a/t/t7502-commit-porcelain.sh
>>> +++ b/t/t7502-commit-porcelain.sh
>>> @@ -402,7 +402,7 @@ echo editor started >"$(pwd)/.git/result"
>>>  exit 0
>>>  EOF
>>>
>>> -test_expect_success !AUTOIDENT 'do not fire editor when committer is bogus' '
>>> +test_expect_success !FAIL_PREREQS,!AUTOIDENT 'do not fire editor when committer is bogus' '
>>>  	>.git/result &&
>>>
>>>  	echo >>negative &&
>>> --
>>> 2.22.0.455.g172b71a6c5
>>>
>>>

Patch
diff mbox series

diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
index 5868a87352..1f600e2cae 100755
--- a/t/t0007-git-var.sh
+++ b/t/t0007-git-var.sh
@@ -17,7 +17,7 @@  test_expect_success 'get GIT_COMMITTER_IDENT' '
 	test_cmp expect actual
 '
 
-test_expect_success !AUTOIDENT 'requested identites are strict' '
+test_expect_success !FAIL_PREREQS,!AUTOIDENT 'requested identites are strict' '
 	(
 		sane_unset GIT_COMMITTER_NAME &&
 		sane_unset GIT_COMMITTER_EMAIL &&
diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
index 5733d9cd34..14c92e4c25 100755
--- a/t/t7502-commit-porcelain.sh
+++ b/t/t7502-commit-porcelain.sh
@@ -402,7 +402,7 @@  echo editor started >"$(pwd)/.git/result"
 exit 0
 EOF
 
-test_expect_success !AUTOIDENT 'do not fire editor when committer is bogus' '
+test_expect_success !FAIL_PREREQS,!AUTOIDENT 'do not fire editor when committer is bogus' '
 	>.git/result &&
 
 	echo >>negative &&