| Message ID | 20190730212915.3509-1-szeder.dev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t5510-fetch: fix negated 'test_i18ngrep' invocation | expand |
On Tue, Jul 30, 2019 at 11:29:15PM +0200, SZEDER Gábor wrote: > The test '--no-show-forced-updates' in 't5510-fetch.sh' added in > cdbd70c437 (fetch: add --[no-]show-forced-updates argument, > 2019-06-18) runs '! test_i18ngrep ...'. This is wrong, because when > running the test with GIT_TEST_GETTEXT_POISON=true, then > 'test_i18ngrep' is basically a noop and always returns with success, > the leading ! turns that into a failure, which then fails the test. > > Use 'test_i18ngrep ! ...' instead. > > This went unnoticed by our GETTEXT_POISON CI builds, because those > builds don't run this test case: in those builds we don't install > Apache, and this test comes after 't5510' sources 'lib-httpd.sh', > which, consequently, skips all the remaining tests, including this > one. Hrm... It looks like there is nothing httpd-specific in this test case, at all, so we could run it even if a webserver is not available. Moving this test case earlier in the script seems to confirm it, as it still succeeds. However, I'm not really familiar with this '--[no-]show-forced-updates' option, and this is not the time to get up to speed, so I would let Derrick to decide and follow up...
On 7/30/2019 5:40 PM, SZEDER Gábor wrote: > On Tue, Jul 30, 2019 at 11:29:15PM +0200, SZEDER Gábor wrote: >> The test '--no-show-forced-updates' in 't5510-fetch.sh' added in >> cdbd70c437 (fetch: add --[no-]show-forced-updates argument, >> 2019-06-18) runs '! test_i18ngrep ...'. This is wrong, because when >> running the test with GIT_TEST_GETTEXT_POISON=true, then >> 'test_i18ngrep' is basically a noop and always returns with success, >> the leading ! turns that into a failure, which then fails the test. >> >> Use 'test_i18ngrep ! ...' instead. >> >> This went unnoticed by our GETTEXT_POISON CI builds, because those >> builds don't run this test case: in those builds we don't install >> Apache, and this test comes after 't5510' sources 'lib-httpd.sh', >> which, consequently, skips all the remaining tests, including this >> one. > > Hrm... It looks like there is nothing httpd-specific in this test > case, at all, so we could run it even if a webserver is not available. > Moving this test case earlier in the script seems to confirm it, as it > still succeeds. > > However, I'm not really familiar with this > '--[no-]show-forced-updates' option, and this is not the time to get > up to speed, so I would let Derrick to decide and follow up... I was about to recommend you move the test to before the check for the changes. I only put the test near the end as I normally don't want to insert into the middle of a test script. It makes sense to do so here. Thanks, -Stolee
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 139f7106f7..f2481de577 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -997,7 +997,7 @@ test_expect_success '--no-show-forced-updates' ' ( cd no-forced-update-clone && git fetch --no-show-forced-updates origin 2>output && - ! test_i18ngrep "(forced update)" output + test_i18ngrep ! "(forced update)" output ) '
The test '--no-show-forced-updates' in 't5510-fetch.sh' added in cdbd70c437 (fetch: add --[no-]show-forced-updates argument, 2019-06-18) runs '! test_i18ngrep ...'. This is wrong, because when running the test with GIT_TEST_GETTEXT_POISON=true, then 'test_i18ngrep' is basically a noop and always returns with success, the leading ! turns that into a failure, which then fails the test. Use 'test_i18ngrep ! ...' instead. This went unnoticed by our GETTEXT_POISON CI builds, because those builds don't run this test case: in those builds we don't install Apache, and this test comes after 't5510' sources 'lib-httpd.sh', which, consequently, skips all the remaining tests, including this one. Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> --- This is a minor issue in v2.23.0-rc0. 'git grep "! test_i18n"' shows no other similar cases. t/t5510-fetch.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)