[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: npostavs
Subject: bug#24402: should-error doesn't catch all errors
Date: Wed, 12 Jul 2017 23:44:19 -0400
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/25.2.50 (gnu/linux)

Alex <address@hidden> 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.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]