60/2c0efb195682e5289471a2df8c836dc9fa9028

   1 Return-Path: <dmitry.kurochkin@gmail.com>\r
   2 X-Original-To: notmuch@notmuchmail.org\r
   3 Delivered-To: notmuch@notmuchmail.org\r
   4 Received: from localhost (localhost [127.0.0.1])\r
   5         by olra.theworths.org (Postfix) with ESMTP id 17DD4429E21\r
   6         for <notmuch@notmuchmail.org>; Sun, 11 Sep 2011 17:26:56 -0700 (PDT)\r
   7 X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
   8 X-Spam-Flag: NO\r
   9 X-Spam-Score: -0.799\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-0.799 tagged_above=-999 required=5\r
  12         tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1,\r
  13         FREEMAIL_FROM=0.001, RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled\r
  14 Received: from olra.theworths.org ([127.0.0.1])\r
  15         by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
  16         with ESMTP id qKTb1DiMqJ4a for <notmuch@notmuchmail.org>;\r
  17         Sun, 11 Sep 2011 17:26:55 -0700 (PDT)\r
  18 Received: from mail-bw0-f53.google.com (mail-bw0-f53.google.com\r
  19         [209.85.214.53]) (using TLSv1 with cipher RC4-SHA (128/128 bits))\r
  20         (No client certificate requested)\r
  21         by olra.theworths.org (Postfix) with ESMTPS id 5174D431FB6\r
  22         for <notmuch@notmuchmail.org>; Sun, 11 Sep 2011 17:26:55 -0700 (PDT)\r
  23 Received: by bkbzt12 with SMTP id zt12so1081882bkb.26\r
  24         for <notmuch@notmuchmail.org>; Sun, 11 Sep 2011 17:26:54 -0700 (PDT)\r
  25 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma;\r
  26         h=from:to:subject:in-reply-to:references:user-agent:date:message-id\r
  27         :mime-version:content-type;\r
  28         bh=kyhTjZln3GjYOEfEGA2GhPhnSkcOu7M9nr3HhfNcT60=;\r
  29         b=HxCbVZFRcYsjlDGky+pdC2Nk1VT0/58SX4n/4FBc7CP+A3BHzjnrVj85lAfxrcMquj\r
  30         N3nu/Ox1OO2sU2LYZ4lp3nL8iXEdXehIHMlQTH2HuUBVgvQkDqUNwYVjUib19Nhhdzyb\r
  31         nlbTqkul9VF/DDWYVckXoohfWK+1m/C4kXc7k=\r
  32 Received: by 10.204.5.137 with SMTP id 9mr647169bkv.55.1315787213901;\r
  33         Sun, 11 Sep 2011 17:26:53 -0700 (PDT)\r
  34 Received: from localhost ([91.144.186.21])\r
  35         by mx.google.com with ESMTPS id b17sm4998212bkd.8.2011.09.11.17.26.52\r
  36         (version=TLSv1/SSLv3 cipher=OTHER);\r
  37         Sun, 11 Sep 2011 17:26:53 -0700 (PDT)\r
  38 From: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>\r
  39 To: David Bremner <david@tethera.net>, notmuch@notmuchmail.org\r
  40 Subject: Re: [PATCH] test: reset known_broken status in test_expect_equal and\r
  41         test_expect_equal_file\r
  42 In-Reply-To: <877h5ed4do.fsf@zancas.localnet>\r
  43 References: <1309752441-10651-3-git-send-email-dmitry.kurochkin@gmail.com>\r
  44         <1315782714-32287-1-git-send-email-david@tethera.net>\r
  45         <87r53mveq9.fsf@gmail.com> <877h5ed4do.fsf@zancas.localnet>\r
  46 User-Agent: Notmuch/0.8-19-g1ca96a5 (http://notmuchmail.org) Emacs/23.3.1\r
  47         (x86_64-pc-linux-gnu)\r
  48 Date: Mon, 12 Sep 2011 04:26:59 +0400\r
  49 Message-ID: <87obyqvc4s.fsf@gmail.com>\r
  50 MIME-Version: 1.0\r
  51 Content-Type: text/plain; charset=us-ascii\r
  52 X-BeenThere: notmuch@notmuchmail.org\r
  53 X-Mailman-Version: 2.1.13\r
  54 Precedence: list\r
  55 List-Id: "Use and development of the notmuch mail system."\r
  56         <notmuch.notmuchmail.org>\r
  57 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  58         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  59 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  60 List-Post: <mailto:notmuch@notmuchmail.org>\r
  61 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  62 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  63         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  64 X-List-Received-Date: Mon, 12 Sep 2011 00:26:56 -0000\r
  65 \r
  66 On Sun, 11 Sep 2011 20:51:47 -0300, David Bremner <david@tethera.net> wrote:\r
  67 > On Mon, 12 Sep 2011 03:30:54 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:\r
  68 > > Hi David.\r
  69 > > IMHO this is not a good idea, because:\r
  70 > > \r
  71 > > 1. It introduces multiple places where the flag is reset.  If new\r
  72 > >    test_expect_* functions are added in the future, there would be more\r
  73 > >    of these.  So it brings us more complex code, code duplication, more\r
  74 > >    chances for bugs, etc.\r
  75 > \r
  76 > Well, I'm not sure how to solve this without code duplication, since\r
  77 > there is no test_end_subtest. We could make one, but that seems pretty\r
  78 > intrusive.\r
  79 > \r
  80 \r
  81 I agree that introducing test_end_subtest for the flag reset is an\r
  82 overkill.\r
  83 \r
  84 > > 2. No support for tests with multiple test_expect_* calls.  I do not\r
  85 > >    know if it is used or works now, but the patch certainly does not\r
  86 > \r
  87 > Looking at the code for test_expect_equal_* (note that these two are\r
  88 > very different than test_expect_success and test_expect_failure), \r
  89 > several things are reset already, so I don't think it will work even\r
  90 > before my patch to call those routines twice.\r
  91 > \r
  92 > > 3. I thought that every test must start with a test_begin_subtest call.\r
  93 > >    So it is the right place to put all subtest initialization code to.\r
  94 > >    Is this not correct?  If it is correct, then I do not understand why\r
  95 > >    we should support buggy tests by hiding (some of) their bugs.  Why do\r
  96 > >    we need it?\r
  97 > \r
  98 > I could not find anything in the docs (or test-lib.sh) that says\r
  99 > test_begin_subtest must be called before test_expect_success or\r
 100 > test_expect_failure. Indeed if test_begin_subtest is called first, the\r
 101 > extra message argument to those two does not really make sense.\r
 102 > \r
 103 > What brought this to my attention is the atomicity test introduced by\r
 104 > amdragon, which does exactly\r
 105 > \r
 106 > test_begin_subtest\r
 107 > test_expect_equal_failure\r
 108 > test_expect_success\r
 109 > \r
 110 \r
 111 So it seems we have 2 types of tests.  Those who start with\r
 112 test_begin_subtest() and end with test_expect_equal_*() call.  And those\r
 113 who are implemented as a single test_expect_[success|code]() call\r
 114 (test-lib.sh mentions test_expect_failure(), but apparently it is not\r
 115 available).  I see several options, starting with the simplest:\r
 116 \r
 117 1. Support broken tests only for the first type of tests.\r
 118    Test_begin_subtest() sets inside_subtest variable.  We can check for\r
 119    broken tests only when it is set (currently it is cleared to early,\r
 120    but that is easy to fix).\r
 121 \r
 122 2. Single-command tests use test_run_() function internally to run the\r
 123    command.  We can reset broken flag in the beginning of it.  That way\r
 124    test_subtest_known_broken() should work fine for both types of tests.\r
 125 \r
 126 3. Remove the single-command tests, and just stick with\r
 127    test_begin_subtest() and friends.\r
 128 \r
 129 The last one may be invasive and perhaps others have some good reasons\r
 130 against it.  But I like it the most because it would make the test\r
 131 system simpler and more consistent.\r
 132 \r
 133 Point 2 should be pretty easy to implement (1 line if I do not miss\r
 134 anything) and it should work fine.  So I would go for it.\r
 135 \r
 136 \r
 137 Hmm... looks like there is one more way to run tests - test_external()\r
 138 function.  It does not use test_run_(), so it another place where we\r
 139 need to reset the flag.  Instead of resetting the flag in 3 different\r
 140 places, we should have a function like test_init_subtest_() which does\r
 141 all subtest-related initialization.\r
 142 \r
 143 Regards,\r
 144   Dmitry\r
 145 \r
 146 > d\r