[PATCH] test: reset known_broken status in test_expect_equal and test_expect_equal_file
Dmitry Kurochkin
dmitry.kurochkin at gmail.com
Sun Sep 11 17:26:59 PDT 2011
On Sun, 11 Sep 2011 20:51:47 -0300, David Bremner <david at tethera.net> wrote:
> On Mon, 12 Sep 2011 03:30:54 +0400, Dmitry Kurochkin <dmitry.kurochkin at 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
More information about the notmuch
mailing list