From: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>
To: David Edmondson <dme@dme.org>, notmuch@notmuchmail.org
Subject: Re: [PATCH 3/3] test: Add address cleaning tests.
In-Reply-To: <1326977643-19792-3-git-send-email-dme@dme.org>
References: <yf639bcarcs.fsf@taco2.nixu.fi>
	<1326977643-19792-1-git-send-email-dme@dme.org>
	<1326977643-19792-3-git-send-email-dme@dme.org>
User-Agent: Notmuch/0.11+100~gd650abf (http://notmuchmail.org) Emacs/23.3.1
	(x86_64-pc-linux-gnu)
Date: Mon, 23 Jan 2012 21:26:54 +0400
Message-ID: <87hazm2uu9.fsf@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: list

On Thu, 19 Jan 2012 12:54:03 +0000, David Edmondson <dme@dme.org> wrote:
> Including some more test framework in test-lib.el.

IMO test-lib.el changes should be in a separate patch.

> ---
>  test/emacs-address-cleaning.el |   29 +++++++++++++++++++++++++++++
>  test/emacs-address-cleaning.sh |   12 ++++++++++++
>  test/notmuch-test              |    1 +
>  test/test-lib.el               |   30 ++++++++++++++++++++++++++++++
>  4 files changed, 72 insertions(+), 0 deletions(-)
>  create mode 100644 test/emacs-address-cleaning.el
>  create mode 100755 test/emacs-address-cleaning.sh
>=20
> diff --git a/test/emacs-address-cleaning.el b/test/emacs-address-cleaning=
.el
> new file mode 100644
> index 0000000..59e8d92
> --- /dev/null
> +++ b/test/emacs-address-cleaning.el
> @@ -0,0 +1,29 @@
> +(defun notmuch-test-address-cleaning-1 ()
> +  (notmuch-test-compare (notmuch-show-clean-address "dme@dme.org")
> +			"dme@dme.org"))
> +
> +(defun notmuch-test-address-cleaning-2 ()
> +  (let* ((input '("foo@bar.com"
> +		  "<foo@bar.com>"
> +		  "Foo Bar <foo@bar.com>"
> +		  "foo@bar.com <foo@bar.com>"
> +		  "\"Foo Bar\" <foo@bar.com>"))
> +	 (expected '("foo@bar.com"
> +		     "foo@bar.com"
> +		     "Foo Bar <foo@bar.com>"
> +		     "foo@bar.com"
> +		     "Foo Bar <foo@bar.com>"))
> +	 (output (mapcar #'notmuch-show-clean-address input)))
> +    (notmuch-test-compare output expected)))
> +
> +(defun notmuch-test-address-cleaning-3 ()
> +  (let* ((input '("=D0=94=D0=91 <db-uknot@stop.me.uk>"
> +		  "foo (at home) <foo@bar.com>"
> +		  "foo [at home] <foo@bar.com>"
> +		  "Foo Bar"))
> +	 (expected '("=D0=94=D0=91 <db-uknot@stop.me.uk>"
> +		     "foo (at home) <foo@bar.com>"
> +		     "foo [at home] <foo@bar.com>"
> +		     "Foo Bar"))
> +	 (output (mapcar #'notmuch-show-clean-address input)))
> +    (notmuch-test-compare output expected)))

At a glance, the tests look good.  Though more review of the tests
itself would be nice.

> diff --git a/test/emacs-address-cleaning.sh b/test/emacs-address-cleaning=
.sh
> new file mode 100755
> index 0000000..1a6eff5
> --- /dev/null
> +++ b/test/emacs-address-cleaning.sh
> @@ -0,0 +1,12 @@
> +#!/usr/bin/env bash
> +
> +test_description=3D"emacs address cleaning"
> +. test-lib.sh
> +
> +for i in 1 2 3; do
> +    test_begin_subtest "notmuch-test-address-clean-$i"
> +    test_emacs_expect_t \
> +	'(load "emacs-address-cleaning.el") (notmuch-test-address-cleaning-'$i'=
)'
> +done
> +
> +test_done
> diff --git a/test/notmuch-test b/test/notmuch-test
> index d034f99..3f1740c 100755
> --- a/test/notmuch-test
> +++ b/test/notmuch-test
> @@ -53,6 +53,7 @@ TESTS=3D"
>    hooks
>    argument-parsing
>    emacs-test-functions.sh
> +  emacs-address-cleaning.sh
>  "
>  TESTS=3D${NOTMUCH_TESTS:=3D$TESTS}
>=20=20
> diff --git a/test/test-lib.el b/test/test-lib.el
> index 1d51b8d..033270d 100644
> --- a/test/test-lib.el
> +++ b/test/test-lib.el
> @@ -20,6 +20,8 @@
>  ;;
>  ;; Authors: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>
>=20=20
> +(require 'cl)	;; This code is generally used uncompiled.
> +
>  ;; `read-file-name' by default uses `completing-read' function to read
>  ;; user input.  It does not respect `standard-input' variable which we
>  ;; use in tests to provide user input.  So replace it with a plain
> @@ -77,6 +79,7 @@ nothing."
>  (add-hook-counter 'notmuch-hello-mode-hook)
>  (add-hook-counter 'notmuch-hello-refresh-hook)
>=20=20
> +

Please revert.

>  (defmacro notmuch-test-run-test (&rest body)
>    "Evaluate a BODY of test expressions and output the result."
>    `(with-temp-buffer
> @@ -85,3 +88,30 @@ nothing."
>  		   result
>  		 (prin1-to-string result)))
>         (test-output))))
> +
> +;; Functions to help when writing tests:
> +

