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