bug#24402: should-error doesn't catch all errors

[Tino Calancha]

Alex <agrambot@gmail.com> writes:

> I ran "make check" and found only one test that the above diff breaks:
> ert-test-test-result-expected-p.
>
> I can't seem to figure out why it doesn't work. The test fails because
> of these two:
>
> (let ((test (make-ert-test :body (lambda () (ert-fail "failed")))))
>   (should-not (ert-test-result-expected-p test (ert-run-test test))))
> (let ((test (make-ert-test :body (lambda () (ert-fail "failed"))
>                            :expected-result-type ':failed)))
>   (should (ert-test-result-expected-p test (ert-run-test test))))
>
> I tried to re-throw the ert-test-failed signal and still the above two
> forms raise error an error.

Hi!
I just arrived from teletransportation from Bug#27559.  Very fast! (and
cheap!).

Thank you for looking on this.  I think you go in the right direction to
fix this problem.

* I have updated your patch and all the test suite pass (even your
  proposed tests in Bug#27559 without requiring "(eval '....)").

* Bear in mind that I am far to be an expert on `ert.el', and i am
  already in my second beer, so please double check that
  the patch have sense.
  
--8<-----------------------------cut here---------------start------------->8---
commit a07f99f062f3da3418060ef30e1a00030fa0b947
Author: Tino Calancha <tino.calancha@gmail.com>
Date:   Wed Jul 5 22:11:46 2017 +0900

    Tweak Alex's 2nd patch

diff --git a/lisp/emacs-lisp/ert.el b/lisp/emacs-lisp/ert.el
index eb2b2e3e11..2d131cf99e 100644
--- a/lisp/emacs-lisp/ert.el
+++ b/lisp/emacs-lisp/ert.el
@@ -276,13 +276,15 @@ ert--special-operator-p
 (defun ert--expand-should-1 (whole form inner-expander)
   "Helper function for the `should' macro and its variants."
   (let ((form
-         (macroexpand form (append (bound-and-true-p
-                                    byte-compile-macro-environment)
-                                   (cond
-                                    ((boundp 'macroexpand-all-environment)
-                                     macroexpand-all-environment)
-                                    ((boundp 'cl-macro-environment)
-                                     cl-macro-environment))))))
+         (condition-case err
+             (macroexpand-all form (append (bound-and-true-p
+                                            byte-compile-macro-environment)
+                                           (cond
+                                            ((boundp 
'macroexpand-all-environment)
+                                             macroexpand-all-environment)
+                                            ((boundp 'cl-macro-environment)
+                                             cl-macro-environment))))
+           (error `(signal ',(car err) ',(cdr err))))))
     (cond
      ((or (atom form) (ert--special-operator-p (car form)))
       (let ((value (cl-gensym "value-")))
@@ -303,8 +305,14 @@ ert--expand-should-1
               (args (cl-gensym "args-"))
               (value (cl-gensym "value-"))
               (default-value (cl-gensym "ert-form-evaluation-aborted-")))
-          `(let ((,fn (function ,fn-name))
-                 (,args (list ,@arg-forms)))
+          `(let* ((,fn (function ,fn-name))
+                  (,args (condition-case err
+                             (list ,@arg-forms)
+                           (error (if (or (eq (car err) 'ert-test-failed)
+                                          (eq (car err) 'ert-test-skipped))
+                                      (list ,@arg-forms)
+                                    (setq ,fn #'signal)
+                                    (list (car err) (cdr err)))))))
              (let ((,value ',default-value))
                ,(funcall inner-expander
                          `(setq ,value (apply ,fn ,args))
--8<-----------------------------cut here---------------end--------------->8---
In GNU Emacs 26.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.22.11)
 of 2017-07-05
Repository revision: 5d62247323f53f3ae9c7d9f51e951635887b2fb6