{"thread":{"id":"54442","subject":"Funny test flake failure: t7518-ident-corner-cases.sh, test 1","startedAt":"2020-10-16T19:29:40Z","lastAt":"2020-10-16T20:00:34Z","messageCount":2,"participants":["Elijah Newren","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"407745","messageId":"CABPp-BHQK-AtAJa6V1mtu6_jq3oeUeL_0p00RCRNA6Jw3DpJSg@mail.gmail.com","threadId":"54442","inReplyTo":null,"subject":"Funny test flake failure: t7518-ident-corner-cases.sh, test 1","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-10-16T19:29:26Z","receivedAt":"2020-10-16T19:29:40Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nOn my update to my test-selection series, where I simply added more\ntext to t/README, I saw a failure in the GitHub Actions testing\ndespite the same series having previously passed without that extra\ntext.  The extra text was unrelated; the failure is due to unsafe\nenvironment assumptions (though ones I probably would have made\ntoo...).  See the actual output here:\n\nhttps://github.com/git/git/pull/878/checks?check_run_id=1266009463\n\nThe issue comes from this test code:\n\n       (\n               sane_unset GIT_AUTHOR_EMAIL &&\n               GIT_AUTHOR_NAME= &&\n               test_must_fail git commit --allow-empty -m foo 2>err &&\n               test_i18ngrep ! null err\n       )\n\nThe last line expects 'null' to NOT be found in err, unfortunately,\nthe file err contained the line:\n\nfatal: empty ident name (for\n<runner@fv-az128-670.gcliasfzo2nullsdbrimjtbyhg.cx.internal.cloudapp.net>)\nnot allowed\n218\n\nNote that 'null' appears as a substring of the domain name, found\nwithin 'gcliasfzo2nullsdbrimjtbyhg'.\n\n\nThis testcase was introduced in commit 862e80a413 (\"ident: handle NULL\nemail when complaining of empty name\", 2017-02-23), and the reason for\nthe check appears in a comment above the test:\n\n+# confirm that we do not segfault _and_ that we do not say \"(null)\", as\n+# glibc systems will quietly handle our NULL pointer\n\nShould we tighten the test to check for \"(null)\" instead of \"null\", or\nshould we do something else?  Or just ignore it as it is somewhat\nunlikely that anyone ever hits this flake again?\n"},{"id":"407747","messageId":"20201016200031.GA3355643@coredump.intra.peff.net","threadId":"54442","inReplyTo":"CABPp-BHQK-AtAJa6V1mtu6_jq3oeUeL_0p00RCRNA6Jw3DpJSg@mail.gmail.com","subject":"Re: Funny test flake failure: t7518-ident-corner-cases.sh, test 1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-16T20:00:31Z","receivedAt":"2020-10-16T20:00:34Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 16, 2020 at 12:29:26PM -0700, Elijah Newren wrote:\n\n> The issue comes from this test code:\n> \n>        (\n>                sane_unset GIT_AUTHOR_EMAIL &&\n>                GIT_AUTHOR_NAME= &&\n>                test_must_fail git commit --allow-empty -m foo 2>err &&\n>                test_i18ngrep ! null err\n>        )\n> \n> The last line expects 'null' to NOT be found in err, unfortunately,\n> the file err contained the line:\n> \n> fatal: empty ident name (for\n> <runner@fv-az128-670.gcliasfzo2nullsdbrimjtbyhg.cx.internal.cloudapp.net>)\n> not allowed\n> 218\n> \n> Note that 'null' appears as a substring of the domain name, found\n> within 'gcliasfzo2nullsdbrimjtbyhg'.\n\nHeh. That's an amusing find. Good detective work, and sorry I created\nthe problem.\n\n> +# confirm that we do not segfault _and_ that we do not say \"(null)\", as\n> +# glibc systems will quietly handle our NULL pointer\n> \n> Should we tighten the test to check for \"(null)\" instead of \"null\", or\n> should we do something else?  Or just ignore it as it is somewhat\n> unlikely that anyone ever hits this flake again?\n\nI think it's worth tightening the test. The \"(null)\" phrasing is pretty\ncommon, and it's really the best we can do. The chance that somebody is\non a platform that neither segfaults not prints \"(null)\", _and_ that\nthey introduce a regression there seems pretty low. And in comparison,\nyou already wasted time tracking down a false negative. Let's make sure\nthat doesn't happen again.\n\nI wish there was an environment variable or something we could set in\nthe test suite to convince glibc to actually segfault on NULL (because\nit _will_ segfault on other platforms, and we're rather catch such\nthings sooner rather than later).\n\nAnother option, I guess, is that we probably _do_ run the tests on such\na platform (IIRC BSD will segfault, so macos probably does). So we could\njust remove the grep entirely from this test and let any regression get\ncaught there.\n\nOf the two, I think I have a slight preference for matching (null), but\nI don't feel strongly.\n\n-Peff\n"}]}