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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Apr 24, 2017 at 3:15 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Æ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.

This turns out to just be a mistake of mine that didn't need
!GETTEXT_POISON at all. Are you OK with fixing that up, or do you want
a v2 due to comments below...?

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

It's because %(refname) itself will get i18n'd:

    -Refname is (HEAD detached from fromtag)
    +Refname is # GETTEXT POISON #

See c8183cd285 ("branch: show more information when HEAD is detached",
2013-03-13).

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

I don't know the root cause for why that fails, I spent some time
re-digging into that for ~10m and didn't come up with anything.

We've marked many of these rebase messages in the past, it's usually
some stray newline being needed which the poison machinery didn't emit
or something like that.

My general approach when writing & maintaining this poison has been
that it's fine if we skip some tests, even though we could be bending
over backwards to run them, or even if we don't know the root cause
beyond "the rebase machinery is always broken with poison".

This is because once I'm satisfied that the breaking test isn't
because of some new plumbing message that got i18n'd I don't see the
point of keeping digging, it's fine to just skip the test, because we
run it when we're not under poison, and we're satisfied that it's not
breaking because of a new plumbing message being i18n'd we've
fulfilled the entire reason for why this poison facility exists in the
first place.

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

The 9/5 value here is the number of lines that match /chain length/ as
determined my max_chain, that message is i18n'd.