Re: [PATCH] tests: fix tests broken under GETTEXT_POISON=YesPlease

[Junio C Hamano <gitster@pobox.com>]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> The GETTEXT_POISON=YesPlease compile-time testing option added in my
> bb946bba76 ("i18n: add GETTEXT_POISON to simulate unfriendly
> translator", 2011-02-22) has been slowly bitrotting as strings have
> been marked for translation, and new tests have been added without
> running it.
>
> I brought this up on the list ("[BUG] test suite broken with
> GETTEXT_POISON=YesPlease", [1]) asking whether this mode was useful at
> all anymore. At least one person occasionally uses it, and Lars
> Schneider offered to change one of the the Travis builds to run in
> this mode, so fix up the failing ones.
>
> My test setup runs most of the tests, with the notable exception of
> skipping all the p4 tests, so it's possible that there's still some
> lurking regressions I haven't fixed.
>
> 1. <CACBZZX62+acvi1dpkknadTL827mtCm_QesGSZ=6+UnyeMpg8+Q@mail.gmail.com>

To be honest, I feel quite uneasy about this patch.  It is no
brainer to take fixes like the one to 1309 where we used grep on a
localizable string to use test_i18ngrep instead---they are obviously
good changes.

But changes that skip tests with !GETTEXT_POISON look suspicious.

> diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
> index 8937e25e49..2003ec7907 100755
> --- a/t/t1430-bad-ref-name.sh
> +++ b/t/t1430-bad-ref-name.sh
> @@ -122,7 +122,7 @@ test_expect_success 'push cannot create a badly named ref' '
>  	! grep -e "broken\.\.\.ref" output
>  '
>  
> -test_expect_failure 'push --mirror can delete badly named ref' '
> +test_expect_failure !GETTEXT_POISON 'push --mirror can delete badly named ref' '
>  	top=$(pwd) &&
>  	git init src &&
>  	git init dest &&

This test affects only src and dest repositories that does not seem
to be looked at by later tests, so skipping it is presumably safe.

I am guessing that the reason why this is skipped is because the
error message is looked at with "! grep ", not "grep", but by
skipping the entire test, we are not checking that "push" succeeds
(which should happen regardless of the locale).

It also is VERY curious that the test before this one (whose tail is
visible in the pre-context) does not need to be skipped.  Is that
because we can expect "broken...ref", which is litrally a part of a
refname, would be emitted intact in any locale (including the
poisoned one)?  If that is the case it is curous why this one needs
to be skipped.

> diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
> index 5778c0afe1..a428ae6703 100755
> --- a/t/t3203-branch-output.sh
> +++ b/t/t3203-branch-output.sh
> @@ -236,7 +236,7 @@ test_expect_success 'git branch --format option' '
>  	Refname is refs/heads/ref-to-remote
>  	EOF
>  	git branch --format="Refname is %(refname)" >actual &&
> -	test_cmp expect actual
> +	test_i18ncmp expect actual
>  '

This is a strange change.  Filling the placeholder in a format
string "Refname is %(refname)" should be affeced by i18n???

> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 33d392ba11..e07d6d8cd2 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -366,7 +366,7 @@ test_expect_success 'verbose flag is heeded, even after --
> ...
> -test_expect_success 'multi-squash only fires up editor once' '
> +test_expect_success !GETTEXT_POISON 'multi-squash only fires up editor once' '
>  	base=$(git rev-parse HEAD~4) &&
>  	set_fake_editor &&
>  	FAKE_COMMIT_AMEND="ONCE" FAKE_LINES="1 squash 2 squash 3 squash 4" \

This is also curious.  Is this because the poison locale does not substitute
anything passed to format template and literal strings like ONCE and
the instructions do not reach the edited file?

 (skipping many changes, not because I find nothing to comment on them)

> diff --git a/t/t5316-pack-delta-depth.sh b/t/t5316-pack-delta-depth.sh
> index 37143ea0ac..2ed479b712 100755
> --- a/t/t5316-pack-delta-depth.sh
> +++ b/t/t5316-pack-delta-depth.sh
> @@ -82,12 +82,16 @@ test_expect_success 'packing produces a long delta' '
>  	# Use --window=0 to make sure we are seeing reused deltas,
>  	# not computing a new long chain.
>  	pack=$(git pack-objects --all --window=0 </dev/null pack) &&
> -	test 9 = "$(max_chain pack-$pack.pack)"
> +	echo 9 >expect &&
> +	max_chain pack-$pack.pack >actual &&
> +	test_i18ncmp expect actual
>  '

This is also curious.  Why do we needto protect comparision with a
line whose contents is '9' from poison locale?  If the last one were
test_cmp I think this is a good change, by the way.

>  test_expect_success '--depth limits depth' '
>  	pack=$(git pack-objects --all --depth=5 </dev/null pack) &&
> -	test 5 = "$(max_chain pack-$pack.pack)"
> +	echo 5 >expect &&
> +	max_chain pack-$pack.pack >actual &&
> +	test_i18ncmp expect actual
>  '

Likewise.