From mboxrd@z Thu Jan 1 00:00:00 1970 From: Junio C Hamano Subject: Re: [PATCH 2/2] t3404: be resilient against running with the -x flag Date: Tue, 10 May 2016 12:49:42 -0700 Message-ID: References: Mime-Version: 1.0 Content-Type: text/plain Cc: git@vger.kernel.org To: Johannes Schindelin X-From: git-owner@vger.kernel.org Tue May 10 21:49:55 2016 Return-path: Envelope-to: gcvg-git-2@plane.gmane.org Received: from vger.kernel.org ([209.132.180.67]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1b0DfE-0004to-UF for gcvg-git-2@plane.gmane.org; Tue, 10 May 2016 21:49:53 +0200 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752708AbcEJTtr (ORCPT ); Tue, 10 May 2016 15:49:47 -0400 Received: from pb-smtp2.pobox.com ([64.147.108.71]:54216 "EHLO sasl.smtp.pobox.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751759AbcEJTtq (ORCPT ); Tue, 10 May 2016 15:49:46 -0400 Received: from sasl.smtp.pobox.com (unknown [127.0.0.1]) by pb-smtp2.pobox.com (Postfix) with ESMTP id 278D4192C8; Tue, 10 May 2016 15:49:45 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=pobox.com; h=from:to:cc :subject:references:date:in-reply-to:message-id:mime-version :content-type; s=sasl; bh=WXWbaV7j91t7RwQbqqHN4AQxMHc=; b=XMdDk5 Kn7U+RIyZHccWF6o8/7DG/G2Tr9JOItTNrbFk06I1MY+yvuSbc48vTteJeANmufb P+Hwo5Au5/azeRHPtp8AUgAE0WK/LP2lj60yXjIPq5CD4dVzBJi2isksik3/vlmF IklAXdGoPEnAQmo61ZoSfiUnBbSJ+5FE4vgqY= DomainKey-Signature: a=rsa-sha1; c=nofws; d=pobox.com; h=from:to:cc :subject:references:date:in-reply-to:message-id:mime-version :content-type; q=dns; s=sasl; b=eAb+07MGZsFqbkPexwlyKAgD58tGvR+4 LicIsHISx8TOUXLPJ07VILYF5JESzYo89cRLpVMLJUjlVEw96T+X5JBTZphsVeCu Gra9ekR4UE5Rg8YGp9lRPeFmyyvLjh7jb1Tk8WpyGgFS+a0cRfQ8n/wlgD1NTPkj 4zP9J4LW0jg= Received: from pb-smtp2.nyi.icgroup.com (unknown [127.0.0.1]) by pb-smtp2.pobox.com (Postfix) with ESMTP id 1F93C192C7; Tue, 10 May 2016 15:49:45 -0400 (EDT) Received: from pobox.com (unknown [104.132.0.95]) (using TLSv1.2 with cipher DHE-RSA-AES128-SHA (128/128 bits)) (No client certificate requested) by pb-smtp2.pobox.com (Postfix) with ESMTPSA id 7CA16192C6; Tue, 10 May 2016 15:49:44 -0400 (EDT) In-Reply-To: (Johannes Schindelin's message of "Tue, 10 May 2016 16:07:22 +0200 (CEST)") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) X-Pobox-Relay-ID: 57F409AA-16E8-11E6-90F4-D05A70183E34-77302942!pb-smtp2.pobox.com Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Archived-At: Johannes Schindelin writes: > To: Junio C Hamano > 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 //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 > --- > 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 ' ' > 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 < -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.