[PATCH 1/2] test: Add "test_expect_emacs_t" to assist with emacs testing.

[PATCH 1/2] test: Add "test_expect_emacs_t" to assist with emacs testing.

[David Edmondson]

"test_expect_emacs_t" expects a single argument. If this is `t' then
the test passes. Otherwise the argument provides the details of the
test failure to be reported.
---

For use in the following set of tests and later for other emacs related tests.

 test/test-lib.sh |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/test/test-lib.sh b/test/test-lib.sh
index d1fbc05..0ffe9a6 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -503,6 +503,26 @@ test_expect_equal_file ()
     fi
 }
 
+# Pass test if the result is `t', else report the results.
+test_expect_emacs_t ()
+{
+	exec 1>&6 2>&7		# Restore stdout and stderr
+	inside_subtest=
+	test "$#" = 1 ||
+	error "bug in the test script: not 1 parameter to test_expect_emacs_t"
+
+	result="$1"
+	if ! test_skip "$test_subtest_name"
+	then
+		if [ "${result}" == "t" ] ; then
+			test_ok_ "$test_subtest_name"
+		else
+			testname=$this_test.$test_count
+			test_failure_ "$test_subtest_name" "$(eval printf ${result})"
+		fi
+    fi
+}
+
 NOTMUCH_NEW ()
 {
     notmuch new | grep -v -E -e '^Processed [0-9]*( total)? file|Found [0-9]* total file'
-- 
1.7.7.3

[PATCH 2/2] test: Add tests for `notmuch-show-test-clean-address'.

[David Edmondson]

---

Add three tests. The third one currently fails.

 emacs/notmuch-test.el |   87 +++++++++++++++++++++++++++++++++++++++++++++++++
 test/emacs            |    6 +++
 2 files changed, 93 insertions(+), 0 deletions(-)
 create mode 100644 emacs/notmuch-test.el

diff --git a/emacs/notmuch-test.el b/emacs/notmuch-test.el
new file mode 100644
index 0000000..d6cf318
--- /dev/null
+++ b/emacs/notmuch-test.el
@@ -0,0 +1,87 @@
+;; notmuch-test.el --- testing the emacs interface
+;;
+;; Copyright © David Edmondson
+;;
+;; This file is part of Notmuch.
+;;
+;; Notmuch is free software: you can redistribute it and/or modify it
+;; under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+;;
+;; Notmuch is distributed in the hope that it will be useful, but
+;; WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+;; General Public License for more details.
+;;
+;; You should have received a copy of the GNU General Public License
+;; along with Notmuch.  If not, see <http://www.gnu.org/licenses/>.
+;;
+;; Authors: David Edmondson <dme@dme.org>
+
+(require 'cl)	;; This code is often used uncompiled.
+
+;;
+
+(defun notmuch-test-reporter (output expected)
+  "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)
+  "`output' and `expected' are dissimilar. Show a summary of
+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)
+  "Compare `output' with `expected'. Report any discrepencies."
+  (if (equal output expected)
+      t
+    (notmuch-test-unsimilar output expected)))
+
+;;
+
+;; Tests:
+
+(defun notmuch-test-clean-address-1 ()
+  (notmuch-test-compare (notmuch-show-clean-address "dme@dme.org")
+			"dme@dme.org"))
+
+(defun notmuch-test-clean-address-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-clean-address-3 ()
+  (let* ((input '("ДБ <db-uknot@stop.me.uk>"
+		  "foo (at home) <foo@bar.com>"
+		  "foo [at home] <foo@bar.com>"
+		  "Foo Bar"))
+	 (expected '("ДБ <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)))
+
+;;
+
+(provide 'notmuch-test)
diff --git a/test/emacs b/test/emacs
index ac47b16..3a536fb 100755
--- a/test/emacs
+++ b/test/emacs
@@ -514,4 +514,10 @@ counter=$(test_emacs \
 )
 test_expect_equal "$counter" 2
 
+for i in 1 2 3; do
+    test_begin_subtest "notmuch-show-clean-address $i"
+    result=$(test_emacs '(load "notmuch-test.el") (notmuch-test-clean-address-'$i')')
+    test_expect_emacs_t "$result"
+done
+
 test_done
-- 
1.7.7.3

[PATCH 1/2] emacs: Avoid `mail-header-parse-address' in `notmuch-show-clean-address'.

[David Edmondson]

`mail-header-parse-address' expects un-decoded mailbox parts, which is
not what we have at this point. Replace it with simple string
deconstruction.
---

Fix the failing test in the previous set.

 emacs/notmuch-show.el |   48 +++++++++++++++++++++++++++++++++++-------------
 1 files changed, 35 insertions(+), 13 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 1a250a3..02819f3 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -227,21 +227,43 @@ indentation."
   "Try to clean a single email ADDRESS for display.  Return
 unchanged ADDRESS if parsing fails."
   (condition-case nil
-    (let* ((parsed (mail-header-parse-address address))
-	   (address (car parsed))
-	   (name (cdr parsed)))
-      ;; Remove double quotes. They might be required during transport,
-      ;; but we don't need to see them.
-      (when name
-        (setq name (replace-regexp-in-string "\"" "" name)))
+    (let (p-name p-address)
+      ;; It would be convenient to use `mail-header-parse-address',
+      ;; but that expects un-decoded mailbox parts, whereas our
+      ;; mailbox parts are already decoded (and hence may contain
+      ;; UTF-8). Given that notmuch should handle most of the awkward
+      ;; cases, some simple string deconstruction should be sufficient
+      ;; here.
+      (cond
+       ;; "User <user@dom.ain>" style.
+       ((string-match "\\(.*\\) <\\(.*\\)>" address)
+	(setq p-name (match-string 1 address)
+	      p-address (match-string 2 address)))
+
+       ;; "<user@dom.ain>" style.
+       ((string-match "<\\(.*\\)>" address)
+	(setq p-address (match-string 1 address)))
+
+       ;; Everything else.
+       (t
+	(setq p-address address)))
+      
+      ;; Remove outer double quotes. They might be required during
+      ;; transport, but we don't need to see them.
+      (when (and p-name
+		 (string-match "^\"\\(.*\\)\"$" p-name))
+        (setq p-name (match-string 1 p-name)))
+
       ;; If the address is 'foo@bar.com <foo@bar.com>' then show just
       ;; 'foo@bar.com'.
-      (when (string= name address)
-        (setq name nil))
-
-      (if (not name)
-        address
-        (concat name " <" address ">")))
+      (when (string= p-name p-address)
+        (setq p-name nil))
+
+      ;; If no name results, return just the address.
+      (if (not p-name)
+	  p-address
+	;; Otherwise format the name and address together.
+	(concat p-name " <" p-address ">")))
     (error address)))
 
 (defun notmuch-show-insert-headerline (headers date tags depth)
-- 
1.7.7.3

[PATCH 2/2] emacs: Another special case for `notmuch-show-clean-address'.

[David Edmondson]

Remove backslashes.
---

Including a new test case.

 emacs/notmuch-show.el |   14 +++++++++-----
 emacs/notmuch-test.el |    6 ++++--
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 02819f3..6b9d1b8 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -248,11 +248,15 @@ unchanged ADDRESS if parsing fails."
        (t
 	(setq p-address address)))
       
-      ;; Remove outer double quotes. They might be required during
-      ;; transport, but we don't need to see them.
-      (when (and p-name
-		 (string-match "^\"\\(.*\\)\"$" p-name))
-        (setq p-name (match-string 1 p-name)))
+      ;; Remove elements of the mailbox part that are not relevant for
+      ;; display, even if they are required during transport.
+      (when p-name
+	;; Outer double quotes.
+	(when (string-match "^\"\\(.*\\)\"$" p-name)
+	  (setq p-name (match-string 1 p-name)))
+
+	;; Backslashes.
+	(setq p-name (replace-regexp-in-string "\\\\" "" p-name)))
 
       ;; If the address is 'foo@bar.com <foo@bar.com>' then show just
       ;; 'foo@bar.com'.
diff --git a/emacs/notmuch-test.el b/emacs/notmuch-test.el
index d6cf318..0637818 100644
--- a/emacs/notmuch-test.el
+++ b/emacs/notmuch-test.el
@@ -74,11 +74,13 @@ the differences, ignoring similarities."
   (let* ((input '("ДБ <db-uknot@stop.me.uk>"
 		  "foo (at home) <foo@bar.com>"
 		  "foo [at home] <foo@bar.com>"
-		  "Foo Bar"))
+		  "Foo Bar"
+		  "Fred Dibna \\[extraordinaire\\] <fred@dibna.com>"))
 	 (expected '("ДБ <db-uknot@stop.me.uk>"
 		     "foo (at home) <foo@bar.com>"
 		     "foo [at home] <foo@bar.com>"
-		     "Foo Bar"))
+		     "Foo Bar"
+		     "Fred Dibna [extraordinaire] <fred@dibna.com>"))
 	 (output (mapcar #'notmuch-show-clean-address input)))
     (notmuch-test-compare output expected)))
 
-- 
1.7.7.3

Re: [PATCH 1/2] test: Add "test_expect_emacs_t" to assist with emacs testing.

[David Edmondson]

[-- Attachment #1: Type: text/plain, Size: 866 bytes --]

I asked for feedback on the approach of implementing tests in .el[1] and
got only positive feedback (well, only one bit of feedback at all).

Here are some simpler tests implemented using this approach, trying to
address the concerns about seeing what failed. The test output is now
something like:

 PASS   notmuch-show-clean-address 1
 PASS   notmuch-show-clean-address 2
 FAIL   notmuch-show-clean-address 3
        Expect: "ДБ <db-uknot@stop.me.uk>"
        Output: "db-uknot@stop.me.uk"
        Expect: "foo (at home) <foo@bar.com>"
        Output: "foo <foo@bar.com>"
        Expect: "foo [at home] <foo@bar.com>"
        Output: "foo at home <foo@bar.com>"
        Expect: "Foo Bar"
        Output: nil

Obviously the first two tests passed and the third failed.

Footnotes: 
[1]  id:"cunmxabqs6k.fsf@hotblack-desiato.hh.sledj.net"


[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 1/2] test: Add "test_expect_emacs_t" to assist with emacs testing.

[Dmitry Kurochkin]

Hi David.

On Mon, 16 Jan 2012 16:47:32 +0000, David Edmondson <dme@dme.org> wrote:
> "test_expect_emacs_t" expects a single argument. If this is `t' then
> the test passes. Otherwise the argument provides the details of the
> test failure to be reported.

Isn't this function the same as something like "test_expect_equal $x t"?

IMO the function seems too complex for what it does (and basically that
is "compare $x with t").  Why do we need to implement it in such a way?

Also, the function has nothing to do with emacs (except for the fact
that 't' is commonly used in it).

I think a more useful approach would be to add a function which takes a
lisp expression, runs test_emacs and compares the result with "t" using
test_expect_equal or similar.  This way you do not need to make an
explicit test_emacs call and avoid code duplication when checking the
result.  test_emacs_expect_t should be a good name for it.  What do you
think?

Regards,
  Dmitry

> ---
> 
> For use in the following set of tests and later for other emacs related tests.
> 
>  test/test-lib.sh |   20 ++++++++++++++++++++
>  1 files changed, 20 insertions(+), 0 deletions(-)
> 
> diff --git a/test/test-lib.sh b/test/test-lib.sh
> index d1fbc05..0ffe9a6 100644
> --- a/test/test-lib.sh
> +++ b/test/test-lib.sh
> @@ -503,6 +503,26 @@ test_expect_equal_file ()
>      fi
>  }
>  
> +# Pass test if the result is `t', else report the results.
> +test_expect_emacs_t ()
> +{
> +	exec 1>&6 2>&7		# Restore stdout and stderr
> +	inside_subtest=
> +	test "$#" = 1 ||
> +	error "bug in the test script: not 1 parameter to test_expect_emacs_t"
> +
> +	result="$1"
> +	if ! test_skip "$test_subtest_name"
> +	then
> +		if [ "${result}" == "t" ] ; then
> +			test_ok_ "$test_subtest_name"
> +		else
> +			testname=$this_test.$test_count
> +			test_failure_ "$test_subtest_name" "$(eval printf ${result})"
> +		fi
> +    fi
> +}
> +
>  NOTMUCH_NEW ()
>  {
>      notmuch new | grep -v -E -e '^Processed [0-9]*( total)? file|Found [0-9]* total file'
> -- 
> 1.7.7.3
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 2/2] test: Add tests for `notmuch-show-test-clean-address'.

[David Edmondson]

[-- Attachment #1: Type: text/plain, Size: 1096 bytes --]

On Mon, 16 Jan 2012 21:41:09 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> On Mon, 16 Jan 2012 16:47:33 +0000, David Edmondson <dme@dme.org> wrote:
> > ---
> > 
> > Add three tests. The third one currently fails.
> > 
> 
> I do not like that we add testing code to emacs/ folder.  My
> understanding is that files in emacs/ are all intended to be installed
> on the system.  But tests should not be.  Can we move this file to
> tests/?
> 
> Also, one file for all tests is a mess.  Let's write more smaller files
> that test different features instead of putting everything to one
> bloated file.  Like we do for other tests now.
> 
> I like the idea of putting elisp testing code into .el files and calling
> functions from regular tests (that is why I added test-lib.el).  For
> tests implemented in elisp, I propose adding a pair of files, e.g.:
> 
>   tests/emacs-address.sh - tests that call elisp functions
>   tests/emacs-address.el - elisp code that actually implements the tests
> 
> What do you think?

All good suggestions - I'll re-whack it.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 1/2] test: Add "test_expect_emacs_t" to assist with emacs testing.

[David Edmondson]

[-- Attachment #1: Type: text/plain, Size: 901 bytes --]

On Mon, 16 Jan 2012 21:50:22 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> Isn't this function the same as something like "test_expect_equal $x
> t"?

Yes, except that it differs in how it reports differences.

> IMO the function seems too complex for what it does (and basically that
> is "compare $x with t").  Why do we need to implement it in such a way?
> 
> Also, the function has nothing to do with emacs (except for the fact
> that 't' is commonly used in it).
> 
> I think a more useful approach would be to add a function which takes a
> lisp expression, runs test_emacs and compares the result with "t" using
> test_expect_equal or similar.  This way you do not need to make an
> explicit test_emacs call and avoid code duplication when checking the
> result.  test_emacs_expect_t should be a good name for it.  What do you
> think?

Agreed. I'll do it.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

[PATCH 2/2] emacs: Another special case for `notmuch-show-clean-address'.

