From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by olra.theworths.org (Postfix) with ESMTP id 17DD4429E21 for ; Sun, 11 Sep 2011 17:26:56 -0700 (PDT) X-Virus-Scanned: Debian amavisd-new at olra.theworths.org X-Spam-Flag: NO X-Spam-Score: -0.799 X-Spam-Level: X-Spam-Status: No, score=-0.799 tagged_above=-999 required=5 tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-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 qKTb1DiMqJ4a for ; Sun, 11 Sep 2011 17:26:55 -0700 (PDT) Received: from mail-bw0-f53.google.com (mail-bw0-f53.google.com [209.85.214.53]) (using TLSv1 with cipher RC4-SHA (128/128 bits)) (No client certificate requested) by olra.theworths.org (Postfix) with ESMTPS id 5174D431FB6 for ; Sun, 11 Sep 2011 17:26:55 -0700 (PDT) Received: by bkbzt12 with SMTP id zt12so1081882bkb.26 for ; Sun, 11 Sep 2011 17:26:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=from:to:subject:in-reply-to:references:user-agent:date:message-id :mime-version:content-type; bh=kyhTjZln3GjYOEfEGA2GhPhnSkcOu7M9nr3HhfNcT60=; b=HxCbVZFRcYsjlDGky+pdC2Nk1VT0/58SX4n/4FBc7CP+A3BHzjnrVj85lAfxrcMquj N3nu/Ox1OO2sU2LYZ4lp3nL8iXEdXehIHMlQTH2HuUBVgvQkDqUNwYVjUib19Nhhdzyb nlbTqkul9VF/DDWYVckXoohfWK+1m/C4kXc7k= Received: by 10.204.5.137 with SMTP id 9mr647169bkv.55.1315787213901; Sun, 11 Sep 2011 17:26:53 -0700 (PDT) Received: from localhost ([91.144.186.21]) by mx.google.com with ESMTPS id b17sm4998212bkd.8.2011.09.11.17.26.52 (version=TLSv1/SSLv3 cipher=OTHER); Sun, 11 Sep 2011 17:26:53 -0700 (PDT) From: Dmitry Kurochkin To: David Bremner , 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 X-BeenThere: notmuch@notmuchmail.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: "Use and development of the notmuch mail system." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 12 Sep 2011 00:26:56 -0000 On Sun, 11 Sep 2011 20:51:47 -0300, David Bremner wrote: > On Mon, 12 Sep 2011 03:30:54 +0400, Dmitry Kurochkin 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