c5/fa411989004d006d1a8b1d2707672fa5d082b6

   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 8C448429E21\r
   6         for <notmuch@notmuchmail.org>; Mon, 28 Nov 2011 13:54:15 -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: -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 ek+cGgnuEdiW for <notmuch@notmuchmail.org>;\r
  17         Mon, 28 Nov 2011 13:54:14 -0800 (PST)\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 99DE2431FB6\r
  22         for <notmuch@notmuchmail.org>; Mon, 28 Nov 2011 13:54:14 -0800 (PST)\r
  23 Received: by bkaq10 with SMTP id q10so9930033bka.26\r
  24         for <notmuch@notmuchmail.org>; Mon, 28 Nov 2011 13:54:13 -0800 (PST)\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=yQDEBx21D/YiLu2qkeyhTaFe9/2lTSPDwKtHjoQ9dDc=;\r
  29         b=hEGMHO6cBqoAvAnWmP5nwtdjLJ3KZtUj58XJpo6PqgbCfEWQgVXLYy9NUQZBwSb8ML\r
  30         VWi/ZYnYGokA7H+su3FYZOarahHWxn/kLDjrZTX+QMEzNrxSgqdy3ryRgyeV1I5l49xQ\r
  31         zncQkpqGkUbkZQzInGSvEWajlomAx1XwSvhGY=\r
  32 Received: by 10.204.156.6 with SMTP id u6mr46164647bkw.135.1322517253285;\r
  33         Mon, 28 Nov 2011 13:54:13 -0800 (PST)\r
  34 Received: from localhost ([91.144.186.21])\r
  35         by mx.google.com with ESMTPS id e18sm32526777bkr.15.2011.11.28.13.54.11\r
  36         (version=TLSv1/SSLv3 cipher=OTHER);\r
  37         Mon, 28 Nov 2011 13:54:12 -0800 (PST)\r
  38 From: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>\r
  39 To: Tomi Ollila <tomi.ollila@iki.fi>, notmuch@notmuchmail.org\r
  40 Subject: Re: [PATCH 4/9] test: add support for external executable\r
  41  dependencies\r
  42 In-Reply-To: <yf662i4j6c4.fsf@taco2.nixu.fi>\r
  43 References: <1321494986-18998-1-git-send-email-dmitry.kurochkin@gmail.com>\r
  44         <1321494986-18998-5-git-send-email-dmitry.kurochkin@gmail.com>\r
  45         <yf662i4j6c4.fsf@taco2.nixu.fi>\r
  46 User-Agent: Notmuch/0.10+44~g067c44f (http://notmuchmail.org) Emacs/23.3.1\r
  47         (x86_64-pc-linux-gnu)\r
  48 Date: Tue, 29 Nov 2011 01:53:49 +0400\r
  49 Message-ID: <87d3cbx6aa.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, 28 Nov 2011 21:54:15 -0000\r
  65 \r
  66 On Mon, 28 Nov 2011 23:16:27 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:\r
  67 > On Thu, 17 Nov 2011 05:56:21 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:\r
  68 > > There is existing support for general prerequisites in the test suite.\r
  69 > > But it is not very convenient to use: every test case has to keep\r
  70 > > track for it's dependencies and they have to be explicitly listed.\r
  71 > > \r
  72 > > The patch aims to add better support for a particular type of external\r
  73 > > dependencies: external executables.  The main idea is to replace\r
  74 > > missing external binaries with shell functions that have the same\r
  75 > > name.  These functions always fail and keep track of missing\r
  76 > > dependencies for a subtest.  The result reporting functions later can\r
  77 > > check that an external binaries are missing and correctly report SKIP\r
  78 > > result instead of FAIL.  The primary benefit is that the test cases do\r
  79 > > not need to declare their dependencies or be changed in any way.\r
  80 > > ---\r
  81 > >  test/test-lib.sh |   49 +++++++++++++++++++++++++++++++++++++++++--------\r
  82 > >  1 files changed, 41 insertions(+), 8 deletions(-)\r
  83 > > \r
  84 > > diff --git a/test/test-lib.sh b/test/test-lib.sh\r
  85 > > index f21e45e..ab8c6fd 100755\r
  86 > > --- a/test/test-lib.sh\r
  87 > > +++ b/test/test-lib.sh\r
  88 > > @@ -526,40 +526,53 @@ notmuch_json_show_sanitize ()\r
  89 > >  # - Implicitly by specifying the prerequisite tag in the calls to\r
  90 > >  #   test_expect_{success,failure,code}.\r
  91 > >  #\r
  92 > >  # The single parameter is the prerequisite tag (a simple word, in all\r
  93 > >  # capital letters by convention).\r
  94 > >  \r
  95 > >  test_set_prereq () {\r
  96 > >     satisfied="$satisfied$1 "\r
  97 > >  }\r
  98 > >  satisfied=" "\r
  99 > >  \r
 100 > >  test_have_prereq () {\r
 101 > >     case $satisfied in\r
 102 > >     *" $1 "*)\r
 103 > >             : yes, have it ;;\r
 104 > >     *)\r
 105 > >             ! : nope ;;\r
 106 > >     esac\r
 107 > >  }\r
 108 > >  \r
 109 > > +# declare prerequisite for the given external binary\r
 110 > > +test_declare_external_prereq () {\r
 111 > > +   binary="$1"\r
 112 > > +   test "$#" = 2 && name=$2 || name="$binary(1)"\r
 113 > > +\r
 114 > > +        hash $binary 2>/dev/null || eval "\r
 115 > > +$1 () {\r
 116 > > +   echo -n \"\$test_subtest_missing_external_prereqs_\" | grep -e \" $name \" ||\r
 117 > > +   test_subtest_missing_external_prereqs_=\"$test_subtest_missing_external_prereqs_ $name\"\r
 118 > > +   false\r
 119 > > +}"\r
 120 > > +}\r
 121 > > +\r
 122 > \r
 123 > Does this work right ?\r
 124 \r
 125 It does not.\r
 126 \r
 127 Moreover, there is a missing backslash before\r
 128 $test_subtest_missing_external_prereqs_ (and that is why I did not\r
 129 notice the bug during my poor testing).\r
 130 \r
 131 > ... the grep -e \" $name \"   -- part requires\r
 132 > spaces on both side of, but next line does not write trailing space...\r
 133 > ... and is there leading space (and also the latest\r
 134 > $test_subtest_missing_external_prereqs_ (just before 'false') is evaluated) ?\r
 135 > \r
 136 > This could perhaps be written like the above test_set_prereq &\r
 137 > test_save_prereq:\r
 138 > ...\r
 139 >         hash $binary 2>/dev/null || eval "\r
 140 > $binary () {\r
 141 >       test_missing_external_prereq_${binary}_=t\r
 142 >       case \$test_subtest_missing_external_prereqs_ in\r
 143 >               *' $name '*) ;;\r
 144 >                 *) test_subtest_missing_external_prereqs_=\"\$test_subtest_missing_external_prereqs_$name \"\r
 145 >         esac\r
 146 >       false\r
 147 > }\r
 148 > \r
 149 > and  test_subtest_missing_external_prereqs_=' '  done in test_reset_state_\r
 150 > \r
 151 \r
 152 Well, this approach would obviously be better, at least because it does\r
 153 work.  But IMO it is too complex, and essentially has the same problem\r
 154 as the current code: it mixes the check with diagnostic message.\r
 155 \r
 156 We already have a proper way to check if dependency is missing already:\r
 157 test_missing_external_prereq_${binary}_.  We should check it and add the\r
 158 binary to test_subtest_missing_external_prereqs_ if it is not set.  And\r
 159 move setting of test_missing_external_prereq_${binary}_ below.\r
 160 \r
 161 > (I took some code from current git head.... also perhaps instead of \r
 162 > setting test_missing_external_prereq_${binary}_=t, (bash) associative\r
 163 > arrays could be taken into use -- the eval to read that information\r
 164 > is a bit hairy -- well, at least that part works for sure :D )\r
 165 > \r
 166 \r
 167 Yes!  I like using hash here.\r
 168 \r
 169 > Hmm... how about this:\r
 170 > \r
 171 >         hash $binary 2>/dev/null || eval "\r
 172 > $binary () {\r
 173 >         if [ -z \"\${test_missing_external_prereq_[$binary]}\" ]\r
 174 >         then\r
 175 >               test_missing_external_prereq_[$binary]=t\r
 176 >                 test_subtest_missing_external_prereqs_=\"\$test_subtest_missing_external_prereqs_ $name\"\r
 177 >         fi\r
 178 >       false\r
 179 > }\r
 180 > \r
 181 \r
 182 There is some inconsistent indenting in the above code (mixed tabs and\r
 183 spaces).  Also test_require_external_prereq() should be changed as well.\r
 184 \r
 185 Otherwise I like it.  Would you please submit a patch?\r
 186 \r
 187 Regards,\r
 188   Dmitry\r
 189 \r
 190 > and test_subtest_missing_external_prereqs_ cleared in test_reset_state_ like now.\r
 191 > \r
 192 > Tomi\r