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

[Tino Calancha]

Alex <agrambot@gmail.com> writes:

> 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.
Good point.

> 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.
I've being thinking about this, and i cannot find something better than
your second patch.

My suggestion is:

1. We apply your 2nd patch.
2. We ammend the failing tests wrapping '> 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.
Good point.

> 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.
I've being thinking about this, and i cannot find something better than
your second patch.

My suggestion is:

1. We apply your 2nd patch.
2. We handle the failing tests wrapping '(ert-fail "failed")' into
  `ignore-errors' as in below patch.

Then, IMO we are in a better situation than we are know:
That fix this bug, and if i am nt missing something, at a low price: just
tweaking a bit 2 innocents tests.

What do you think?


--8<-----------------------------cut here---------------start------------->8---

diff --git a/test/lisp/emacs-lisp/ert-tests.el 
b/test/lisp/emacs-lisp/ert-tests.el
index 317838b250..f3e4db192a 100644
--- a/test/lisp/emacs-lisp/ert-tests.el
+++ b/test/lisp/emacs-lisp/ert-tests.el
@@ -413,12 +413,14 @@ ert-test--which-file
   (let ((test (make-ert-test :body (lambda ()))))
     (should (ert-test-result-expected-p test (ert-run-test test))))
   ;; unexpected failure
-  (let ((test (make-ert-test :body (lambda () (ert-fail "failed")))))
-    (should-not (ert-test-result-expected-p test (ert-run-test test))))
-  ;; expected failure
-  (let ((test (make-ert-test :body (lambda () (ert-fail "failed"))
-                             :expected-result-type ':failed)))
+  (let ((test (make-ert-test
+               :body (lambda () (ignore-errors (ert-fail "failed"))))))
     (should (ert-test-result-expected-p test (ert-run-test test))))
+  ;; expected failure
+  (let ((test (make-ert-test
+               :body (lambda () (ignore-errors (ert-fail "failed")))
+               :expected-result-type ':failed)))
+    (should-not (ert-test-result-expected-p test (ert-run-test test))))
   ;; `not' expected type
   (let ((test (make-ert-test :body (lambda ())
                              :expected-result-type '(not :failed))))
--8<-----------------------------cut here---------------end--------------->8---