Re: [PATCH 2/2] t3404: be resilient against running with the -x flag

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

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> To: Junio C Hamano <gitster@pobox.com>
> Cc: git@vger.kernel.org

Probably the above is the other way around.

> The -x flag (trace commands) is a priceless tool when hunting down bugs
> that trigger test failures. It is a worthless tool if the -x flag
> *itself* triggers test failures.

True.

I wonder if we can fix "-x" instead so that we do not have to
butcher tests like this patch does.  It was quite clear what it
expected to see before this patch, and it is sad that the workaround
makes less readable (and relies on the real output we are looking
for never begins with '+').

I do agree the change from 1d to /<expected string>/d in this patch
is a very good thing; it makes it clear that we are excluding the
"error: ", and expect that after removing the message what follows
is the help text.  And in the spirit of that change, I think it is
better to match /^error: / instead of /option .exec. requires.../.

> So let's change the offending tests so that they are a bit less
> stringent and do not stumble over the "+..." lines generated by the -x
> flag.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  t/t3404-rebase-interactive.sh | 67 ++++++++++---------------------------------
>  1 file changed, 15 insertions(+), 52 deletions(-)
>
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 66348f1..25f1118 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -882,7 +882,8 @@ test_expect_success 'rebase -i --exec without <CMD>' '
>  	git reset --hard execute &&
>  	set_fake_editor &&
>  	test_must_fail git rebase -i --exec 2>tmp &&
> -	sed -e "1d" tmp >actual &&
> +	sed -e "/option .exec. requires a value/d" -e '/^+/d' \
> +		tmp >actual &&
>  	test_must_fail git rebase -h >expected &&
>  	test_cmp expected actual &&
>  	git checkout master
> @@ -1149,10 +1150,6 @@ test_expect_success 'drop' '
>  	test A = $(git cat-file commit HEAD^^ | sed -ne \$p)
>  '
>  
> -cat >expect <<EOF
> -Successfully rebased and updated refs/heads/missing-commit.
> -EOF
> -
>  test_expect_success 'rebase -i respects rebase.missingCommitsCheck = ignore' '
>  	test_config rebase.missingCommitsCheck ignore &&
>  	rebase_setup_and_clean missing-commit &&
> @@ -1160,52 +1157,33 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = ignore' '
>  	FAKE_LINES="1 2 3 4" \
>  		git rebase -i --root 2>actual &&
>  	test D = $(git cat-file commit HEAD | sed -ne \$p) &&
> -	test_cmp expect actual
> +	test_i18ngrep \
> +		"Successfully rebased and updated refs/heads/missing-commit." \
> +		actual

Is this string going to be i18n'ed?  If so this change is good, but
it probably wants to be a separate "prepare for i18n" patch, not
this "work around -x that pollutes 2>actual output" patch.

>  test_expect_success 'rebase -i respects rebase.missingCommitsCheck = warn' '
> +	line="$(git rev-list --pretty=oneline --abbrev-commit -1 master)" &&
>  	test_config rebase.missingCommitsCheck warn &&
>  	rebase_setup_and_clean missing-commit &&
>  	set_fake_editor &&
>  	FAKE_LINES="1 2 3 4" \
>  		git rebase -i --root 2>actual &&
> -	test_cmp expect actual &&
> +	test_i18ngrep "Warning: some commits may have been dropped" actual &&
> +	test_i18ngrep "^ - $line" actual &&

The former is good in "prepare for i18n" patch.  The latter is not.

test_i18ngrep is primarily to make sure what is *not* supposed to be
localized are not localized.  GETTEXT_POISON build-time option
builds Git with garbage translations for strings marked for
localization, and test_i18ngrep knows to pretend the test always
passes in POISON build.

We test things that are _not_ to be localized with "grep", so a test
with POISON build will catch if a string (like plumbing output) that
are not supposed to be localized is marked for localization by
mistake.

I stop quoting here, but I think the remainder has the same
over-eager use of i18ngrep.