| Message ID | 20190620204227.8947-1-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tests: mark two failing tests under FAIL_PREREQS | expand |
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 > >
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 >> >>
Æ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 >>> >>>
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 &&
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(-)