* [BUG] test suite broken with GETTEXT_POISON=YesPlease @ 2017-04-20 21:58 Ævar Arnfjörð Bjarmason 2017-04-21 14:47 ` Michael J Gruber 2017-04-21 14:54 ` [BUG] test suite broken with GETTEXT_POISON=YesPlease Lars Schneider 0 siblings, 2 replies; 21+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-04-20 21:58 UTC (permalink / raw) To: Git Mailing List; +Cc: Jiang Xin, Lars Schneider, Junio C Hamano As a refresh of everyone's memory (because mine needed it). This is a feature I added back in 2011 when the i18n support was initially added. There was concern at the time that we would inadvertently mark plumbing messages for translation, particularly something in a shared code path, and this was a way to hopefully smoke out those issues with the test suite. However compiling with it breaks a couple of dozen tests, I stopped digging when I saw some broke back in 2014. What should be done about this? I think if we're going to keep them they need to be run regularly by something like Travis (Lars CC'd), however empirical evidence suggests that not running them is just fine too, so should we just remove support for this test mode? I don't care, but I can come up with the patch either way, but would only be motivated to write the one-time fix for it if some CI system is actually running them regularly, otherwise they'll just be subject to bitrotting again. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [BUG] test suite broken with GETTEXT_POISON=YesPlease 2017-04-20 21:58 [BUG] test suite broken with GETTEXT_POISON=YesPlease Ævar Arnfjörð Bjarmason @ 2017-04-21 14:47 ` Michael J Gruber 2017-04-21 17:28 ` Ævar Arnfjörð Bjarmason 2017-04-24 1:18 ` Junio C Hamano 2017-04-21 14:54 ` [BUG] test suite broken with GETTEXT_POISON=YesPlease Lars Schneider 1 sibling, 2 replies; 21+ messages in thread From: Michael J Gruber @ 2017-04-21 14:47 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason, Git Mailing List Cc: Jiang Xin, Lars Schneider, Junio C Hamano Ævar Arnfjörð Bjarmason venit, vidit, dixit 20.04.2017 23:58: > As a refresh of everyone's memory (because mine needed it). This is a > feature I added back in 2011 when the i18n support was initially > added. > > There was concern at the time that we would inadvertently mark > plumbing messages for translation, particularly something in a shared > code path, and this was a way to hopefully smoke out those issues with > the test suite. > > However compiling with it breaks a couple of dozen tests, I stopped > digging when I saw some broke back in 2014. > > What should be done about this? I think if we're going to keep them > they need to be run regularly by something like Travis (Lars CC'd), > however empirical evidence suggests that not running them is just fine > too, so should we just remove support for this test mode? > > I don't care, but I can come up with the patch either way, but would > only be motivated to write the one-time fix for it if some CI system > is actually running them regularly, otherwise they'll just be subject > to bitrotting again. > I use that switch when I change something that involves l10n, but usually I run specific tests only. To be honest: I have to make sure not to get confused by (nor forget one of) the build flag GETTEXT_POISON and the environment variable GIT_GETTEXT_POISON. I'm not sure I always tested what I meant to test... Michael ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [BUG] test suite broken with GETTEXT_POISON=YesPlease 2017-04-21 14:47 ` Michael J Gruber @ 2017-04-21 17:28 ` Ævar Arnfjörð Bjarmason 2017-04-24 1:18 ` Junio C Hamano 1 sibling, 0 replies; 21+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-04-21 17:28 UTC (permalink / raw) To: Michael J Gruber Cc: Git Mailing List, Jiang Xin, Lars Schneider, Junio C Hamano On Fri, Apr 21, 2017 at 4:47 PM, Michael J Gruber <git@grubix.eu> wrote: > Ævar Arnfjörð Bjarmason venit, vidit, dixit 20.04.2017 23:58: >> As a refresh of everyone's memory (because mine needed it). This is a >> feature I added back in 2011 when the i18n support was initially >> added. >> >> There was concern at the time that we would inadvertently mark >> plumbing messages for translation, particularly something in a shared >> code path, and this was a way to hopefully smoke out those issues with >> the test suite. >> >> However compiling with it breaks a couple of dozen tests, I stopped >> digging when I saw some broke back in 2014. >> >> What should be done about this? I think if we're going to keep them >> they need to be run regularly by something like Travis (Lars CC'd), >> however empirical evidence suggests that not running them is just fine >> too, so should we just remove support for this test mode? >> >> I don't care, but I can come up with the patch either way, but would >> only be motivated to write the one-time fix for it if some CI system >> is actually running them regularly, otherwise they'll just be subject >> to bitrotting again. >> > > I use that switch when I change something that involves l10n, but > usually I run specific tests only. To be honest: I have to make sure not > to get confused by (nor forget one of) the build flag GETTEXT_POISON and > the environment variable GIT_GETTEXT_POISON. I'm not sure I always > tested what I meant to test... For any of the built-in tests, you just need to compile git with GETTEXT_POISON=YesPlease, the env var is set by test-lib.sh for you, you only need to set GIT_GETTEXT_POISON=1 if you're ad-hoc running some git command yourself, e.g.: $ ./git status On branch master Your branch is up-to-date with 'origin/master'. nothing to commit, working tree clean $ GIT_GETTEXT_POISON=1 ./git status # GETTEXT POISON #master # GETTEXT POISON # ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [BUG] test suite broken with GETTEXT_POISON=YesPlease 2017-04-21 14:47 ` Michael J Gruber 2017-04-21 17:28 ` Ævar Arnfjörð Bjarmason @ 2017-04-24 1:18 ` Junio C Hamano [not found] ` <20170424110434.27689-1-avarab@gmail.com> 1 sibling, 1 reply; 21+ messages in thread From: Junio C Hamano @ 2017-04-24 1:18 UTC (permalink / raw) To: Michael J Gruber Cc: Ævar Arnfjörð Bjarmason, Git Mailing List, Jiang Xin, Lars Schneider Michael J Gruber <git@grubix.eu> writes: > Ævar Arnfjörð Bjarmason venit, vidit, dixit 20.04.2017 23:58: >> As a refresh of everyone's memory (because mine needed it). This is a >> feature I added back in 2011 when the i18n support was initially >> added. >> >> There was concern at the time that we would inadvertently mark >> plumbing messages for translation, particularly something in a shared >> code path, and this was a way to hopefully smoke out those issues with >> the test suite. >> >> However compiling with it breaks a couple of dozen tests, I stopped >> digging when I saw some broke back in 2014. >> >> What should be done about this? I think if we're going to keep them >> they need to be run regularly by something like Travis (Lars CC'd), >> however empirical evidence suggests that not running them is just fine >> too, so should we just remove support for this test mode? >> >> I don't care, but I can come up with the patch either way, but would >> only be motivated to write the one-time fix for it if some CI system >> is actually running them regularly, otherwise they'll just be subject >> to bitrotting again. > > I use that switch when I change something that involves l10n, but > usually I run specific tests only. To be honest: I have to make sure not > to get confused by (nor forget one of) the build flag GETTEXT_POISON and > the environment variable GIT_GETTEXT_POISON. I'm not sure I always > tested what I meant to test... To be quite honest, I have always felt that we are just as likely inadvertently use test_i18ncmp when we should use test_cmp (and vice versa) as we would mark plumbing messages with _() by mistake with this approach, and even with constant monitoring by something like Travis, GETTEXT_POISON may be able to catch mistakes only some of the time (i.e. when we do not make mistakes in writing our tests). Without constant monitoring, I agree that the mechanism does not work well to catch our mistakes. ^ permalink raw reply [flat|nested] 21+ messages in thread
[parent not found: <20170424110434.27689-1-avarab@gmail.com>]
* Re: [PATCH] tests: remove the GETTEXT_POISON compile-time option to test i18n marking [not found] ` <20170424110434.27689-1-avarab@gmail.com> @ 2017-04-26 9:17 ` Michael J Gruber 2017-04-26 14:27 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 21+ messages in thread From: Michael J Gruber @ 2017-04-26 9:17 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Git Mailing List, Junio C Hamano, Lars Schneider, Jiang Xin [Turns out I still can't operate gmail's web interface. Sorry for the dupe.] 2017-04-24 13:04 GMT+02:00 Ævar Arnfjörð Bjarmason <avarab@gmail.com>: > Remove the GETTEXT_POISON=YesPlease compile-time which turns all of > git's LC_*=C output into strings like "# GETTEXT POISON #" instead of > gettext(msgid). > > See commit bb946bba76 ("i18n: add GETTEXT_POISON to simulate > unfriendly translator", 2011-02-22) for what this was originally > intended for. > > This facility has been broken for quite a while and has been subjected > to frequent bitrot. The initial idea behind it back when it was added > in 2011 was to prevent the accidental translation of plumbing > messages. > > This isn't a big concern anymore as git isn't mass-adding i18n > messages for a newly developed i18n facility as it was back then, > maintaining this facility incurs a burden, and in actuality this has > often been broken long enough for potential plumbing messages to be > translated & make their way into major releases anyway. > > Most of this patch consists of search/replacing the test suite for: > > test_i18ngrep ! -> ! grep > test_i18ngrep -> grep > test_i18ncmp -> test_cmp > > 1. <AANLkTi=5MrU-JyeQ3UVNbVwzn-8FbstUXafgcQaLWXDB@mail.gmail.com> > (https://public-inbox.org/git/AANLkTi=5MrU-JyeQ3UVNbVwzn-8FbstUXafgcQaLWXDB@mail.gmail.com/) > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > > On Mon, Apr 24, 2017 at 3:18 AM, Junio C Hamano <gitster@pobox.com> wrote: >> Michael J Gruber <git@grubix.eu> writes: >> >>> Ævar Arnfjörð Bjarmason venit, vidit, dixit 20.04.2017 23:58: >>>> As a refresh of everyone's memory (because mine needed it). This is a >>>> feature I added back in 2011 when the i18n support was initially >>>> added. >>>> >>>> There was concern at the time that we would inadvertently mark >>>> plumbing messages for translation, particularly something in a shared >>>> code path, and this was a way to hopefully smoke out those issues with >>>> the test suite. >>>> >>>> However compiling with it breaks a couple of dozen tests, I stopped >>>> digging when I saw some broke back in 2014. >>>> >>>> What should be done about this? I think if we're going to keep them >>>> they need to be run regularly by something like Travis (Lars CC'd), >>>> however empirical evidence suggests that not running them is just fine >>>> too, so should we just remove support for this test mode? >>>> >>>> I don't care, but I can come up with the patch either way, but would >>>> only be motivated to write the one-time fix for it if some CI system >>>> is actually running them regularly, otherwise they'll just be subject >>>> to bitrotting again. >>> >>> I use that switch when I change something that involves l10n, but >>> usually I run specific tests only. To be honest: I have to make sure not >>> to get confused by (nor forget one of) the build flag GETTEXT_POISON and >>> the environment variable GIT_GETTEXT_POISON. I'm not sure I always >>> tested what I meant to test... >> >> To be quite honest, I have always felt that we are just as likely >> inadvertently use test_i18ncmp when we should use test_cmp (and vice >> versa) as we would mark plumbing messages with _() by mistake with >> this approach, and even with constant monitoring by something like >> Travis, GETTEXT_POISON may be able to catch mistakes only some of >> the time (i.e. when we do not make mistakes in writing our tests). >> Without constant monitoring, I agree that the mechanism does not >> work well to catch our mistakes. > > Here's an alternate patch to just remove it entirely. I think we > should apply this instead, the only reason I sent the patch to fix it > up was because of Michael's comment that he was occasionally using it. Yes, I think test_i18ngrep and test_i18ncmp gave the impression that they are i18n-aware grep and cmp, whereas in fact they turned off these tese test lines completely. Combined with the fact that GETTEXT_POSON builds turned on GIT_GETTEXT_POISON, this sounds somewhat dangerous - we test more aspects of plumbing commands but turn off (some) tests for porcelain. I'm still wondering wether we couldn't generate a test locale automatically by mangling the english strings (but preserving format specifiers). That way, tests that test porcelain output could require LANG=C while others could run in the mangled locale. (tt_TT is taken, though ;) ) Michael ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] tests: remove the GETTEXT_POISON compile-time option to test i18n marking 2017-04-26 9:17 ` [PATCH] tests: remove the GETTEXT_POISON compile-time option to test i18n marking Michael J Gruber @ 2017-04-26 14:27 ` Ævar Arnfjörð Bjarmason 0 siblings, 0 replies; 21+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-04-26 14:27 UTC (permalink / raw) To: Michael J Gruber Cc: Git Mailing List, Junio C Hamano, Lars Schneider, Jiang Xin On Wed, Apr 26, 2017 at 11:17 AM, Michael J Gruber <git@grubix.eu> wrote: > [Turns out I still can't operate gmail's web interface. Sorry for the dupe.] > > 2017-04-24 13:04 GMT+02:00 Ævar Arnfjörð Bjarmason <avarab@gmail.com>: >> Remove the GETTEXT_POISON=YesPlease compile-time which turns all of >> git's LC_*=C output into strings like "# GETTEXT POISON #" instead of >> gettext(msgid). >> >> See commit bb946bba76 ("i18n: add GETTEXT_POISON to simulate >> unfriendly translator", 2011-02-22) for what this was originally >> intended for. >> >> This facility has been broken for quite a while and has been subjected >> to frequent bitrot. The initial idea behind it back when it was added >> in 2011 was to prevent the accidental translation of plumbing >> messages. >> >> This isn't a big concern anymore as git isn't mass-adding i18n >> messages for a newly developed i18n facility as it was back then, >> maintaining this facility incurs a burden, and in actuality this has >> often been broken long enough for potential plumbing messages to be >> translated & make their way into major releases anyway. >> >> Most of this patch consists of search/replacing the test suite for: >> >> test_i18ngrep ! -> ! grep >> test_i18ngrep -> grep >> test_i18ncmp -> test_cmp >> >> 1. <AANLkTi=5MrU-JyeQ3UVNbVwzn-8FbstUXafgcQaLWXDB@mail.gmail.com> >> (https://public-inbox.org/git/AANLkTi=5MrU-JyeQ3UVNbVwzn-8FbstUXafgcQaLWXDB@mail.gmail.com/) >> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> >> --- >> >> On Mon, Apr 24, 2017 at 3:18 AM, Junio C Hamano <gitster@pobox.com> wrote: >>> Michael J Gruber <git@grubix.eu> writes: >>> >>>> Ævar Arnfjörð Bjarmason venit, vidit, dixit 20.04.2017 23:58: >>>>> As a refresh of everyone's memory (because mine needed it). This is a >>>>> feature I added back in 2011 when the i18n support was initially >>>>> added. >>>>> >>>>> There was concern at the time that we would inadvertently mark >>>>> plumbing messages for translation, particularly something in a shared >>>>> code path, and this was a way to hopefully smoke out those issues with >>>>> the test suite. >>>>> >>>>> However compiling with it breaks a couple of dozen tests, I stopped >>>>> digging when I saw some broke back in 2014. >>>>> >>>>> What should be done about this? I think if we're going to keep them >>>>> they need to be run regularly by something like Travis (Lars CC'd), >>>>> however empirical evidence suggests that not running them is just fine >>>>> too, so should we just remove support for this test mode? >>>>> >>>>> I don't care, but I can come up with the patch either way, but would >>>>> only be motivated to write the one-time fix for it if some CI system >>>>> is actually running them regularly, otherwise they'll just be subject >>>>> to bitrotting again. >>>> >>>> I use that switch when I change something that involves l10n, but >>>> usually I run specific tests only. To be honest: I have to make sure not >>>> to get confused by (nor forget one of) the build flag GETTEXT_POISON and >>>> the environment variable GIT_GETTEXT_POISON. I'm not sure I always >>>> tested what I meant to test... >>> >>> To be quite honest, I have always felt that we are just as likely >>> inadvertently use test_i18ncmp when we should use test_cmp (and vice >>> versa) as we would mark plumbing messages with _() by mistake with >>> this approach, and even with constant monitoring by something like >>> Travis, GETTEXT_POISON may be able to catch mistakes only some of >>> the time (i.e. when we do not make mistakes in writing our tests). >>> Without constant monitoring, I agree that the mechanism does not >>> work well to catch our mistakes. >> >> Here's an alternate patch to just remove it entirely. I think we >> should apply this instead, the only reason I sent the patch to fix it >> up was because of Michael's comment that he was occasionally using it. > > Yes, I think test_i18ngrep and test_i18ncmp gave the impression that > they are i18n-aware grep and cmp, whereas in fact they turned off > these tese test lines completely. > Combined with the fact that GETTEXT_POSON builds turned on > GIT_GETTEXT_POISON, this sounds somewhat dangerous - we test more > aspects of plumbing commands but turn off (some) tests for porcelain. It gives us no less test coverage because we still run the tests without GETTEXT_POSON. Thus running first without that & then with gives 100% coverage. The entire point of the GETTEXT_POSON mode is that we've already manually gone over things that broke in the past, and are skipping them with C_LOCALE_OUTPUT or one of these i18n functions, and when adding new translations we keep an eye out for new breakages that haven't been marked yet. These are then a candidate either for marking to skip, if they're porcelain commands using our new gettext()-utilizing code, or we've made a mistake and translated some plumbing, in which case that would hopefully be caught by list review or Travis. I still think it's better to just get rid of it, but this aspect of it is working exactly as intended. > I'm still wondering wether we couldn't generate a test locale > automatically by mangling the english strings (but preserving format > specifiers). That way, tests that test porcelain output could require > LANG=C while others could run in the mangled locale. (tt_TT is taken, > though ;) ) If we wanted to keep GETTEXT_POSON this would be going further in the wrong direction. The reason the message starts with a "#", has no trailing \n & most importantly ignores any format specifiers the real translation would have is exactly because we'd like as many things as can break break under this mode. If anything we should consider changing "# GETTEXT_POISON #" to "<random binary garbage>" if we keep it. Including format specifiers would just reduce the set of things that would break, consider e.g. a test that merely greps for some substring that would be emitted by a "%s branch %d commits ahead" format, now we change that to "%s GIBBER %d ISH", but the test still passes because it just does $(awk '{print $1}'), but it might be a plumbing message that could be translated as e.g. "%d WORD1 %s WORD2 WORD3", breaking the API. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [BUG] test suite broken with GETTEXT_POISON=YesPlease 2017-04-20 21:58 [BUG] test suite broken with GETTEXT_POISON=YesPlease Ævar Arnfjörð Bjarmason 2017-04-21 14:47 ` Michael J Gruber @ 2017-04-21 14:54 ` Lars Schneider 2017-04-21 18:05 ` Ævar Arnfjörð Bjarmason ` (2 more replies) 1 sibling, 3 replies; 21+ messages in thread From: Lars Schneider @ 2017-04-21 14:54 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Git Mailing List, Jiang Xin, Junio C Hamano > Am 20.04.2017 um 23:58 schrieb Ævar Arnfjörð Bjarmason <avarab@gmail.com>: > > As a refresh of everyone's memory (because mine needed it). This is a > feature I added back in 2011 when the i18n support was initially > added. > > There was concern at the time that we would inadvertently mark > plumbing messages for translation, particularly something in a shared > code path, and this was a way to hopefully smoke out those issues with > the test suite. > > However compiling with it breaks a couple of dozen tests, I stopped > digging when I saw some broke back in 2014. > > What should be done about this? I think if we're going to keep them > they need to be run regularly by something like Travis (Lars CC'd), > however empirical evidence suggests that not running them is just fine > too, so should we just remove support for this test mode? Right now we are building and testing Git in the following configurations: 1. Linux, gcc, stable Perforce an GitLFS version (used by git-p4 tests) 2. Linux, gcc, stable Perforce an GitLFS version (used by git-p4 tests) * 3. OSX, clang, latest Perforce an GitLFS version (used by git-p4 tests) 4. OSX, clang, latest Perforce an GitLFS version (used by git-p4 tests) * 5. Linux32, gcc, no git-p4 tests 6. Windows, gcc, no git-p4 tests 1-4 run the same tests right now. This was especially useful in the beginning to identify flaky tests (t0025 is still flaky!). We could easily run the tests in 1-4 with different configurations. E.g. enable GETTEXT_POISON in 2. Cheers, Lars *) 2 and 4 use the wrong compiler right now. 2 should use clang on Linux and 4 should use gcc. A patch is on my todo list. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [BUG] test suite broken with GETTEXT_POISON=YesPlease 2017-04-21 14:54 ` [BUG] test suite broken with GETTEXT_POISON=YesPlease Lars Schneider @ 2017-04-21 18:05 ` Ævar Arnfjörð Bjarmason 2017-04-21 18:57 ` [PATCH] tests: fix tests broken under GETTEXT_POISON=YesPlease Ævar Arnfjörð Bjarmason 2017-04-24 13:22 ` [BUG] test suite broken with GETTEXT_POISON=YesPlease Duy Nguyen 2 siblings, 0 replies; 21+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-04-21 18:05 UTC (permalink / raw) To: Lars Schneider; +Cc: Git Mailing List, Jiang Xin, Junio C Hamano On Fri, Apr 21, 2017 at 4:54 PM, Lars Schneider <larsxschneider@gmail.com> wrote: > >> Am 20.04.2017 um 23:58 schrieb Ævar Arnfjörð Bjarmason <avarab@gmail.com>: >> >> As a refresh of everyone's memory (because mine needed it). This is a >> feature I added back in 2011 when the i18n support was initially >> added. >> >> There was concern at the time that we would inadvertently mark >> plumbing messages for translation, particularly something in a shared >> code path, and this was a way to hopefully smoke out those issues with >> the test suite. >> >> However compiling with it breaks a couple of dozen tests, I stopped >> digging when I saw some broke back in 2014. >> >> What should be done about this? I think if we're going to keep them >> they need to be run regularly by something like Travis (Lars CC'd), >> however empirical evidence suggests that not running them is just fine >> too, so should we just remove support for this test mode? > > Right now we are building and testing Git in the following configurations: > > 1. Linux, gcc, stable Perforce an GitLFS version (used by git-p4 tests) > 2. Linux, gcc, stable Perforce an GitLFS version (used by git-p4 tests) * > 3. OSX, clang, latest Perforce an GitLFS version (used by git-p4 tests) > 4. OSX, clang, latest Perforce an GitLFS version (used by git-p4 tests) * > 5. Linux32, gcc, no git-p4 tests > 6. Windows, gcc, no git-p4 tests > > 1-4 run the same tests right now. This was especially useful in the beginning to identify flaky tests (t0025 is still flaky!). > > We could easily run the tests in 1-4 with different configurations. E.g. enable GETTEXT_POISON in 2. > > Cheers, > Lars > > *) 2 and 4 use the wrong compiler right now. 2 should use clang on Linux and 4 should use gcc. A patch is on my todo list. Great, thanks. I'll fixup the poison tests then. ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] tests: fix tests broken under GETTEXT_POISON=YesPlease 2017-04-21 14:54 ` [BUG] test suite broken with GETTEXT_POISON=YesPlease Lars Schneider 2017-04-21 18:05 ` Ævar Arnfjörð Bjarmason @ 2017-04-21 18:57 ` Ævar Arnfjörð Bjarmason 2017-04-24 1:15 ` Junio C Hamano 2017-04-24 13:22 ` [BUG] test suite broken with GETTEXT_POISON=YesPlease Duy Nguyen 2 siblings, 1 reply; 21+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-04-21 18:57 UTC (permalink / raw) To: git Cc: Junio C Hamano, Michael J Gruber, Lars Schneider, Ævar Arnfjörð Bjarmason 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> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/t1309-early-config.sh | 2 +- t/t1430-bad-ref-name.sh | 2 +- t/t3203-branch-output.sh | 2 +- t/t3404-rebase-interactive.sh | 14 +++++++------- t/t3415-rebase-autosquash.sh | 10 +++++----- t/t3903-stash.sh | 4 ++-- t/t4205-log-pretty-formats.sh | 4 ++-- t/t5316-pack-delta-depth.sh | 8 ++++++-- t/t6134-pathspec-in-submodule.sh | 4 ++-- t/t7004-tag.sh | 4 ++-- t/t7406-submodule-update.sh | 2 +- t/t7509-commit.sh | 4 ++-- t/t7800-difftool.sh | 4 ++-- 13 files changed, 34 insertions(+), 30 deletions(-) diff --git a/t/t1309-early-config.sh b/t/t1309-early-config.sh index b97357b8ab..016e43b8d5 100755 --- a/t/t1309-early-config.sh +++ b/t/t1309-early-config.sh @@ -59,7 +59,7 @@ test_with_config () { test_expect_success 'ignore .git/ with incompatible repository version' ' test_with_config "[core]repositoryformatversion = 999999" 2>err && - grep "warning:.* Expected git repo version <= [1-9]" err + test_i18ngrep "warning:.* Expected git repo version <= [1-9]" err ' test_expect_failure 'ignore .git/ with invalid repository version' ' 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 && 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 ' test_done 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 --continue' ' grep "^ file1 | 2 +-$" output ' -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" \ @@ -376,7 +376,7 @@ test_expect_success 'multi-squash only fires up editor once' ' test 1 = $(git show | grep ONCE | wc -l) ' -test_expect_success 'multi-fixup does not fire up editor' ' +test_expect_success !GETTEXT_POISON 'multi-fixup does not fire up editor' ' git checkout -b multi-fixup E && base=$(git rev-parse HEAD~4) && set_fake_editor && @@ -426,7 +426,7 @@ D ONCE EOF -test_expect_success 'squash and fixup generate correct log messages' ' +test_expect_success !GETTEXT_POISON 'squash and fixup generate correct log messages' ' git checkout -b squash-fixup E && base=$(git rev-parse HEAD~4) && set_fake_editor && @@ -439,7 +439,7 @@ test_expect_success 'squash and fixup generate correct log messages' ' git branch -D squash-fixup ' -test_expect_success 'squash ignores comments' ' +test_expect_success !GETTEXT_POISON 'squash ignores comments' ' git checkout -b skip-comments E && base=$(git rev-parse HEAD~4) && set_fake_editor && @@ -452,7 +452,7 @@ test_expect_success 'squash ignores comments' ' git branch -D skip-comments ' -test_expect_success 'squash ignores blank lines' ' +test_expect_success !GETTEXT_POISON 'squash ignores blank lines' ' git checkout -b skip-blank-lines E && base=$(git rev-parse HEAD~4) && set_fake_editor && @@ -860,7 +860,7 @@ test_expect_success 'rebase -ix with several instances of --exec' ' test_cmp expected actual ' -test_expect_success 'rebase -ix with --autosquash' ' +test_expect_success !GETTEXT_POISON 'rebase -ix with --autosquash' ' git reset --hard execute && git checkout -b autosquash && echo second >second.txt && @@ -943,7 +943,7 @@ test_expect_success 'rebase -i --root fixup root commit' ' test 0 = $(git cat-file commit HEAD | grep -c ^parent\ ) ' -test_expect_success 'rebase --edit-todo does not works on non-interactive rebase' ' +test_expect_success !GETTEXT_POISON 'rebase --edit-todo does not work on non-interactive rebase' ' git reset --hard && git checkout conflict-branch && set_fake_editor && diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh index 48346f1cc0..b7265460f5 100755 --- a/t/t3415-rebase-autosquash.sh +++ b/t/t3415-rebase-autosquash.sh @@ -234,23 +234,23 @@ test_auto_fixup_fixup () { fi } -test_expect_success 'fixup! fixup!' ' +test_expect_success !GETTEXT_POISON 'fixup! fixup!' ' test_auto_fixup_fixup fixup fixup ' -test_expect_success 'fixup! squash!' ' +test_expect_success !GETTEXT_POISON 'fixup! squash!' ' test_auto_fixup_fixup fixup squash ' -test_expect_success 'squash! squash!' ' +test_expect_success !GETTEXT_POISON 'squash! squash!' ' test_auto_fixup_fixup squash squash ' -test_expect_success 'squash! fixup!' ' +test_expect_success !GETTEXT_POISON 'squash! fixup!' ' test_auto_fixup_fixup squash fixup ' -test_expect_success 'autosquash with custom inst format' ' +test_expect_success !GETTEXT_POISON 'autosquash with custom inst format' ' git reset --hard base && git config --add rebase.instructionFormat "[%an @ %ar] %s" && echo 2 >file1 && diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index b71d1e659e..3b4bed5c9a 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -865,7 +865,7 @@ test_expect_success 'stash push -p with pathspec shows no changes only once' ' git stash push -p foo >actual && echo "No local changes to save" >expect && git reset --hard HEAD~ && - test_cmp expect actual + test_i18ncmp expect actual ' test_expect_success 'stash push with pathspec shows no changes when there are none' ' @@ -875,7 +875,7 @@ test_expect_success 'stash push with pathspec shows no changes when there are no git stash push foo >actual && echo "No local changes to save" >expect && git reset --hard HEAD~ && - test_cmp expect actual + test_i18ncmp expect actual ' test_expect_success 'stash push with pathspec not in the repository errors out' ' diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index 21eb8c8587..1322bd461b 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -126,12 +126,12 @@ test_expect_success 'NUL separation with --stat' ' test_i18ncmp expected actual ' -test_expect_failure 'NUL termination with --stat' ' +test_expect_failure !GETTEXT_POISON 'NUL termination with --stat' ' stat0_part=$(git diff --stat HEAD^ HEAD) && stat1_part=$(git diff-tree --no-commit-id --stat --root HEAD^) && printf "add bar\n$stat0_part\n\0$(commit_msg)\n$stat1_part\n0" >expected && git log -z --stat --pretty="tformat:%s" >actual && - test_i18ncmp expected actual + test_cmp expected actual ' test_expect_success 'setup more commits' ' 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 ' 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 ' test_done diff --git a/t/t6134-pathspec-in-submodule.sh b/t/t6134-pathspec-in-submodule.sh index fd401ca605..99a8982ab1 100755 --- a/t/t6134-pathspec-in-submodule.sh +++ b/t/t6134-pathspec-in-submodule.sh @@ -21,7 +21,7 @@ EOF test_expect_success 'error message for path inside submodule' ' echo a >sub/a && test_must_fail git add sub/a 2>actual && - test_cmp expect actual + test_i18ncmp expect actual ' cat <<EOF >expect @@ -30,7 +30,7 @@ EOF test_expect_success 'error message for path inside submodule from within submodule' ' test_must_fail git -C sub add . 2>actual && - test_cmp expect actual + test_i18ncmp expect actual ' test_done diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index bb2e4d704d..0ef7b94394 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -87,7 +87,7 @@ test_expect_success 'creating a tag with --create-reflog should create reflog' ' git tag --create-reflog tag_with_reflog && git reflog exists refs/tags/tag_with_reflog && sed -e "s/^.* //" .git/logs/refs/tags/tag_with_reflog >actual && - test_cmp expected actual + test_i18ncmp expected actual ' test_expect_success 'annotated tag with --create-reflog has correct message' ' @@ -98,7 +98,7 @@ test_expect_success 'annotated tag with --create-reflog has correct message' ' git tag -m "annotated tag" --create-reflog tag_with_reflog && git reflog exists refs/tags/tag_with_reflog && sed -e "s/^.* //" .git/logs/refs/tags/tag_with_reflog >actual && - test_cmp expected actual + test_i18ncmp expected actual ' test_expect_success '--create-reflog does not create reflog on failure' ' diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh index 4ac386d98b..034914a14f 100755 --- a/t/t7406-submodule-update.sh +++ b/t/t7406-submodule-update.sh @@ -447,7 +447,7 @@ test_expect_success 'submodule update - command run for initial population of su EOF rm -rf super/submodule && test_must_fail git -C super submodule update 2>actual && - test_cmp expect actual && + test_i18ncmp expect actual && git -C super submodule update --checkout ' diff --git a/t/t7509-commit.sh b/t/t7509-commit.sh index db9774e345..ddef7ea6b0 100755 --- a/t/t7509-commit.sh +++ b/t/t7509-commit.sh @@ -101,7 +101,7 @@ test_expect_success '--amend option with empty author' ' echo "Empty author test" >>foo && test_tick && test_must_fail git commit -a -m "empty author" --amend 2>err && - grep "empty ident" err + test_i18ngrep "empty ident" err ' test_expect_success '--amend option with missing author' ' @@ -114,7 +114,7 @@ test_expect_success '--amend option with missing author' ' echo "Missing author test" >>foo && test_tick && test_must_fail git commit -a -m "malformed author" --amend 2>err && - grep "empty ident" err + test_i18ngrep "empty ident" err ' test_expect_success '--reset-author makes the commit ours even with --amend option' ' diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh index 7f09867478..668bbee73c 100755 --- a/t/t7800-difftool.sh +++ b/t/t7800-difftool.sh @@ -25,14 +25,14 @@ prompt_given () test_expect_success 'basic usage requires no repo' ' test_expect_code 129 git difftool -h >output && - grep ^usage: output && + test_i18ngrep ^usage: output && # create a ceiling directory to prevent Git from finding a repo mkdir -p not/repo && test_when_finished rm -r not && test_expect_code 129 \ env GIT_CEILING_DIRECTORIES="$(pwd)/not" \ git -C not/repo difftool -h >output && - grep ^usage: output + test_i18ngrep ^usage: output ' # Create a file on master and change it on branch -- 2.11.0 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] tests: fix tests broken under GETTEXT_POISON=YesPlease 2017-04-21 18:57 ` [PATCH] tests: fix tests broken under GETTEXT_POISON=YesPlease Ævar Arnfjörð Bjarmason @ 2017-04-24 1:15 ` Junio C Hamano 2017-04-24 8:18 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 21+ messages in thread From: Junio C Hamano @ 2017-04-24 1:15 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Michael J Gruber, Lars Schneider Æ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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] tests: fix tests broken under GETTEXT_POISON=YesPlease 2017-04-24 1:15 ` Junio C Hamano @ 2017-04-24 8:18 ` Ævar Arnfjörð Bjarmason 2017-04-25 4:11 ` Junio C Hamano 0 siblings, 1 reply; 21+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-04-24 8:18 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git Mailing List, Michael J Gruber, Lars Schneider 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] tests: fix tests broken under GETTEXT_POISON=YesPlease 2017-04-24 8:18 ` Ævar Arnfjörð Bjarmason @ 2017-04-25 4:11 ` Junio C Hamano 2017-04-25 8:56 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 21+ messages in thread From: Junio C Hamano @ 2017-04-25 4:11 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Git Mailing List, Michael J Gruber, Lars Schneider Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > 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. As to skipping tests, I am worried mostly because it is very easy to mark one test as skipped under poison build, even where the side effect from that test left behind in the trash repository is a prerequisite for a later test to succeed. For example, a test that creates a tag may be marked as skipped-under-poison. Then a new test that is added to such a test may want to do something using that tag, and it will succeed in the usual test. As most people do not test poison build, when somebody notices that the new test fails under poison build, it is unclear if the breakage is due to new i18n issues or something else, like a missing prerequisite tag due to skipping an earlier test. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] tests: fix tests broken under GETTEXT_POISON=YesPlease 2017-04-25 4:11 ` Junio C Hamano @ 2017-04-25 8:56 ` Ævar Arnfjörð Bjarmason 2017-04-26 1:58 ` Junio C Hamano 0 siblings, 1 reply; 21+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-04-25 8:56 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git Mailing List, Michael J Gruber, Lars Schneider On Tue, Apr 25, 2017 at 6:11 AM, Junio C Hamano <gitster@pobox.com> wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> 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. > > As to skipping tests, I am worried mostly because it is very easy to > mark one test as skipped under poison build, even where the side > effect from that test left behind in the trash repository is a > prerequisite for a later test to succeed. For example, a test that > creates a tag may be marked as skipped-under-poison. Then a new > test that is added to such a test may want to do something using > that tag, and it will succeed in the usual test. As most people do > not test poison build, when somebody notices that the new test fails > under poison build, it is unclear if the breakage is due to new i18n > issues or something else, like a missing prerequisite tag due to > skipping an earlier test. Indeed, I've tried to be careful not to introduce bugs like that, but in this skipped case the tests look completely stand-alone to me. In any case, I like my other patch to just remove this whole thing better. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] tests: fix tests broken under GETTEXT_POISON=YesPlease 2017-04-25 8:56 ` Ævar Arnfjörð Bjarmason @ 2017-04-26 1:58 ` Junio C Hamano 2017-04-26 7:56 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 21+ messages in thread From: Junio C Hamano @ 2017-04-26 1:58 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Git Mailing List, Michael J Gruber, Lars Schneider Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Indeed, I've tried to be careful not to introduce bugs like that, but > in this skipped case the tests look completely stand-alone to me. Yes, the ones I commented on in the upthread looked like their side effect were not felt in the later tests. > In any case, I like my other patch to just remove this whole thing better. Hmph, did I miss a patch message? ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] tests: fix tests broken under GETTEXT_POISON=YesPlease 2017-04-26 1:58 ` Junio C Hamano @ 2017-04-26 7:56 ` Ævar Arnfjörð Bjarmason 0 siblings, 0 replies; 21+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-04-26 7:56 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git Mailing List, Michael J Gruber, Lars Schneider On Wed, Apr 26, 2017 at 3:58 AM, Junio C Hamano <gitster@pobox.com> wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> Indeed, I've tried to be careful not to introduce bugs like that, but >> in this skipped case the tests look completely stand-alone to me. > > Yes, the ones I commented on in the upthread looked like their side > effect were not felt in the later tests. > >> In any case, I like my other patch to just remove this whole thing better. > > Hmph, did I miss a patch message? I sent it to you & CC'd the list, but it looks like it didn't make it on-list. It's "[PATCH] tests: remove the GETTEXT_POISON compile-time option to test i18n marking" (<20170424110434.27689-1-avarab@gmail.com>). It's 240K & 6k lines, so perhaps the list is rejecting it, here's the patch: https://raw.githubusercontent.com/avar/git-patches/master/remove-gettext-poison/0001-tests-remove-the-GETTEXT_POISON-compile-time-option-.patch ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [BUG] test suite broken with GETTEXT_POISON=YesPlease 2017-04-21 14:54 ` [BUG] test suite broken with GETTEXT_POISON=YesPlease Lars Schneider 2017-04-21 18:05 ` Ævar Arnfjörð Bjarmason 2017-04-21 18:57 ` [PATCH] tests: fix tests broken under GETTEXT_POISON=YesPlease Ævar Arnfjörð Bjarmason @ 2017-04-24 13:22 ` Duy Nguyen 2017-04-24 20:37 ` Jeff King 2 siblings, 1 reply; 21+ messages in thread From: Duy Nguyen @ 2017-04-24 13:22 UTC (permalink / raw) To: Lars Schneider Cc: Ævar Arnfjörð Bjarmason, Git Mailing List, Jiang Xin, Junio C Hamano On Fri, Apr 21, 2017 at 9:54 PM, Lars Schneider <larsxschneider@gmail.com> wrote: > >> Am 20.04.2017 um 23:58 schrieb Ævar Arnfjörð Bjarmason <avarab@gmail.com>: >> >> As a refresh of everyone's memory (because mine needed it). This is a >> feature I added back in 2011 when the i18n support was initially >> added. >> >> There was concern at the time that we would inadvertently mark >> plumbing messages for translation, particularly something in a shared >> code path, and this was a way to hopefully smoke out those issues with >> the test suite. >> >> However compiling with it breaks a couple of dozen tests, I stopped >> digging when I saw some broke back in 2014. >> >> What should be done about this? I think if we're going to keep them >> they need to be run regularly by something like Travis (Lars CC'd), >> however empirical evidence suggests that not running them is just fine >> too, so should we just remove support for this test mode? > > Right now we are building and testing Git in the following configurations: > > 1. Linux, gcc, stable Perforce an GitLFS version (used by git-p4 tests) > 2. Linux, gcc, stable Perforce an GitLFS version (used by git-p4 tests) * > 3. OSX, clang, latest Perforce an GitLFS version (used by git-p4 tests) > 4. OSX, clang, latest Perforce an GitLFS version (used by git-p4 tests) * > 5. Linux32, gcc, no git-p4 tests > 6. Windows, gcc, no git-p4 tests > > 1-4 run the same tests right now. This was especially useful in the beginning to identify flaky tests (t0025 is still flaky!). > > We could easily run the tests in 1-4 with different configurations. E.g. enable GETTEXT_POISON in 2. If CI stays idle some time, I suggest adding a test run with valgrind on 'pu' or 'next'. If there's still spare capacity I would add another test run with GIT_TEST_SPLIT_INDEX=1 (but not right now, it's broken) Off topic, is it possible to receive mail notifications from Travis when a fault is found in either 'pu', 'next' or 'master'? I know how to do it in Jenkins, but I'm not familiar with Travis and there's no obvious button from the web page.. -- Duy ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [BUG] test suite broken with GETTEXT_POISON=YesPlease 2017-04-24 13:22 ` [BUG] test suite broken with GETTEXT_POISON=YesPlease Duy Nguyen @ 2017-04-24 20:37 ` Jeff King 2017-04-25 8:51 ` Lars Schneider 0 siblings, 1 reply; 21+ messages in thread From: Jeff King @ 2017-04-24 20:37 UTC (permalink / raw) To: Duy Nguyen Cc: Lars Schneider, Ævar Arnfjörð Bjarmason, Git Mailing List, Jiang Xin, Junio C Hamano On Mon, Apr 24, 2017 at 08:22:36PM +0700, Duy Nguyen wrote: > Off topic, is it possible to receive mail notifications from Travis > when a fault is found in either 'pu', 'next' or 'master'? I know how > to do it in Jenkins, but I'm not familiar with Travis and there's no > obvious button from the web page.. I looked into this a bit for my personal builds. Notification config has to go into the .travis.yml file[1]. So I think the best we could do is send a notification email to some mailing list, and then let people subscribe to that (or it could go to git@vger; I don't know how noisy it would be). -Peff [1] I wanted to set up an email just to me for my personal fork, but I couldn't make it work. AFAICT there's no way to override upstream's config short of adding another commit, which would mean I'd have to base all my branches on a "fix up .travis.yml" commit. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [BUG] test suite broken with GETTEXT_POISON=YesPlease 2017-04-24 20:37 ` Jeff King @ 2017-04-25 8:51 ` Lars Schneider 2017-04-25 10:01 ` Jeff King 0 siblings, 1 reply; 21+ messages in thread From: Lars Schneider @ 2017-04-25 8:51 UTC (permalink / raw) To: Jeff King Cc: Duy Nguyen, Ævar Arnfjörð Bjarmason, Git Mailing List, Jiang Xin, Junio C Hamano > On 24 Apr 2017, at 22:37, Jeff King <peff@peff.net> wrote: > > On Mon, Apr 24, 2017 at 08:22:36PM +0700, Duy Nguyen wrote: > >> Off topic, is it possible to receive mail notifications from Travis >> when a fault is found in either 'pu', 'next' or 'master'? I know how >> to do it in Jenkins, but I'm not familiar with Travis and there's no >> obvious button from the web page.. > > I looked into this a bit for my personal builds. Notification config has > to go into the .travis.yml file[1]. So I think the best we could do is > send a notification email to some mailing list, and then let people > subscribe to that (or it could go to git@vger; I don't know how noisy it > would be). A separate mailing list sounds like a very good idea to me! Maybe "git-builds@vger.kernel.org" or something? What would it take to set something up like this? - Lars ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [BUG] test suite broken with GETTEXT_POISON=YesPlease 2017-04-25 8:51 ` Lars Schneider @ 2017-04-25 10:01 ` Jeff King 2017-04-28 8:42 ` Lars Schneider 0 siblings, 1 reply; 21+ messages in thread From: Jeff King @ 2017-04-25 10:01 UTC (permalink / raw) To: Lars Schneider Cc: Duy Nguyen, Ævar Arnfjörð Bjarmason, Git Mailing List, Jiang Xin, Junio C Hamano On Tue, Apr 25, 2017 at 10:51:20AM +0200, Lars Schneider wrote: > >> Off topic, is it possible to receive mail notifications from Travis > >> when a fault is found in either 'pu', 'next' or 'master'? I know how > >> to do it in Jenkins, but I'm not familiar with Travis and there's no > >> obvious button from the web page.. > > > > I looked into this a bit for my personal builds. Notification config has > > to go into the .travis.yml file[1]. So I think the best we could do is > > send a notification email to some mailing list, and then let people > > subscribe to that (or it could go to git@vger; I don't know how noisy it > > would be). > > A separate mailing list sounds like a very good idea to me! > Maybe "git-builds@vger.kernel.org" or something? > What would it take to set something up like this? I suspect that emailing the vger admins is the right place (or that they can point us in the right direction, or tell us to get lost). The best address is probably postmaster@vger.kernel.org. (I resisted just cc-ing them here to see if other people had opinions on just sending the output to the regular list). -Peff ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [BUG] test suite broken with GETTEXT_POISON=YesPlease 2017-04-25 10:01 ` Jeff King @ 2017-04-28 8:42 ` Lars Schneider 2017-04-28 8:44 ` Jeff King 0 siblings, 1 reply; 21+ messages in thread From: Lars Schneider @ 2017-04-28 8:42 UTC (permalink / raw) To: Jeff King Cc: Duy Nguyen, Ævar Arnfjörð Bjarmason, Git Mailing List, Jiang Xin, Junio C Hamano > On 25 Apr 2017, at 12:01, Jeff King <peff@peff.net> wrote: > > On Tue, Apr 25, 2017 at 10:51:20AM +0200, Lars Schneider wrote: > >>>> Off topic, is it possible to receive mail notifications from Travis >>>> when a fault is found in either 'pu', 'next' or 'master'? I know how >>>> to do it in Jenkins, but I'm not familiar with Travis and there's no >>>> obvious button from the web page.. >>> >>> I looked into this a bit for my personal builds. Notification config has >>> to go into the .travis.yml file[1]. So I think the best we could do is >>> send a notification email to some mailing list, and then let people >>> subscribe to that (or it could go to git@vger; I don't know how noisy it >>> would be). >> >> A separate mailing list sounds like a very good idea to me! >> Maybe "git-builds@vger.kernel.org" or something? >> What would it take to set something up like this? > > I suspect that emailing the vger admins is the right place (or that they > can point us in the right direction, or tell us to get lost). The best > address is probably postmaster@vger.kernel.org. > > (I resisted just cc-ing them here to see if other people had opinions on > just sending the output to the regular list). OK, I'll send them an email. I just realized a strong reason for a separate mailing list: I haven't found a knob to make TravisCI send plain text emails. - Lars ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [BUG] test suite broken with GETTEXT_POISON=YesPlease 2017-04-28 8:42 ` Lars Schneider @ 2017-04-28 8:44 ` Jeff King 0 siblings, 0 replies; 21+ messages in thread From: Jeff King @ 2017-04-28 8:44 UTC (permalink / raw) To: Lars Schneider Cc: Duy Nguyen, Ævar Arnfjörð Bjarmason, Git Mailing List, Jiang Xin, Junio C Hamano On Fri, Apr 28, 2017 at 10:42:10AM +0200, Lars Schneider wrote: > >> A separate mailing list sounds like a very good idea to me! > >> Maybe "git-builds@vger.kernel.org" or something? > >> What would it take to set something up like this? > > > > I suspect that emailing the vger admins is the right place (or that they > > can point us in the right direction, or tell us to get lost). The best > > address is probably postmaster@vger.kernel.org. > > > > (I resisted just cc-ing them here to see if other people had opinions on > > just sending the output to the regular list). > > OK, I'll send them an email. I just realized a strong reason for a > separate mailing list: I haven't found a knob to make TravisCI > send plain text emails. I think that's going to be a problem for any list on vger. I suspect the spam filtering all happens at an early stage. You might consider just setting up a google group or some other list as an initial experiment. It wouldn't be that hard to switch the list later (people will need to resubscribe, but you can notify them by sending an email to...the list). -Peff ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2017-04-28 8:44 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-20 21:58 [BUG] test suite broken with GETTEXT_POISON=YesPlease Ævar Arnfjörð Bjarmason 2017-04-21 14:47 ` Michael J Gruber 2017-04-21 17:28 ` Ævar Arnfjörð Bjarmason 2017-04-24 1:18 ` Junio C Hamano [not found] ` <20170424110434.27689-1-avarab@gmail.com> 2017-04-26 9:17 ` [PATCH] tests: remove the GETTEXT_POISON compile-time option to test i18n marking Michael J Gruber 2017-04-26 14:27 ` Ævar Arnfjörð Bjarmason 2017-04-21 14:54 ` [BUG] test suite broken with GETTEXT_POISON=YesPlease Lars Schneider 2017-04-21 18:05 ` Ævar Arnfjörð Bjarmason 2017-04-21 18:57 ` [PATCH] tests: fix tests broken under GETTEXT_POISON=YesPlease Ævar Arnfjörð Bjarmason 2017-04-24 1:15 ` Junio C Hamano 2017-04-24 8:18 ` Ævar Arnfjörð Bjarmason 2017-04-25 4:11 ` Junio C Hamano 2017-04-25 8:56 ` Ævar Arnfjörð Bjarmason 2017-04-26 1:58 ` Junio C Hamano 2017-04-26 7:56 ` Ævar Arnfjörð Bjarmason 2017-04-24 13:22 ` [BUG] test suite broken with GETTEXT_POISON=YesPlease Duy Nguyen 2017-04-24 20:37 ` Jeff King 2017-04-25 8:51 ` Lars Schneider 2017-04-25 10:01 ` Jeff King 2017-04-28 8:42 ` Lars Schneider 2017-04-28 8:44 ` Jeff King
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).