[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#24402: should-error doesn't catch all errors
From: |
Alex |
Subject: |
bug#24402: should-error doesn't catch all errors |
Date: |
Wed, 05 Jul 2017 13:56:48 -0600 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.0.50 (gnu/linux) |
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.
- bug#24402: should-error doesn't catch all errors (Was:bug#24402: More Information), Alex, 2017/07/03
- bug#24402: should-error doesn't catch all errors, Alex, 2017/07/04
- bug#24402: should-error doesn't catch all errors, Tino Calancha, 2017/07/05
- bug#24402: should-error doesn't catch all errors, npostavs, 2017/07/11
- bug#24402: should-error doesn't catch all errors, Alex, 2017/07/11
- bug#24402: should-error doesn't catch all errors, npostavs, 2017/07/12
- bug#24402: should-error doesn't catch all errors, Alex, 2017/07/12
- bug#24402: should-error doesn't catch all errors, npostavs, 2017/07/12
- bug#24402: should-error doesn't catch all errors, Alex, 2017/07/12
- bug#24402: should-error doesn't catch all errors, npostavs, 2017/07/12
- bug#24402: should-error doesn't catch all errors, Alex, 2017/07/13