From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <amdragon@gmail.com>
Received: from localhost (localhost [127.0.0.1])
	by olra.theworths.org (Postfix) with ESMTP id 2845B431FD0
	for <notmuch@notmuchmail.org>; Sun,  3 Jul 2011 20:42:16 -0700 (PDT)
X-Virus-Scanned: Debian amavisd-new at olra.theworths.org
X-Spam-Flag: NO
X-Spam-Score: -0.699
X-Spam-Level: 
X-Spam-Status: No, score=-0.699 tagged_above=-999 required=5
	tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, FREEMAIL_FROM=0.001,
	RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled
Received: from olra.theworths.org ([127.0.0.1])
	by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)
	with ESMTP id WZsA8IDmIQsm for <notmuch@notmuchmail.org>;
	Sun,  3 Jul 2011 20:42:15 -0700 (PDT)
Received: from mail-qy0-f181.google.com (mail-qy0-f181.google.com
	[209.85.216.181]) (using TLSv1 with cipher RC4-SHA (128/128 bits))
	(No client certificate requested)
	by olra.theworths.org (Postfix) with ESMTPS id ECE64431FB6
	for <notmuch@notmuchmail.org>; Sun,  3 Jul 2011 20:42:14 -0700 (PDT)
Received: by qyk9 with SMTP id 9so3224310qyk.5
	for <notmuch@notmuchmail.org>; Sun, 03 Jul 2011 20:42:14 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma;
	h=mime-version:sender:in-reply-to:references:date
	:x-google-sender-auth:message-id:subject:from:to:cc:content-type
	:content-transfer-encoding;
	bh=W6ge2dTNoDe1XlDUS23AIq8QGQnEUaX2+hdKL8Zl4iQ=;
	b=UhhtF7WPli4Bn9jRt1byNTSh5auQYpcyUAS25Y4rF6vvp21EAzTXRk9i/L4xoQ5de0
	f964nXGd8k+xrOLVC3XbtuzcSvsW1zZhLtzsDhaZOKJTprX0CjbzZulT4ovffidEcgme
	Udxcc8YRmynYahb2Nk+5xHCm3HM8vLwoA55S8=
MIME-Version: 1.0
Received: by 10.224.188.146 with SMTP id da18mr4523195qab.124.1309750934019;
	Sun, 03 Jul 2011 20:42:14 -0700 (PDT)
Sender: amdragon@gmail.com
Received: by 10.229.229.211 with HTTP; Sun, 3 Jul 2011 20:42:13 -0700 (PDT)
In-Reply-To: <1309744743-8556-2-git-send-email-dmitry.kurochkin@gmail.com>
References: <1309744743-8556-1-git-send-email-dmitry.kurochkin@gmail.com>
	<1309744743-8556-2-git-send-email-dmitry.kurochkin@gmail.com>
Date: Sun, 3 Jul 2011 23:42:13 -0400
X-Google-Sender-Auth: 7_W2JV2V_d4bwMTBKzqUavpscWQ
Message-ID: <CAH-f9WuKrBpEjBaBet0d95oHXo1kWzGv6oa7zz7Zqe4JK=4N+Q@mail.gmail.com>
Subject: Re: [PATCH 2/3] test: improve known broken tests support
From: Austin Clements <amdragon@mit.edu>
To: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
Cc: notmuch@notmuchmail.org
X-BeenThere: notmuch@notmuchmail.org
X-Mailman-Version: 2.1.13
Precedence: list
List-Id: "Use and development of the notmuch mail system."
	<notmuch.notmuchmail.org>
List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,
	<mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>
List-Archive: <http://notmuchmail.org/pipermail/notmuch>
List-Post: <mailto:notmuch@notmuchmail.org>
List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>
List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,
	<mailto:notmuch-request@notmuchmail.org?subject=subscribe>
X-List-Received-Date: Mon, 04 Jul 2011 03:42:16 -0000

Great idea!

Just a heads-up, this will conflict with the first two patches in the
atomicity series (though git may not detect the second conflict).
Both are trivial patches, though, so the merge should be easy.