IMO this comment is misplaced.  There are other functions above which
also help to write tests e.g. `test-output.  I think we should either
remove the comment or move all test helpers in one place below the
comment (in a separate patch).

> +(defun notmuch-test-reporter (output expected)

Please consider renaming to `notmuch-test-report-unexpected'.

> +  "Report that the `output' does not match the `expected' result."
> +  (concat "Expect:\t" (prin1-to-string expected) "\n"
> +	  "Output:\t" (prin1-to-string output) "\n"))
> +
> +(defun notmuch-test-unsimilar (output expected)

Please consider renaming to notmuch-test-unexpected.

> +  "`output' and `expected' are dissimilar. Show a summary of

Previous line says "unsimilar", now it is "dissimilar".  I am not sure
which one is correct.  Also, it is not clear what "similar" means for
test results.  IMO "not equal" would be more clear.

> +the differences, ignoring similarities."
> +  (cond ((and (listp output)
> +	      (listp expected))
> +	 (apply #'concat (loop for o in output
> +			       for e in expected
> +			       if (not (equal o e))
> +			       collect (notmuch-test-reporter o e))))
> +
> +	(t
> +	 ;; Catch all case.
> +	 (notmuch-test-reporter output expected))))
> +
> +(defun notmuch-test-compare (output expected)

Please consider renaming to notmuch-test-expect-equal for consistency
with shell tests.

> +  "Compare `output' with `expected'. Report any discrepencies."
> +  (if (equal output expected)
> +      t
> +    (notmuch-test-unsimilar output expected)))

I do not like that equality check is split between notmuch-test-compare
and notmuch-test-unsimilar.  It makes the code harder to read (took me a
while to understand why "catch all case" above does not need the
equality check).  Also, notmuch-test-unsimilar would be hard to reuse
because the comparison check is done in one case and not done in
another.  How about merging them?  The code would look something like:

  (cond
    (both are lists)
      ; code from notmuch-test-unsimilar
    (catch all case)
      (if not equal)
        (notmuch-test-report-unexpected output expected))

Regards,
  Dmitry

> --=20
> 1.7.8.3
>=20
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch