t5510-fetch: fix negated 'test_i18ngrep' invocation
diff mbox series

Message ID 20190730212915.3509-1-szeder.dev@gmail.com
State New
Headers show
Series
  • t5510-fetch: fix negated 'test_i18ngrep' invocation
Related show

Commit Message

SZEDER Gábor July 30, 2019, 9:29 p.m. UTC
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(-)

Comments

SZEDER Gábor July 30, 2019, 9:40 p.m. UTC | #1
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...
Derrick Stolee July 31, 2019, 10:35 a.m. UTC | #2
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

Patch
diff mbox series

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
 	)
 '