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

[Alex <agrambot@gmail.com>]

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

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

I agree that it's a low price, and that it fixes more than it breaks,
but it does involve switching the logic in a simple basic test.

Also note that ignore-errors expands into a condition-case, so instead
of changing the tests, the patch could be tweaked to return nil if the
test signals ert-test-{failed, skipped}. Or the two tests could use
condition-case directly and make sure that ert-test-failed is/isn't
signalled.

I'd like to get some more opinions on this bug, in the hopes that
there's a better solution. Eli/Noam (or anyone happening upon this), do
you have any ideas on how to solve this?