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

[npostavs]

Alex <agrambot@gmail.com> writes:

>>> I get 3 test failures with TEST_LOAD_EL=no, but I don't believe they're
>>> because of me. On a mostly clean master (d014a5e15) those 3 also error.
>>> One of them is simple to fix (the (require 'subr-x) should not be inside
>>> eval-when-compile in dom-tests).
>>
>> Ah, the `should' blocks inlining during compilation.  Is that necessary?
>> Probably yes if we expect to catch errors during macroexpansion I guess.
>
> Can you get errors by expanding inlined functions?

Yes I think so, probably not from defsubst (except for wrong number of
arguments maybe?), but I believe define-inline basically lets you run an
arbitrary macro to produce the inlined call.

> Macros are expanded at compile-time with the current patch. If there are
> any macroexpansion errors, then the form is altered to be (error <type>
> <data>). Perhaps inline functions could work similarly.
>
> Here's a diff to my patch that uses byte-compile-inline-expand. This
> fixes the dom-tests case. Do you think it's worth it?

It would be nice if we can make code inside tests behave the same as
outside.  But we should make it conditional on whether the code is being
compiled, otherwise code inside tests would behave differently when
being interpreted.  Anyway, we can leave this for a separate bug.

> Yes, that's why there's the second test that checks for error-symbol to
> be ert-test-{failed, skipped}. Basically what's happening is that
> ert--signal-hook forces the debugger to trigger even inside a
> condition-case, but only with a non-symbol `debugger' (since
> ert--run-test-internal binds it to a closure), and one of the above two
> errors.
>
> The only time this approach fails is when you bind `debugger' to a
> non-symbol and also signal ert-test-{failed, skipped}.

Okay, it sounds like we would only hit problems when running an ert test
from inside an ert test.  That should be pretty rare outside of ert's
test suite, and we already have workarounds for that case, right?

> This is relatively rare compared to the problems at hand (macro and
> argument errors), so unless there are unforeseen issues I think it's an
> acceptable stop-gap solution. Hopefully Someone™ can properly fix this
> eventually.

Yes, I think this approach is acceptable.