From: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>
To: David Bremner <david@tethera.net>, notmuch@notmuchmail.org
Subject: Re: [PATCH] test: reset known_broken status in test_expect_equal and
	test_expect_equal_file
In-Reply-To: <877h5ed4do.fsf@zancas.localnet>
References: <1309752441-10651-3-git-send-email-dmitry.kurochkin@gmail.com>
	<1315782714-32287-1-git-send-email-david@tethera.net>
	<87r53mveq9.fsf@gmail.com> <877h5ed4do.fsf@zancas.localnet>
User-Agent: Notmuch/0.8-19-g1ca96a5 (http://notmuchmail.org) Emacs/23.3.1
	(x86_64-pc-linux-gnu)
Date: Mon, 12 Sep 2011 04:26:59 +0400
Message-ID: <87obyqvc4s.fsf@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

On Sun, 11 Sep 2011 20:51:47 -0300, David Bremner <david@tethera.net> wrote:
> On Mon, 12 Sep 2011 03:30:54 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > Hi David.
> > IMHO this is not a good idea, because:
> > 
> > 1. It introduces multiple places where the flag is reset.  If new
> >    test_expect_* functions are added in the future, there would be more
> >    of these.  So it brings us more complex code, code duplication, more
> >    chances for bugs, etc.
> 
> Well, I'm not sure how to solve this without code duplication, since
> there is no test_end_subtest. We could make one, but that seems pretty
> intrusive.
> 

I agree that introducing test_end_subtest for the flag reset is an
overkill.

> > 2. No support for tests with multiple test_expect_* calls.  I do not
> >    know if it is used or works now, but the patch certainly does not
> 
> Looking at the code for test_expect_equal_* (note that these two are
> very different than test_expect_success and test_expect_failure), 
> several things are reset already, so I don't think it will work even
> before my patch to call those routines twice.
> 
> > 3. I thought that every test must start with a test_begin_subtest call.
> >    So it is the right place to put all subtest initialization code to.
> >    Is this not correct?  If it is correct, then I do not understand why
> >    we should support buggy tests by hiding (some of) their bugs.  Why do
> >    we need it?
> 
> I could not find anything in the docs (or test-lib.sh) that says
> test_begin_subtest must be called before test_expect_success or
> test_expect_failure. Indeed if test_begin_subtest is called first, the
> extra message argument to those two does not really make sense.
> 
> What brought this to my attention is the atomicity test introduced by
> amdragon, which does exactly
> 
> test_begin_subtest
> test_expect_equal_failure
> test_expect_success
> 

So it seems we have 2 types of tests.  Those who start with
test_begin_subtest() and end with test_expect_equal_*() call.  And those
who are implemented as a single test_expect_[success|code]() call
(test-lib.sh mentions test_expect_failure(), but apparently it is not
available).  I see several options, starting with the simplest:

1. Support broken tests only for the first type of tests.
   Test_begin_subtest() sets inside_subtest variable.  We can check for
   broken tests only when it is set (currently it is cleared to early,
   but that is easy to fix).

2. Single-command tests use test_run_() function internally to run the
   command.  We can reset broken flag in the beginning of it.  That way
   test_subtest_known_broken() should work fine for both types of tests.

3. Remove the single-command tests, and just stick with
   test_begin_subtest() and friends.

The last one may be invasive and perhaps others have some good reasons
against it.  But I like it the most because it would make the test
system simpler and more consistent.

Point 2 should be pretty easy to implement (1 line if I do not miss
anything) and it should work fine.  So I would go for it.


Hmm... looks like there is one more way to run tests - test_external()
function.  It does not use test_run_(), so it another place where we
need to reset the flag.  Instead of resetting the flag in 3 different
places, we should have a function like test_init_subtest_() which does
all subtest-related initialization.

Regards,
  Dmitry

> d