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

[Alex <agrambot@gmail.com>]

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

npostavs@users.sourceforge.net writes:

> Alex <agrambot@gmail.com> writes:
>
>> Macros are expanded at compile-time with the current patch. If there are
>> any macroexpansion errors, then the form is altered to be (error <type>
>> <data>). Perhaps inline functions could work similarly.
>>
>> Here's a diff to my patch that uses byte-compile-inline-expand. This
>> fixes the dom-tests case. Do you think it's worth it?
>
> It would be nice if we can make code inside tests behave the same as
> outside.  But we should make it conditional on whether the code is being
> compiled, otherwise code inside tests would behave differently when
> being interpreted.  Anyway, we can leave this for a separate bug.

I agree, but that sounds like it'll require a fair bit of refactoring
and knowledge of ert internals.

OOC, is there a robust way to check whether or not you're currently
byte-compiling?

>> Yes, that's why there's the second test that checks for error-symbol to
>> be ert-test-{failed, skipped}. Basically what's happening is that
>> ert--signal-hook forces the debugger to trigger even inside a
>> condition-case, but only with a non-symbol `debugger' (since
>> ert--run-test-internal binds it to a closure), and one of the above two
>> errors.
>>
>> The only time this approach fails is when you bind `debugger' to a
>> non-symbol and also signal ert-test-{failed, skipped}.
>
> Okay, it sounds like we would only hit problems when running an ert test
> from inside an ert test.  That should be pretty rare outside of ert's
> test suite, and we already have workarounds for that case, right?

I tried a nested case using two ert-deftests and it worked, so it looks
okay here too.

>> This is relatively rare compared to the problems at hand (macro and
>> argument errors), so unless there are unforeseen issues I think it's an
>> acceptable stop-gap solution. Hopefully Someone™ can properly fix this
>> eventually.
>
> Yes, I think this approach is acceptable.

I was going to ask if you would merge in a few days, but it appears that
what should have been a simple rebase to master caused unforeseen
consequences. For instance, for some reason I now get a segmentation
fault when executing 'make cl-lib-tests TEST_LOAD_EL=no'. I even reset
to the commit I was at before and it still segfaults. Can you reproduce
this with the following patch on master?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: ert --]
[-- Type: text/x-diff, Size: 6080 bytes --]

From fdbf66f73672d9b69fc37273223ae90fff951eb2 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 expansion errors in ert

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

* lisp/emacs-lisp/ert.el: (ert--should-signal-hook): New function.
(ert--expand-should-1): Catch expansion 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            | 47 ++++++++++++++++++++++++++++++---------
 test/lisp/emacs-lisp/ert-tests.el |  9 ++++++++
 2 files changed, 46 insertions(+), 10 deletions(-)

diff --git a/lisp/emacs-lisp/ert.el b/lisp/emacs-lisp/ert.el
index eb2b2e3e11..c6e3ee8503 100644
--- a/lisp/emacs-lisp/ert.el
+++ b/lisp/emacs-lisp/ert.el
@@ -65,6 +65,7 @@
 (require 'find-func)
 (require 'help)
 (require 'pp)
+(require 'byte-opt) ; for ert--expand-should-1 kludge
 
 ;;; UI customization options.
 
@@ -266,6 +267,14 @@ ert--signal-should-execution
   (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)
@@ -276,13 +285,20 @@ 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))))))
+         ;; catch all macro and inline function expansion errors
+         ;; FIXME: Code inside of tests should be evaluated like it is
+         ;; outside of tests, with the sole exception of error
+         ;; handling
+         (condition-case err
+             (byte-compile-inline-expand
+              (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-")))
@@ -298,13 +314,20 @@ ert--expand-should-1
         (cl-assert (or (symbolp fn-name)
                        (and (consp fn-name)
                             (eql (car fn-name) 'lambda)
-                            (listp (cdr fn-name)))))
+                            (listp (cdr fn-name)))
+                       ;; for inline functions
+                       (byte-code-function-p fn-name)))
         (let ((fn (cl-gensym "fn-"))
               (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 +789,10 @@ ert--run-test-internal
     ;; 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 @@ ert-self-test-and-exit
                   "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