MIME-Version: 1.0
Sender: amdragon@gmail.com
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
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
Precedence: list

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