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

[Alex <agrambot@gmail.com>]

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

npostavs@users.sourceforge.net writes:

> Alex <agrambot@gmail.com> writes:
>
>> The segfault appears to have been because I didn't wipe out the elc
>> files when testing different implementations.
>
> I suspect getting a segfault might indicate an actual bug somewhere.

Probably, but I can't reproduce it anymore since I got rid of the
offending .elc files.

>> I spent a lot longer than I'd like to admit finding this out. Is there a
>> reason why "make clean" in the test directory doesn't wipe out elc
>> files? I don't understand why there's a separate bootstrap-clean that
>> does this. Can this and TEST_LOAD_EL please be documented in the test
>> README?
>
> I think it was basically copied from the other Makefiles, where cleaning
> all elc files would mean a very long subsequent compilation.  It might
> make sense to break the pattern for the test/ subdirectory though.

That makes sense to me, but if others disagree then the behaviour being
documented is the next best thing.

>> Anyway, I got everything back in order. Sadly, there's a couple extra
>> tests that now fail for me in the patch that *doesn't* expand inline
>> functions, and these don't fail for me in a clean master. They are in
>> eieio-tests (23 and 24).
>
> I'm seeing eieio-tests failing also in master.  This seems to be an
> actual bug, in the definition of `cl-typep' I think.  I've opened a new
> bug for this (Bug#27718).

Oddly enough, I can't reproduce this on master. I cloned a new copy, ran
"./autogen.sh && ./configure && make -j4", then ran "make eieio-tests
TEST_LOAD_EL=no" with no error. I cloned from 30444c695, then tried
again with 7a0fb008. I also tried "make check-expensive TEST_LOAD_EL=no"
and got only 2 errors (dom-tests and cl-lib-tests).

Perhaps odder is that I can still reproduce your recipe in Bug#27718 in
that same repository.

>> With the inline expansion, I also get some errors in ert-tests. All of
>> the errors, with the exception of subr-tests error, seem to be from
>> cl-defstruct and cl-typep (which is defined by define-inline).
>>
>> Do you have any ideas? There should be 5 unexpected errors without the
>> inline expansion, and 6 errors with it. Note that all tests pass in both
>> cases without "TEST_LOAD_EL=no".
>>
>> If it's easy to fix the eieio tests and not the other ones, then it
>> might be better to leave the inline-function expansion out for now.
>
> I have a fix for the subr-tests failed, as for the others, I don't know
> enough about the compilation process to untangle it yet.  I think we
> should just leave the inline-function expansion part out for now, at
> which point I believe your patch won't be making anything worse, so it
> should be okay to install.

Sounds good. It would be lucky if a fix to Bug#27718 also resolves the
other inline function errors so that we could use the previous patch.
Here's an updated patch with inline function excluded:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Catch-argument-and-macroexpansion-errors-in-ert.patch --]
[-- Type: text/x-diff, Size: 5700 bytes --]

From 05f0734dd0b066058d263756a2e98cafc4ef8515 Mon Sep 17 00:00:00 2001
From: Alexander Gramiak <agrambot@gmail.com>
Date: Thu, 13 Jul 2017 14:54:35 -0600
Subject: [PATCH] Catch argument and macroexpansion errors in ert

This kludge catches errors caused by evaluating arguments in ert's
should, should-not, and should-error macros; it also catches
macroexpansion errors inside of the above macros (Bug#24402).

* lisp/emacs-lisp/ert.el: (ert--should-signal-hook): New function.
(ert--expand-should-1): Catch macroexpansion errors.
* test/lisp/emacs-lisp/ert-tests.el (ert-test-should-error-argument)
(ert-test-should-error-macroexpansion): Tests for argument and
expansion errors.
---
 lisp/emacs-lisp/ert.el            | 41 ++++++++++++++++++++++++++++++---------
 test/lisp/emacs-lisp/ert-tests.el |  9 +++++++++
 2 files changed, 41 insertions(+), 9 deletions(-)

diff --git a/lisp/emacs-lisp/ert.el b/lisp/emacs-lisp/ert.el
index eb2b2e3e11..caa4a9f389 100644
--- a/lisp/emacs-lisp/ert.el
+++ b/lisp/emacs-lisp/ert.el
@@ -266,6 +266,14 @@ DATA is displayed to the user and should state the reason for skipping."
   (when ert--should-execution-observer
     (funcall ert--should-execution-observer form-description)))
 
+;; See Bug#24402 for why this exists
+(defun ert--should-signal-hook (error-symbol data)
+  "Stupid hack to stop `condition-case' from catching ert signals.
+It should only be stopped when ran from inside ert--run-test-internal."
+  (when (and (not (symbolp debugger))   ; only run on anonymous debugger
+             (memq error-symbol '(ert-test-failed ert-test-skipped)))
+    (funcall debugger 'error data)))
+
 (defun ert--special-operator-p (thing)
   "Return non-nil if THING is a symbol naming a special operator."
   (and (symbolp thing)
@@ -273,16 +281,22 @@ DATA is displayed to the user and should state the reason for skipping."
          (and (subrp definition)
               (eql (cdr (subr-arity definition)) 'unevalled)))))
 
+;; FIXME: Code inside of here should probably be evaluated like it is
+;; outside of tests, with the sole exception of error handling
 (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))))))
+         ;; catch macroexpansion errors
+         (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 +317,13 @@ DATA is displayed to the user and should state the reason for skipping."
               (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
+                             (let ((signal-hook-function #'ert--should-signal-hook))
+                               (list ,@arg-forms))
+                           (error (progn (setq ,fn #'signal)
+                                         (list (car err)
+                                               (cdr err)))))))
              (let ((,value ',default-value))
                ,(funcall inner-expander
                          `(setq ,value (apply ,fn ,args))
@@ -766,6 +785,10 @@ This mainly sets up debugger-related bindings."
     ;; too expensive, we can remove it.
     (with-temp-buffer
       (save-window-excursion
+        ;; FIXME: Use `signal-hook-function' instead of `debugger' to
+        ;; handle ert errors. Once that's done, remove
+        ;; `ert--should-signal-hook'. See Bug#24402 and Bug#11218 for
+        ;; details.
         (let ((debugger (lambda (&rest args)
                           (ert--run-test-debugger test-execution-info
                                                   args)))
diff --git a/test/lisp/emacs-lisp/ert-tests.el b/test/lisp/emacs-lisp/ert-tests.el
index 317838b250..b6a7eb68da 100644
--- a/test/lisp/emacs-lisp/ert-tests.el
+++ b/test/lisp/emacs-lisp/ert-tests.el
@@ -294,6 +294,15 @@ failed or if there was a problem."
                   "the error signaled was a subtype of the expected type")))))
     ))
 
+(ert-deftest ert-test-should-error-argument ()
+  "Errors due to evaluating arguments should not break tests."
+  (should-error (identity (/ 1 0))))
+
+(ert-deftest ert-test-should-error-macroexpansion ()
+  "Errors due to expanding macros should not break tests."
+  (cl-macrolet ((test () (error "Foo")))
+    (should-error (test))))
+
 (ert-deftest ert-test-skip-unless ()
   ;; Don't skip.
   (let ((test (make-ert-test :body (lambda () (skip-unless t)))))
-- 
2.13.2