Three minor comments below.

On Sun, Jul 3, 2011 at 9:59 PM, Dmitry Kurochkin
<dmitry.kurochkin@gmail.com> wrote:
> There is existing support for broken tests. =A0But it is not convenient
> to use. =A0The primary issue is that we have to maintain a set of
> test_expect_*_failure functions which are equivalent to the normal
> test_expect_* counterparts except for what functions are called for
> result reporting. =A0The patch adds test_subtest_known_broken function
> which marks a subset as broken, making the normal test_expect_*
> functions behave as test_expect_*_failure. =A0All test_expect_*_failure
> functions are removed. =A0Test_known_broken_failure_ is changed to
> format details the same way as test_failure_ does.
>
> Another benefit of this change is that the diff when a broken test is
> fixed would be small and nice.
>
> Documentation is updated accordingly.
> ---
> =A0test/README =A0 =A0 =A0| =A0 17 ++++++++---------
> =A0test/test-lib.sh | =A0 53 +++++++++++++++-----------------------------=
---------
> =A02 files changed, 23 insertions(+), 47 deletions(-)
>
> diff --git a/test/README b/test/README
> index a245bf1..f926b9f 100644
> --- a/test/README
> +++ b/test/README
> @@ -132,20 +132,19 @@ library for your script to use.
> =A0 =A0<script>. =A0If it yields success, test is considered
> =A0 =A0successful. =A0<message> should state what it is testing.
>
> - test_expect_failure <message> <script>
> -
> - =A0 This is NOT the opposite of test_expect_success, but is used
> - =A0 to mark a test that demonstrates a known breakage. =A0Unlike
> - =A0 the usual test_expect_success tests, which say "ok" on
> - =A0 success and "FAIL" on failure, this will say "FIXED" on
> - =A0 success and "still broken" on failure. =A0Failures from these
> - =A0 tests won't cause -i (immediate) to stop.
> -
> =A0test_begin_subtest <message>
>
> =A0 =A0Set the test description message for a subsequent test_expect_equa=
l
> =A0 =A0invocation (see below).
>
> + test_subtest_known_broken
> +
> + =A0 Mark the current test as broken. =A0Such tests are expected to fail=
.
> + =A0 Unlike the normal tests, which say "PASS" on success and "FAIL" on
> + =A0 failure, these will say "FIXED" on success and "BROKEN" on failure.
> + =A0 Failures from these tests won't cause -i (immediate) to stop. =A0Th=
is
> + =A0 must be called before any test_expect_* function.

Perhaps "A test must call this before any test_expect_* function"?
There can be earlier test_expect_* calls from other tests.

