Funny test flake failure: t7518-ident-corner-cases.sh, test 1

Funny test flake failure: t7518-ident-corner-cases.sh, test 1

[Elijah Newren]

Hi,

On my update to my test-selection series, where I simply added more
text to t/README, I saw a failure in the GitHub Actions testing
despite the same series having previously passed without that extra
text.  The extra text was unrelated; the failure is due to unsafe
environment assumptions (though ones I probably would have made
too...).  See the actual output here:

https://github.com/git/git/pull/878/checks?check_run_id=1266009463

The issue comes from this test code:

       (
               sane_unset GIT_AUTHOR_EMAIL &&
               GIT_AUTHOR_NAME= &&
               test_must_fail git commit --allow-empty -m foo 2>err &&
               test_i18ngrep ! null err
       )

The last line expects 'null' to NOT be found in err, unfortunately,
the file err contained the line:

fatal: empty ident name (for
<runner@fv-az128-670.gcliasfzo2nullsdbrimjtbyhg.cx.internal.cloudapp.net>)
not allowed
218

Note that 'null' appears as a substring of the domain name, found
within 'gcliasfzo2nullsdbrimjtbyhg'.


This testcase was introduced in commit 862e80a413 ("ident: handle NULL
email when complaining of empty name", 2017-02-23), and the reason for
the check appears in a comment above the test:

+# confirm that we do not segfault _and_ that we do not say "(null)", as
+# glibc systems will quietly handle our NULL pointer

Should we tighten the test to check for "(null)" instead of "null", or
should we do something else?  Or just ignore it as it is somewhat
unlikely that anyone ever hits this flake again?

Re: Funny test flake failure: t7518-ident-corner-cases.sh, test 1

[Jeff King]

On Fri, Oct 16, 2020 at 12:29:26PM -0700, Elijah Newren wrote:

> The issue comes from this test code:
> 
>        (
>                sane_unset GIT_AUTHOR_EMAIL &&
>                GIT_AUTHOR_NAME= &&
>                test_must_fail git commit --allow-empty -m foo 2>err &&
>                test_i18ngrep ! null err
>        )
> 
> The last line expects 'null' to NOT be found in err, unfortunately,
> the file err contained the line:
> 
> fatal: empty ident name (for
> <runner@fv-az128-670.gcliasfzo2nullsdbrimjtbyhg.cx.internal.cloudapp.net>)
> not allowed
> 218
> 
> Note that 'null' appears as a substring of the domain name, found
> within 'gcliasfzo2nullsdbrimjtbyhg'.

Heh. That's an amusing find. Good detective work, and sorry I created
the problem.

> +# confirm that we do not segfault _and_ that we do not say "(null)", as
> +# glibc systems will quietly handle our NULL pointer
> 
> Should we tighten the test to check for "(null)" instead of "null", or
> should we do something else?  Or just ignore it as it is somewhat
> unlikely that anyone ever hits this flake again?

I think it's worth tightening the test. The "(null)" phrasing is pretty
common, and it's really the best we can do. The chance that somebody is
on a platform that neither segfaults not prints "(null)", _and_ that
they introduce a regression there seems pretty low. And in comparison,
you already wasted time tracking down a false negative. Let's make sure
that doesn't happen again.

I wish there was an environment variable or something we could set in
the test suite to convince glibc to actually segfault on NULL (because
it _will_ segfault on other platforms, and we're rather catch such
things sooner rather than later).

Another option, I guess, is that we probably _do_ run the tests on such
a platform (IIRC BSD will segfault, so macos probably does). So we could
just remove the grep entirely from this test and let any regression get
caught there.

Of the two, I think I have a slight preference for matching (null), but
I don't feel strongly.

-Peff