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

[Alex]

tags 24402 patch
quit

Alex <agrambot@gmail.com> writes:

> Gemini Lasswell <gazally@runbox.com> writes:
>
>> I’ve done some investigating of why this is happening. testcover-start
>> transforms:
>>     (should-error (my-error))
>> into:
>>     (should-error (testcover-after 2 (my-error)))
>>
>> Then the macro expansion of should-error separates the form which it
>> is passed into a function symbol and list of arguments, and applies
>> the function to the arguments inside of a condition-case that traps
>> errors. The problem is that the arguments are evaluated first, outside
>> of the condition-case, so errors in their evaluation do not get
>> caught. This problem is not specific to testcover. In this example,
>> the first test passes and the second fails:
>>
>> (defun div-by (n)
>>   (/ 100 n))
>>
>> (defmacro log-div-by (n)
>>   `(message "div-by: %d" (div-by ,n)))
>>
>> (ert-deftest test-div-by ()
>>   (should-error (div-by 0)))
>>
>> (ert-deftest test-log-div-by ()
>>   (should-error (log-div-by 0)))
>>   
>
> I just ran into this as well. Consider these two forms:
>
> (should-error (cl-fourth "1234") :type 'wrong-type-argument)
>
> (should-error (car (cdr (cdr (cdr "1234")))) :type 'wrong-type-argument)
>
> Only the second raises an error, even though cl-fourth is equivalent to
> the car/cdr chain.

Here's a diff that appears to fix this. It's a bit crude, but I'd rather
not mess around too much with ert's internals.

ert-args.diff
Description: first

There's a similar issue with macro-expansions inside of
should/should-error/should-not that could/should be fixed by wrapping
the macroexpand call at the top of ert--expand-should-1 in a similar
condition-case. Here's another diff that does just that:

ert-args2.diff
Description: second

I ran "make check" and found only one test that the above diff breaks:
ert-test-test-result-expected-p.

I can't seem to figure out why it doesn't work. The test fails because
of these two:

(let ((test (make-ert-test :body (lambda () (ert-fail "failed")))))
  (should-not (ert-test-result-expected-p test (ert-run-test test))))
(let ((test (make-ert-test :body (lambda () (ert-fail "failed"))
                           :expected-result-type ':failed)))
  (should (ert-test-result-expected-p test (ert-run-test test))))

I tried to re-throw the ert-test-failed signal and still the above two
forms raise error an error.