ad/c2529b247258f731119555aabec18b56599c52

   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 B8FAE429E54\r
   6         for <notmuch@notmuchmail.org>; Mon, 23 Jan 2012 09:28:04 -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 9Vu-Zo-eWEx2 for <notmuch@notmuchmail.org>;\r
  17         Mon, 23 Jan 2012 09:28:03 -0800 (PST)\r
  18 Received: from mail-bk0-f53.google.com (mail-bk0-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 83838429E21\r
  22         for <notmuch@notmuchmail.org>; Mon, 23 Jan 2012 09:28:03 -0800 (PST)\r
  23 Received: by bkbzt19 with SMTP id zt19so1494361bkb.26\r
  24         for <notmuch@notmuchmail.org>; Mon, 23 Jan 2012 09:28:02 -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:content-transfer-encoding;\r
  28         bh=wnm837bWv3Y8k0wuNYnHn/ub9gHvMV+ehkFiEv/4sZk=;\r
  29         b=nCwM7p4f15YztZRvzgM5sXDR9J7fEgJTDPxez4DbzeoT8rlVjb2FHqLk+YCSnSJR9t\r
  30         NaC6wE9VtG+Imgb7/INweDWY7+xNqCTvDTmrUHbBpwt+Rm0Mjm6PwgipMZColpkKMsgZ\r
  31         3oEvpKg9oOOcZt1vOCuA2x5AvLgZxcAjssEus=\r
  32 Received: by 10.204.133.201 with SMTP id g9mr3608223bkt.137.1327339679240;\r
  33         Mon, 23 Jan 2012 09:27:59 -0800 (PST)\r
  34 Received: from localhost ([91.144.186.21])\r
  35         by mx.google.com with ESMTPS id iu2sm29935832bkb.0.2012.01.23.09.27.58\r
  36         (version=TLSv1/SSLv3 cipher=OTHER);\r
  37         Mon, 23 Jan 2012 09:27:58 -0800 (PST)\r
  38 From: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>\r
  39 To: David Edmondson <dme@dme.org>, notmuch@notmuchmail.org\r
  40 Subject: Re: [PATCH 3/3] test: Add address cleaning tests.\r
  41 In-Reply-To: <1326977643-19792-3-git-send-email-dme@dme.org>\r
  42 References: <yf639bcarcs.fsf@taco2.nixu.fi>\r
  43         <1326977643-19792-1-git-send-email-dme@dme.org>\r
  44         <1326977643-19792-3-git-send-email-dme@dme.org>\r
  45 User-Agent: Notmuch/0.11+100~gd650abf (http://notmuchmail.org) Emacs/23.3.1\r
  46         (x86_64-pc-linux-gnu)\r
  47 Date: Mon, 23 Jan 2012 21:26:54 +0400\r
  48 Message-ID: <87hazm2uu9.fsf@gmail.com>\r
  49 MIME-Version: 1.0\r
  50 Content-Type: text/plain; charset=utf-8\r
  51 Content-Transfer-Encoding: quoted-printable\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, 23 Jan 2012 17:28:04 -0000\r
  65 \r
  66 On Thu, 19 Jan 2012 12:54:03 +0000, David Edmondson <dme@dme.org> wrote:\r
  67 > Including some more test framework in test-lib.el.\r
  68 \r
  69 IMO test-lib.el changes should be in a separate patch.\r
  70 \r
  71 > ---\r
  72 >  test/emacs-address-cleaning.el |   29 +++++++++++++++++++++++++++++\r
  73 >  test/emacs-address-cleaning.sh |   12 ++++++++++++\r
  74 >  test/notmuch-test              |    1 +\r
  75 >  test/test-lib.el               |   30 ++++++++++++++++++++++++++++++\r
  76 >  4 files changed, 72 insertions(+), 0 deletions(-)\r
  77 >  create mode 100644 test/emacs-address-cleaning.el\r
  78 >  create mode 100755 test/emacs-address-cleaning.sh\r
  79 >=20\r
  80 > diff --git a/test/emacs-address-cleaning.el b/test/emacs-address-cleaning=\r
  81 .el\r
  82 > new file mode 100644\r
  83 > index 0000000..59e8d92\r
  84 > --- /dev/null\r
  85 > +++ b/test/emacs-address-cleaning.el\r
  86 > @@ -0,0 +1,29 @@\r
  87 > +(defun notmuch-test-address-cleaning-1 ()\r
  88 > +  (notmuch-test-compare (notmuch-show-clean-address "dme@dme.org")\r
  89 > +                     "dme@dme.org"))\r
  90 > +\r
  91 > +(defun notmuch-test-address-cleaning-2 ()\r
  92 > +  (let* ((input '("foo@bar.com"\r
  93 > +               "<foo@bar.com>"\r
  94 > +               "Foo Bar <foo@bar.com>"\r
  95 > +               "foo@bar.com <foo@bar.com>"\r
  96 > +               "\"Foo Bar\" <foo@bar.com>"))\r
  97 > +      (expected '("foo@bar.com"\r
  98 > +                  "foo@bar.com"\r
  99 > +                  "Foo Bar <foo@bar.com>"\r
 100 > +                  "foo@bar.com"\r
 101 > +                  "Foo Bar <foo@bar.com>"))\r
 102 > +      (output (mapcar #'notmuch-show-clean-address input)))\r
 103 > +    (notmuch-test-compare output expected)))\r
 104 > +\r
 105 > +(defun notmuch-test-address-cleaning-3 ()\r
 106 > +  (let* ((input '("=D0=94=D0=91 <db-uknot@stop.me.uk>"\r
 107 > +               "foo (at home) <foo@bar.com>"\r
 108 > +               "foo [at home] <foo@bar.com>"\r
 109 > +               "Foo Bar"))\r
 110 > +      (expected '("=D0=94=D0=91 <db-uknot@stop.me.uk>"\r
 111 > +                  "foo (at home) <foo@bar.com>"\r
 112 > +                  "foo [at home] <foo@bar.com>"\r
 113 > +                  "Foo Bar"))\r
 114 > +      (output (mapcar #'notmuch-show-clean-address input)))\r
 115 > +    (notmuch-test-compare output expected)))\r
 116 \r
 117 At a glance, the tests look good.  Though more review of the tests\r
 118 itself would be nice.\r
 119 \r
 120 > diff --git a/test/emacs-address-cleaning.sh b/test/emacs-address-cleaning=\r
 121 .sh\r
 122 > new file mode 100755\r
 123 > index 0000000..1a6eff5\r
 124 > --- /dev/null\r
 125 > +++ b/test/emacs-address-cleaning.sh\r
 126 > @@ -0,0 +1,12 @@\r
 127 > +#!/usr/bin/env bash\r
 128 > +\r
 129 > +test_description=3D"emacs address cleaning"\r
 130 > +. test-lib.sh\r
 131 > +\r
 132 > +for i in 1 2 3; do\r
 133 > +    test_begin_subtest "notmuch-test-address-clean-$i"\r
 134 > +    test_emacs_expect_t \\r
 135 > +     '(load "emacs-address-cleaning.el") (notmuch-test-address-cleaning-'$i'=\r
 136 )'\r
 137 > +done\r
 138 > +\r
 139 > +test_done\r
 140 > diff --git a/test/notmuch-test b/test/notmuch-test\r
 141 > index d034f99..3f1740c 100755\r
 142 > --- a/test/notmuch-test\r
 143 > +++ b/test/notmuch-test\r
 144 > @@ -53,6 +53,7 @@ TESTS=3D"\r
 145 >    hooks\r
 146 >    argument-parsing\r
 147 >    emacs-test-functions.sh\r
 148 > +  emacs-address-cleaning.sh\r
 149 >  "\r
 150 >  TESTS=3D${NOTMUCH_TESTS:=3D$TESTS}\r
 151 >=20=20\r
 152 > diff --git a/test/test-lib.el b/test/test-lib.el\r
 153 > index 1d51b8d..033270d 100644\r
 154 > --- a/test/test-lib.el\r
 155 > +++ b/test/test-lib.el\r
 156 > @@ -20,6 +20,8 @@\r
 157 >  ;;\r
 158 >  ;; Authors: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>\r
 159 >=20=20\r
 160 > +(require 'cl)        ;; This code is generally used uncompiled.\r
 161 > +\r
 162 >  ;; `read-file-name' by default uses `completing-read' function to read\r
 163 >  ;; user input.  It does not respect `standard-input' variable which we\r
 164 >  ;; use in tests to provide user input.  So replace it with a plain\r
 165 > @@ -77,6 +79,7 @@ nothing."\r
 166 >  (add-hook-counter 'notmuch-hello-mode-hook)\r
 167 >  (add-hook-counter 'notmuch-hello-refresh-hook)\r
 168 >=20=20\r
 169 > +\r
 170 \r
 171 Please revert.\r
 172 \r
 173 >  (defmacro notmuch-test-run-test (&rest body)\r
 174 >    "Evaluate a BODY of test expressions and output the result."\r
 175 >    `(with-temp-buffer\r
 176 > @@ -85,3 +88,30 @@ nothing."\r
 177 >                  result\r
 178 >                (prin1-to-string result)))\r
 179 >         (test-output))))\r
 180 > +\r
 181 > +;; Functions to help when writing tests:\r
 182 > +\r
 183 \r
 184 IMO this comment is misplaced.  There are other functions above which\r
 185 also help to write tests e.g. `test-output.  I think we should either\r
 186 remove the comment or move all test helpers in one place below the\r
 187 comment (in a separate patch).\r
 188 \r
 189 > +(defun notmuch-test-reporter (output expected)\r
 190 \r
 191 Please consider renaming to `notmuch-test-report-unexpected'.\r
 192 \r
 193 > +  "Report that the `output' does not match the `expected' result."\r
 194 > +  (concat "Expect:\t" (prin1-to-string expected) "\n"\r
 195 > +       "Output:\t" (prin1-to-string output) "\n"))\r
 196 > +\r
 197 > +(defun notmuch-test-unsimilar (output expected)\r
 198 \r
 199 Please consider renaming to notmuch-test-unexpected.\r
 200 \r
 201 > +  "`output' and `expected' are dissimilar. Show a summary of\r
 202 \r
 203 Previous line says "unsimilar", now it is "dissimilar".  I am not sure\r
 204 which one is correct.  Also, it is not clear what "similar" means for\r
 205 test results.  IMO "not equal" would be more clear.\r
 206 \r
 207 > +the differences, ignoring similarities."\r
 208 > +  (cond ((and (listp output)\r
 209 > +           (listp expected))\r
 210 > +      (apply #'concat (loop for o in output\r
 211 > +                            for e in expected\r
 212 > +                            if (not (equal o e))\r
 213 > +                            collect (notmuch-test-reporter o e))))\r
 214 > +\r
 215 > +     (t\r
 216 > +      ;; Catch all case.\r
 217 > +      (notmuch-test-reporter output expected))))\r
 218 > +\r
 219 > +(defun notmuch-test-compare (output expected)\r
 220 \r
 221 Please consider renaming to notmuch-test-expect-equal for consistency\r
 222 with shell tests.\r
 223 \r
 224 > +  "Compare `output' with `expected'. Report any discrepencies."\r
 225 > +  (if (equal output expected)\r
 226 > +      t\r
 227 > +    (notmuch-test-unsimilar output expected)))\r
 228 \r
 229 I do not like that equality check is split between notmuch-test-compare\r
 230 and notmuch-test-unsimilar.  It makes the code harder to read (took me a\r
 231 while to understand why "catch all case" above does not need the\r
 232 equality check).  Also, notmuch-test-unsimilar would be hard to reuse\r
 233 because the comparison check is done in one case and not done in\r
 234 another.  How about merging them?  The code would look something like:\r
 235 \r
 236   (cond\r
 237     (both are lists)\r
 238       ; code from notmuch-test-unsimilar\r
 239     (catch all case)\r
 240       (if not equal)\r
 241         (notmuch-test-report-unexpected output expected))\r
 242 \r
 243 Regards,\r
 244   Dmitry\r
 245 \r
 246 > --=20\r
 247 > 1.7.8.3\r
 248 >=20\r
 249 > _______________________________________________\r
 250 > notmuch mailing list\r
 251 > notmuch@notmuchmail.org\r
 252 > http://notmuchmail.org/mailman/listinfo/notmuch\r