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

From: Junio C Hamano <gitster@pobox.com>
Date: May 10, 2016, 19:49 UTC
Message-ID: <xmqqshxpofqh.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <alpine.DEB.2.20.1605101607180.4092@virtualbox>

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.../.

Show 42 quoted lines

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

Show 10 quoted lines

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