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

[Alex <agrambot@gmail.com>]

Tino Calancha <tino.calancha@gmail.com> writes:

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

Thanks, that helps with my understanding of the issue. Sadly, I don't
think your tweak will work in all cases, namely whenever (list
,@arg-forms) has side-effects. Luckily this doesn't happen in any
current tests, but I think those types of tests should be supported.

I believe the following is why my previous diff doesn't work. Consider:

(let ((test (make-ert-test :body (lambda () (ert-fail "failed")))))
  (ert-run-test test))

The above returns a struct/record and does not error.

(let ((test (make-ert-test :body (lambda () (ert-fail "failed")))))
  (condition-case err
      (ert-run-test test)
    (error "woops")))

Even though ert-run-test by itself does not error, the error handler is
ran. I believe this is because `ert--run-test-internal' binds `debugger'
around the evaluation of the test to somehow suppress this error.

Using condition-case-unless-debug gets around this, but now anytime
debug-on-error is non-nil the condition-case won't catch the error. I'm
not sure how to solve this.