--- /dev/null
+Return-Path: <dmitry.kurochkin@gmail.com>\r
+X-Original-To: notmuch@notmuchmail.org\r
+Delivered-To: notmuch@notmuchmail.org\r
+Received: from localhost (localhost [127.0.0.1])\r
+ by olra.theworths.org (Postfix) with ESMTP id B8FAE429E54\r
+ for <notmuch@notmuchmail.org>; Mon, 23 Jan 2012 09:28:04 -0800 (PST)\r
+X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
+X-Spam-Flag: NO\r
+X-Spam-Score: -0.799\r
+X-Spam-Level: \r
+X-Spam-Status: No, score=-0.799 tagged_above=-999 required=5\r
+ tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1,\r
+ FREEMAIL_FROM=0.001, RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled\r
+Received: from olra.theworths.org ([127.0.0.1])\r
+ by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
+ with ESMTP id 9Vu-Zo-eWEx2 for <notmuch@notmuchmail.org>;\r
+ Mon, 23 Jan 2012 09:28:03 -0800 (PST)\r
+Received: from mail-bk0-f53.google.com (mail-bk0-f53.google.com\r
+ [209.85.214.53]) (using TLSv1 with cipher RC4-SHA (128/128 bits))\r
+ (No client certificate requested)\r
+ by olra.theworths.org (Postfix) with ESMTPS id 83838429E21\r
+ for <notmuch@notmuchmail.org>; Mon, 23 Jan 2012 09:28:03 -0800 (PST)\r
+Received: by bkbzt19 with SMTP id zt19so1494361bkb.26\r
+ for <notmuch@notmuchmail.org>; Mon, 23 Jan 2012 09:28:02 -0800 (PST)\r
+DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma;\r
+ h=from:to:subject:in-reply-to:references:user-agent:date:message-id\r
+ :mime-version:content-type:content-transfer-encoding;\r
+ bh=wnm837bWv3Y8k0wuNYnHn/ub9gHvMV+ehkFiEv/4sZk=;\r
+ b=nCwM7p4f15YztZRvzgM5sXDR9J7fEgJTDPxez4DbzeoT8rlVjb2FHqLk+YCSnSJR9t\r
+ NaC6wE9VtG+Imgb7/INweDWY7+xNqCTvDTmrUHbBpwt+Rm0Mjm6PwgipMZColpkKMsgZ\r
+ 3oEvpKg9oOOcZt1vOCuA2x5AvLgZxcAjssEus=\r
+Received: by 10.204.133.201 with SMTP id g9mr3608223bkt.137.1327339679240;\r
+ Mon, 23 Jan 2012 09:27:59 -0800 (PST)\r
+Received: from localhost ([91.144.186.21])\r
+ by mx.google.com with ESMTPS id iu2sm29935832bkb.0.2012.01.23.09.27.58\r
+ (version=TLSv1/SSLv3 cipher=OTHER);\r
+ Mon, 23 Jan 2012 09:27:58 -0800 (PST)\r
+From: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>\r
+To: David Edmondson <dme@dme.org>, notmuch@notmuchmail.org\r
+Subject: Re: [PATCH 3/3] test: Add address cleaning tests.\r
+In-Reply-To: <1326977643-19792-3-git-send-email-dme@dme.org>\r
+References: <yf639bcarcs.fsf@taco2.nixu.fi>\r
+ <1326977643-19792-1-git-send-email-dme@dme.org>\r
+ <1326977643-19792-3-git-send-email-dme@dme.org>\r
+User-Agent: Notmuch/0.11+100~gd650abf (http://notmuchmail.org) Emacs/23.3.1\r
+ (x86_64-pc-linux-gnu)\r
+Date: Mon, 23 Jan 2012 21:26:54 +0400\r
+Message-ID: <87hazm2uu9.fsf@gmail.com>\r
+MIME-Version: 1.0\r
+Content-Type: text/plain; charset=utf-8\r
+Content-Transfer-Encoding: quoted-printable\r
+X-BeenThere: notmuch@notmuchmail.org\r
+X-Mailman-Version: 2.1.13\r
+Precedence: list\r
+List-Id: "Use and development of the notmuch mail system."\r
+ <notmuch.notmuchmail.org>\r
+List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
+ <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
+List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
+List-Post: <mailto:notmuch@notmuchmail.org>\r
+List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
+List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
+ <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
+X-List-Received-Date: Mon, 23 Jan 2012 17:28:04 -0000\r
+\r
+On Thu, 19 Jan 2012 12:54:03 +0000, David Edmondson <dme@dme.org> wrote:\r
+> Including some more test framework in test-lib.el.\r
+\r
+IMO test-lib.el changes should be in a separate patch.\r
+\r
+> ---\r
+> test/emacs-address-cleaning.el | 29 +++++++++++++++++++++++++++++\r
+> test/emacs-address-cleaning.sh | 12 ++++++++++++\r
+> test/notmuch-test | 1 +\r
+> test/test-lib.el | 30 ++++++++++++++++++++++++++++++\r
+> 4 files changed, 72 insertions(+), 0 deletions(-)\r
+> create mode 100644 test/emacs-address-cleaning.el\r
+> create mode 100755 test/emacs-address-cleaning.sh\r
+>=20\r
+> diff --git a/test/emacs-address-cleaning.el b/test/emacs-address-cleaning=\r
+.el\r
+> new file mode 100644\r
+> index 0000000..59e8d92\r
+> --- /dev/null\r
+> +++ b/test/emacs-address-cleaning.el\r
+> @@ -0,0 +1,29 @@\r
+> +(defun notmuch-test-address-cleaning-1 ()\r
+> + (notmuch-test-compare (notmuch-show-clean-address "dme@dme.org")\r
+> + "dme@dme.org"))\r
+> +\r
+> +(defun notmuch-test-address-cleaning-2 ()\r
+> + (let* ((input '("foo@bar.com"\r
+> + "<foo@bar.com>"\r
+> + "Foo Bar <foo@bar.com>"\r
+> + "foo@bar.com <foo@bar.com>"\r
+> + "\"Foo Bar\" <foo@bar.com>"))\r
+> + (expected '("foo@bar.com"\r
+> + "foo@bar.com"\r
+> + "Foo Bar <foo@bar.com>"\r
+> + "foo@bar.com"\r
+> + "Foo Bar <foo@bar.com>"))\r
+> + (output (mapcar #'notmuch-show-clean-address input)))\r
+> + (notmuch-test-compare output expected)))\r
+> +\r
+> +(defun notmuch-test-address-cleaning-3 ()\r
+> + (let* ((input '("=D0=94=D0=91 <db-uknot@stop.me.uk>"\r
+> + "foo (at home) <foo@bar.com>"\r
+> + "foo [at home] <foo@bar.com>"\r
+> + "Foo Bar"))\r
+> + (expected '("=D0=94=D0=91 <db-uknot@stop.me.uk>"\r
+> + "foo (at home) <foo@bar.com>"\r
+> + "foo [at home] <foo@bar.com>"\r
+> + "Foo Bar"))\r
+> + (output (mapcar #'notmuch-show-clean-address input)))\r
+> + (notmuch-test-compare output expected)))\r
+\r
+At a glance, the tests look good. Though more review of the tests\r
+itself would be nice.\r
+\r
+> diff --git a/test/emacs-address-cleaning.sh b/test/emacs-address-cleaning=\r
+.sh\r
+> new file mode 100755\r
+> index 0000000..1a6eff5\r
+> --- /dev/null\r
+> +++ b/test/emacs-address-cleaning.sh\r
+> @@ -0,0 +1,12 @@\r
+> +#!/usr/bin/env bash\r
+> +\r
+> +test_description=3D"emacs address cleaning"\r
+> +. test-lib.sh\r
+> +\r
+> +for i in 1 2 3; do\r
+> + test_begin_subtest "notmuch-test-address-clean-$i"\r
+> + test_emacs_expect_t \\r
+> + '(load "emacs-address-cleaning.el") (notmuch-test-address-cleaning-'$i'=\r
+)'\r
+> +done\r
+> +\r
+> +test_done\r
+> diff --git a/test/notmuch-test b/test/notmuch-test\r
+> index d034f99..3f1740c 100755\r
+> --- a/test/notmuch-test\r
+> +++ b/test/notmuch-test\r
+> @@ -53,6 +53,7 @@ TESTS=3D"\r
+> hooks\r
+> argument-parsing\r
+> emacs-test-functions.sh\r
+> + emacs-address-cleaning.sh\r
+> "\r
+> TESTS=3D${NOTMUCH_TESTS:=3D$TESTS}\r
+>=20=20\r
+> diff --git a/test/test-lib.el b/test/test-lib.el\r
+> index 1d51b8d..033270d 100644\r
+> --- a/test/test-lib.el\r
+> +++ b/test/test-lib.el\r
+> @@ -20,6 +20,8 @@\r
+> ;;\r
+> ;; Authors: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>\r
+>=20=20\r
+> +(require 'cl) ;; This code is generally used uncompiled.\r
+> +\r
+> ;; `read-file-name' by default uses `completing-read' function to read\r
+> ;; user input. It does not respect `standard-input' variable which we\r
+> ;; use in tests to provide user input. So replace it with a plain\r
+> @@ -77,6 +79,7 @@ nothing."\r
+> (add-hook-counter 'notmuch-hello-mode-hook)\r
+> (add-hook-counter 'notmuch-hello-refresh-hook)\r
+>=20=20\r
+> +\r
+\r
+Please revert.\r
+\r
+> (defmacro notmuch-test-run-test (&rest body)\r
+> "Evaluate a BODY of test expressions and output the result."\r
+> `(with-temp-buffer\r
+> @@ -85,3 +88,30 @@ nothing."\r
+> result\r
+> (prin1-to-string result)))\r
+> (test-output))))\r
+> +\r
+> +;; Functions to help when writing tests:\r
+> +\r
+\r
+IMO this comment is misplaced. There are other functions above which\r
+also help to write tests e.g. `test-output. I think we should either\r
+remove the comment or move all test helpers in one place below the\r
+comment (in a separate patch).\r
+\r
+> +(defun notmuch-test-reporter (output expected)\r
+\r
+Please consider renaming to `notmuch-test-report-unexpected'.\r
+\r
+> + "Report that the `output' does not match the `expected' result."\r
+> + (concat "Expect:\t" (prin1-to-string expected) "\n"\r
+> + "Output:\t" (prin1-to-string output) "\n"))\r
+> +\r
+> +(defun notmuch-test-unsimilar (output expected)\r
+\r
+Please consider renaming to notmuch-test-unexpected.\r
+\r
+> + "`output' and `expected' are dissimilar. Show a summary of\r
+\r
+Previous line says "unsimilar", now it is "dissimilar". I am not sure\r
+which one is correct. Also, it is not clear what "similar" means for\r
+test results. IMO "not equal" would be more clear.\r
+\r
+> +the differences, ignoring similarities."\r
+> + (cond ((and (listp output)\r
+> + (listp expected))\r
+> + (apply #'concat (loop for o in output\r
+> + for e in expected\r
+> + if (not (equal o e))\r
+> + collect (notmuch-test-reporter o e))))\r
+> +\r
+> + (t\r
+> + ;; Catch all case.\r
+> + (notmuch-test-reporter output expected))))\r
+> +\r
+> +(defun notmuch-test-compare (output expected)\r
+\r
+Please consider renaming to notmuch-test-expect-equal for consistency\r
+with shell tests.\r
+\r
+> + "Compare `output' with `expected'. Report any discrepencies."\r
+> + (if (equal output expected)\r
+> + t\r
+> + (notmuch-test-unsimilar output expected)))\r
+\r
+I do not like that equality check is split between notmuch-test-compare\r
+and notmuch-test-unsimilar. It makes the code harder to read (took me a\r
+while to understand why "catch all case" above does not need the\r
+equality check). Also, notmuch-test-unsimilar would be hard to reuse\r
+because the comparison check is done in one case and not done in\r
+another. How about merging them? The code would look something like:\r
+\r
+ (cond\r
+ (both are lists)\r
+ ; code from notmuch-test-unsimilar\r
+ (catch all case)\r
+ (if not equal)\r
+ (notmuch-test-report-unexpected output expected))\r
+\r
+Regards,\r
+ Dmitry\r
+\r
+> --=20\r
+> 1.7.8.3\r
+>=20\r
+> _______________________________________________\r
+> notmuch mailing list\r
+> notmuch@notmuchmail.org\r
+> http://notmuchmail.org/mailman/listinfo/notmuch\r
projects / notmuch-archives.git / commitdiff