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

Elijah Newren· Oct 16, 2020, 19:29 UTC · lore

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?

Jeff King· Oct 16, 2020, 20:00 UTC · re: Elijah Newren · lore

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

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

Show 19 quoted lines

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

Show 6 quoted lines

> +# 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