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

[Alex <agrambot@gmail.com>]

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

npostavs@users.sourceforge.net writes:

> Alex <agrambot@gmail.com> writes:
>
>> npostavs@users.sourceforge.net writes:
>>
>>> Does it also work when loading the elc version of the test file?  (try
>>> 'make check TEST_LOAD_EL=no')
>>
>> Oh, it doesn't load the elc version by default? That's surprising; I
>> think that should be documented in the test README.
>>
>> I get 3 test failures with TEST_LOAD_EL=no, but I don't believe they're
>> because of me. On a mostly clean master (d014a5e15) those 3 also error.
>> One of them is simple to fix (the (require 'subr-x) should not be inside
>> eval-when-compile in dom-tests).
>
> Ah, the `should' blocks inlining during compilation.  Is that necessary?
> Probably yes if we expect to catch errors during macroexpansion I guess.

Can you get errors by expanding inlined functions?

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?



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

diff --git a/lisp/emacs-lisp/ert.el b/lisp/emacs-lisp/ert.el
index beb32c48ce..f4d2f725ac 100644
--- a/lisp/emacs-lisp/ert.el
+++ b/lisp/emacs-lisp/ert.el
@@ -281,17 +281,19 @@ ert--special-operator-p
          (and (subrp definition)
               (eql (cdr (subr-arity definition)) 'unevalled)))))
 
+(require 'byte-opt)
+
 (defun ert--expand-should-1 (whole form inner-expander)
   "Helper function for the `should' macro and its variants."
   (let ((form
          (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))))
+             (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)))
@@ -308,7 +310,8 @@ 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)))
+                       (byte-code-function-p fn-name)))
         (let ((fn (cl-gensym "fn-"))
               (args (cl-gensym "args-"))
               (value (cl-gensym "value-"))

[-- Attachment #3: Type: text/plain, Size: 1607 bytes --]


>> The other failing tests are
>> subr-test-backtrace-integration-test and cl-lib-defstruct-record.
>
> Hmm, I'll see if I can fix these.

Thanks. I noticed when byte-compiling cl-lib-tests, I got this warning:
Unused lexical variable ‘cl-struct-foo’.

>>> What about tests like this?
>>>
>>>     (ert-deftest check-error-handling ()
>>>       (should
>>>        (eq 42
>>>            (condition-case ()
>>>                (/ 1 0)
>>>              (arith-error 42)))))
>>
>> It works for me, yes. As long as `debugger' is set to a symbol. I can
>> make it a bit more robust by using an additional defvar in
>> ert--run-test-internal.
>>
>> Are you asking because it doesn't work for you?
>
> No, I'm just trying to explore the edges of this solution.  Isn't
> `debugger' bound to a non-symbol while running the the tests?  I'm
> confused as to why this solution works.

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}.

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.