> +
> =A0test_expect_equal <output> <expected>
>
> =A0 =A0This is an often-used convenience function built on top of
> diff --git a/test/test-lib.sh b/test/test-lib.sh
> index 22e387e..0cd4170 100755
> --- a/test/test-lib.sh
> +++ b/test/test-lib.sh
> @@ -424,6 +424,7 @@ test_begin_subtest ()
> =A0 =A0 =A0 =A0error "bug in test script: Missing test_expect_equal in ${=
BASH_SOURCE[1]}:${BASH_LINENO[0]}"
> =A0 =A0 fi
> =A0 =A0 test_subtest_name=3D"$1"
> + =A0 =A0test_subtest_known_broken_=3D
> =A0 =A0 # Remember stdout and stderr file descriptors and redirect test
> =A0 =A0 # output to the previously prepared file descriptors 3 and 4 (see
> =A0 =A0 # below)
> @@ -484,29 +485,6 @@ test_expect_equal_file ()
> =A0 =A0 fi
> =A0}
>
> -test_expect_equal_failure ()
> -{
> - =A0 =A0 =A0 exec 1>&6 2>&7 =A0 =A0 =A0 =A0 =A0# Restore stdout and stde=
rr
> - =A0 =A0 =A0 inside_subtest=3D
> - =A0 =A0 =A0 test "$#" =3D 3 && { prereq=3D$1; shift; } || prereq=3D
> - =A0 =A0 =A0 test "$#" =3D 2 ||
> - =A0 =A0 =A0 error "bug in the test script: not 2 or 3 parameters to tes=
t_expect_equal"
> -
> - =A0 =A0 =A0 output=3D"$1"
> - =A0 =A0 =A0 expected=3D"$2"
> - =A0 =A0 =A0 if ! test_skip "$@"
> - =A0 =A0 =A0 then
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if [ "$output" =3D "$expected" ]; then
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 test_known_broken_ok_ "$tes=
t_subtest_name"
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 else
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 test_known_broken_failure_ =
"$test_subtest_name"
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 testname=3D$this_test.$test=
_count
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 echo "$expected" > $testnam=
e.expected
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 echo "$output" > $testname.=
output
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 fi
> - =A0 =A0fi
> -}
> -
> =A0NOTMUCH_NEW ()
> =A0{
> =A0 =A0 notmuch new | grep -v -E -e '^Processed [0-9]*( total)? file|Foun=
d [0-9]* total file'
> @@ -568,12 +546,20 @@ test_have_prereq () {
> =A0# the text_expect_* functions instead.
>
> =A0test_ok_ () {
> + =A0 =A0 =A0 if [ "$test_subtest_known_broken_" =3D 1 ]; then
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 test_known_broken_ok_ "$@"
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return
> + =A0 =A0 =A0 fi
> =A0 =A0 =A0 =A0test_success=3D$(($test_success + 1))
> =A0 =A0 =A0 =A0say_color pass "%-6s" "PASS"
> =A0 =A0 =A0 =A0echo " $@"
> =A0}
>
> =A0test_failure_ () {
> + =A0 =A0 =A0 if [ "$test_subtest_known_broken_" =3D 1 ]; then
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 test_known_broken_failure_ "$@"
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return
> + =A0 =A0 =A0 fi
> =A0 =A0 =A0 =A0test_failure=3D$(($test_failure + 1))
> =A0 =A0 =A0 =A0say_color error "%-6s" "FAIL"
> =A0 =A0 =A0 =A0echo " $1"
> @@ -592,7 +578,10 @@ test_known_broken_ok_ () {
> =A0test_known_broken_failure_ () {
> =A0 =A0 =A0 =A0test_broken=3D$(($test_broken+1))
> =A0 =A0 =A0 =A0say_color pass "%-6s" "BROKEN"
> - =A0 =A0 =A0 echo " $@"
> + =A0 =A0 =A0 echo " $1"
> + =A0 =A0 =A0 shift
> + =A0 =A0 =A0 echo "$@" | sed -e 's/^/ =A0 =A0 =A0 =A0/'
> + =A0 =A0 =A0 if test "$verbose" !=3D "t"; then cat test.output; fi

Would it be possible to keep just the one copy of the failure printing
code in test_failure_?

> =A0}
>
> =A0test_debug () {
> @@ -636,20 +625,8 @@ test_skip () {
> =A0 =A0 =A0 =A0esac
> =A0}
>
> -test_expect_failure () {
> - =A0 =A0 =A0 test "$#" =3D 3 && { prereq=3D$1; shift; } || prereq=3D
> - =A0 =A0 =A0 test "$#" =3D 2 ||
> - =A0 =A0 =A0 error "bug in the test script: not 2 or 3 parameters to tes=
t-expect-failure"
> - =A0 =A0 =A0 if ! test_skip "$@"
> - =A0 =A0 =A0 then
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 test_run_ "$2"
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if [ "$?" =3D 0 -a "$eval_ret" =3D 0 ]
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 then
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 test_known_broken_ok_ "$1"
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 else
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 test_known_broken_failure_ =
"$1"
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 fi
> - =A0 =A0 =A0 fi
> +test_subtest_known_broken () {
> + =A0 =A0 =A0 test_subtest_known_broken_=3D1

The convention in test-lib appears to be "t" for true variables.

> =A0}
>
> =A0test_expect_success () {
> --
> 1.7.5.4