[David Edmondson]

Remove backslashes.
---
 emacs/notmuch-show.el          |   14 +++++++++-----
 test/emacs-address-cleaning.el |    6 ++++--
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 1fd2524..c849e84 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -310,11 +310,15 @@ unchanged ADDRESS if parsing fails."
        (t
 	(setq p-address address)))
 
-      ;; Remove outer double quotes. They might be required during
-      ;; transport, but we don't need to see them.
-      (when (and p-name
-		 (string-match "^\"\\(.*\\)\"$" p-name))
-	(setq p-name (match-string 1 p-name)))
+      ;; Remove elements of the mailbox part that are not relevant for
+      ;; display, even if they are required during transport.
+      (when p-name
+	;; Outer double quotes.
+	(when (string-match "^\"\\(.*\\)\"$" p-name)
+	  (setq p-name (match-string 1 p-name)))
+
+	;; Backslashes.
+	(setq p-name (replace-regexp-in-string "\\\\" "" p-name)))
 
       ;; If the address is 'foo@bar.com <foo@bar.com>' then show just
       ;; 'foo@bar.com'.
diff --git a/test/emacs-address-cleaning.el b/test/emacs-address-cleaning.el
index 19e9e05..3b0b109 100644
--- a/test/emacs-address-cleaning.el
+++ b/test/emacs-address-cleaning.el
@@ -20,10 +20,12 @@
   (let* ((input '("ДБ <db-uknot@stop.me.uk>"
 		  "foo (at home) <foo@bar.com>"
 		  "foo [at home] <foo@bar.com>"
-		  "Foo Bar"))
+		  "Foo Bar"
+		  "Fred Dibna \\[extraordinaire\\] <fred@dibna.com>"))
 	 (expected '("ДБ <db-uknot@stop.me.uk>"
 		     "foo (at home) <foo@bar.com>"
 		     "foo [at home] <foo@bar.com>"
-		     "Foo Bar"))
+		     "Foo Bar"
+		     "Fred Dibna [extraordinaire] <fred@dibna.com>"))
 	 (output (mapcar #'notmuch-show-clean-address input)))
     (notmuch-test-expect-equal output expected)))
-- 
1.7.8.3