17/1f59c2840e9f546b618f59c6fa305ecf2b99a5

   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 A6654429E21\r
   6         for <notmuch@notmuchmail.org>; Mon, 28 Nov 2011 14:42:57 -0800 (PST)\r
   7 X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
   8 X-Spam-Flag: NO\r
   9 X-Spam-Score: 1.7\r
  10 X-Spam-Level: *\r
  11 X-Spam-Status: No, score=1.7 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, FREEMAIL_REPLY=2.499, RCVD_IN_DNSWL_LOW=-0.7]\r
  14         autolearn=disabled\r
  15 Received: from olra.theworths.org ([127.0.0.1])\r
  16         by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
  17         with ESMTP id oJTzhxZxSOyy for <notmuch@notmuchmail.org>;\r
  18         Mon, 28 Nov 2011 14:42:57 -0800 (PST)\r
  19 Received: from mail-bw0-f53.google.com (mail-bw0-f53.google.com\r
  20         [209.85.214.53]) (using TLSv1 with cipher RC4-SHA (128/128 bits))\r
  21         (No client certificate requested)\r
  22         by olra.theworths.org (Postfix) with ESMTPS id 98DC9431FB6\r
  23         for <notmuch@notmuchmail.org>; Mon, 28 Nov 2011 14:42:56 -0800 (PST)\r
  24 Received: by bkaq10 with SMTP id q10so9987266bka.26\r
  25         for <notmuch@notmuchmail.org>; Mon, 28 Nov 2011 14:42:55 -0800 (PST)\r
  26 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma;\r
  27         h=from:to:subject:in-reply-to:references:user-agent:date:message-id\r
  28         :mime-version:content-type;\r
  29         bh=iyOUSr4AXX+vETn5Nz5EJctfjQsEMsQE8gXbIbH26GM=;\r
  30         b=b9AoK7+3wRrLEs2GRFWKaeDh1z54olGAbU6k27Ik/1AQV4JuUnM41ReowFgW5BjhYT\r
  31         8rmGNsRHI8zmDPt1ldbVt9leczdF8VHosFvagyiLowhRaTvpD8/x3V6y5bQbItfKJ9sg\r
  32         nEi3VFHSdwUlcbhF2Du3xo1q6QLeGLMdGJiII=\r
  33 Received: by 10.204.13.78 with SMTP id b14mr33549417bka.23.1322520175148;\r
  34         Mon, 28 Nov 2011 14:42:55 -0800 (PST)\r
  35 Received: from localhost ([91.144.186.21])\r
  36         by mx.google.com with ESMTPS id q6sm32791231bka.6.2011.11.28.14.42.53\r
  37         (version=TLSv1/SSLv3 cipher=OTHER);\r
  38         Mon, 28 Nov 2011 14:42:54 -0800 (PST)\r
  39 From: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>\r
  40 To: Tomi Ollila <tomi.ollila@iki.fi>, notmuch@notmuchmail.org\r
  41 Subject: Re: [PATCH 4/9] test: add support for external executable\r
  42  dependencies\r
  43 In-Reply-To: <87aa7fx5cu.fsf@gmail.com>\r
  44 References: <1321494986-18998-1-git-send-email-dmitry.kurochkin@gmail.com>\r
  45         <1321494986-18998-5-git-send-email-dmitry.kurochkin@gmail.com>\r
  46         <yf662i4j6c4.fsf@taco2.nixu.fi> <87d3cbx6aa.fsf@gmail.com>\r
  47         <87aa7fx5cu.fsf@gmail.com>\r
  48 User-Agent: Notmuch/0.10+44~g067c44f (http://notmuchmail.org) Emacs/23.3.1\r
  49         (x86_64-pc-linux-gnu)\r
  50 Date: Tue, 29 Nov 2011 02:42:31 +0400\r
  51 Message-ID: <877h2jx414.fsf@gmail.com>\r
  52 MIME-Version: 1.0\r
  53 Content-Type: text/plain; charset=us-ascii\r
  54 X-BeenThere: notmuch@notmuchmail.org\r
  55 X-Mailman-Version: 2.1.13\r
  56 Precedence: list\r
  57 List-Id: "Use and development of the notmuch mail system."\r
  58         <notmuch.notmuchmail.org>\r
  59 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  60         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  61 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  62 List-Post: <mailto:notmuch@notmuchmail.org>\r
  63 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  64 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  65         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  66 X-List-Received-Date: Mon, 28 Nov 2011 22:42:57 -0000\r
  67 \r
  68 On Tue, 29 Nov 2011 02:13:53 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:\r
  69 > On Tue, 29 Nov 2011 01:53:49 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:\r
  70 > > On Mon, 28 Nov 2011 23:16:27 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:\r
  71 > > > On Thu, 17 Nov 2011 05:56:21 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:\r
  72 > > > > There is existing support for general prerequisites in the test suite.\r
  73 > > > > But it is not very convenient to use: every test case has to keep\r
  74 > > > > track for it's dependencies and they have to be explicitly listed.\r
  75 > > > > \r
  76 > > > > The patch aims to add better support for a particular type of external\r
  77 > > > > dependencies: external executables.  The main idea is to replace\r
  78 > > > > missing external binaries with shell functions that have the same\r
  79 > > > > name.  These functions always fail and keep track of missing\r
  80 > > > > dependencies for a subtest.  The result reporting functions later can\r
  81 > > > > check that an external binaries are missing and correctly report SKIP\r
  82 > > > > result instead of FAIL.  The primary benefit is that the test cases do\r
  83 > > > > not need to declare their dependencies or be changed in any way.\r
  84 > > > > ---\r
  85 > > > >  test/test-lib.sh |   49 +++++++++++++++++++++++++++++++++++++++++--------\r
  86 > > > >  1 files changed, 41 insertions(+), 8 deletions(-)\r
  87 > > > > \r
  88 > > > > diff --git a/test/test-lib.sh b/test/test-lib.sh\r
  89 > > > > index f21e45e..ab8c6fd 100755\r
  90 > > > > --- a/test/test-lib.sh\r
  91 > > > > +++ b/test/test-lib.sh\r
  92 > > > > @@ -526,40 +526,53 @@ notmuch_json_show_sanitize ()\r
  93 > > > >  # - Implicitly by specifying the prerequisite tag in the calls to\r
  94 > > > >  #   test_expect_{success,failure,code}.\r
  95 > > > >  #\r
  96 > > > >  # The single parameter is the prerequisite tag (a simple word, in all\r
  97 > > > >  # capital letters by convention).\r
  98 > > > >  \r
  99 > > > >  test_set_prereq () {\r
 100 > > > >         satisfied="$satisfied$1 "\r
 101 > > > >  }\r
 102 > > > >  satisfied=" "\r
 103 > > > >  \r
 104 > > > >  test_have_prereq () {\r
 105 > > > >         case $satisfied in\r
 106 > > > >         *" $1 "*)\r
 107 > > > >                 : yes, have it ;;\r
 108 > > > >         *)\r
 109 > > > >                 ! : nope ;;\r
 110 > > > >         esac\r
 111 > > > >  }\r
 112 > > > >  \r
 113 > > > > +# declare prerequisite for the given external binary\r
 114 > > > > +test_declare_external_prereq () {\r
 115 > > > > +       binary="$1"\r
 116 > > > > +       test "$#" = 2 && name=$2 || name="$binary(1)"\r
 117 > > > > +\r
 118 > > > > +        hash $binary 2>/dev/null || eval "\r
 119 > > > > +$1 () {\r
 120 > > > > +       echo -n \"\$test_subtest_missing_external_prereqs_\" | grep -e \" $name \" ||\r
 121 > > > > +       test_subtest_missing_external_prereqs_=\"$test_subtest_missing_external_prereqs_ $name\"\r
 122 > > > > +       false\r
 123 > > > > +}"\r
 124 > > > > +}\r
 125 > > > > +\r
 126 > > > \r
 127 > > > Does this work right ?\r
 128 > > \r
 129 > > It does not.\r
 130 > > \r
 131 > > Moreover, there is a missing backslash before\r
 132 > > $test_subtest_missing_external_prereqs_ (and that is why I did not\r
 133 > > notice the bug during my poor testing).\r
 134 > > \r
 135 > > > ... the grep -e \" $name \"   -- part requires\r
 136 > > > spaces on both side of, but next line does not write trailing space...\r
 137 > > > ... and is there leading space (and also the latest\r
 138 > > > $test_subtest_missing_external_prereqs_ (just before 'false') is evaluated) ?\r
 139 > > > \r
 140 > > > This could perhaps be written like the above test_set_prereq &\r
 141 > > > test_save_prereq:\r
 142 > > > ...\r
 143 > > >         hash $binary 2>/dev/null || eval "\r
 144 > > > $binary () {\r
 145 > > >   test_missing_external_prereq_${binary}_=t\r
 146 > > >   case \$test_subtest_missing_external_prereqs_ in\r
 147 > > >           *' $name '*) ;;\r
 148 > > >                 *) test_subtest_missing_external_prereqs_=\"\$test_subtest_missing_external_prereqs_$name \"\r
 149 > > >         esac\r
 150 > > >   false\r
 151 > > > }\r
 152 > > > \r
 153 > > > and  test_subtest_missing_external_prereqs_=' '  done in test_reset_state_\r
 154 > > > \r
 155 > > \r
 156 > > Well, this approach would obviously be better, at least because it does\r
 157 > > work.  But IMO it is too complex, and essentially has the same problem\r
 158 > > as the current code: it mixes the check with diagnostic message.\r
 159 > > \r
 160 > > We already have a proper way to check if dependency is missing already:\r
 161 > > test_missing_external_prereq_${binary}_.  We should check it and add the\r
 162 > > binary to test_subtest_missing_external_prereqs_ if it is not set.  And\r
 163 > > move setting of test_missing_external_prereq_${binary}_ below.\r
 164 > > \r
 165 > > > (I took some code from current git head.... also perhaps instead of \r
 166 > > > setting test_missing_external_prereq_${binary}_=t, (bash) associative\r
 167 > > > arrays could be taken into use -- the eval to read that information\r
 168 > > > is a bit hairy -- well, at least that part works for sure :D )\r
 169 > > > \r
 170 > > \r
 171 > > Yes!  I like using hash here.\r
 172 > > \r
 173 > > > Hmm... how about this:\r
 174 > > > \r
 175 > > >         hash $binary 2>/dev/null || eval "\r
 176 > > > $binary () {\r
 177 > > >         if [ -z \"\${test_missing_external_prereq_[$binary]}\" ]\r
 178 > > >         then\r
 179 > > >           test_missing_external_prereq_[$binary]=t\r
 180 > > >                 test_subtest_missing_external_prereqs_=\"\$test_subtest_missing_external_prereqs_ $name\"\r
 181 > > >         fi\r
 182 > > >   false\r
 183 > > > }\r
 184 > > > \r
 185 > > \r
 186 > > There is some inconsistent indenting in the above code (mixed tabs and\r
 187 > > spaces).  Also test_require_external_prereq() should be changed as well.\r
 188 > > \r
 189 > > Otherwise I like it.  Would you please submit a patch?\r
 190 > > \r
 191 > \r
 192 > Actually, test_missing_external_prereq_${binary}_ must be set outside of\r
 193 > the $binary() function.  It indicates that the binary is missing, not\r
 194 > that it is required.  I will send some patches to fix these bugs.  Then\r
 195 > you can change it to use hashes.\r
 196 \r
 197 See id:1322520067-15227-1-git-send-email-dmitry.kurochkin@gmail.com.\r
 198 \r
 199 Regards,\r
 200   Dmitry\r
 201 \r
 202 > \r
 203 > Regards,\r
 204 >   Dmitry\r
 205 > \r
 206 > > Regards,\r
 207 > >   Dmitry\r
 208 > > \r
 209 > > > and test_subtest_missing_external_prereqs_ cleared in test_reset_state_ like now.\r
 210 > > > \r
 211 > > > Tomi\r