bug#11218: with-demoted-errors use of condition-case-unless-debug; ert

bug#11218: with-demoted-errors use of condition-case-unless-debug; ert

[Glenn Morris]

Package: emacs
Version: 24.0.95

with-demoted-errors use of condition-case-unless-debug is annoying.
If I want to debug a condition-case, I'll set debug-on-signal non-nil.
As it stands, with-demoted-errors is not useful if you want to write an
ERT test (for reasons I haven't investigated).

(defun foo ()
  (with-demoted-errors (error "error!"))
  t)

(ert-deftest test-foo ()
  "Test foo"
  (should (foo)))

M-x ert RET test-foo RET

Selector: test-foo
Passed: 0
Failed: 1 (1 unexpected)
Total:  1/1


If you replace with-demoted-errors with ignore-errors, the test passes.

bug#11218: with-demoted-errors use of condition-case-unless-debug; ert

[Lars Magne Ingebrigtsen]

Glenn Morris <rgm@gnu.org> writes:

> with-demoted-errors use of condition-case-unless-debug is annoying.
> If I want to debug a condition-case, I'll set debug-on-signal non-nil.
> As it stands, with-demoted-errors is not useful if you want to write an
> ERT test (for reasons I haven't investigated).

Isn't this an ERT bug, then?

-- 
(domestic pets only, the antidote for overdose, milk.)
  bloggy blog http://lars.ingebrigtsen.no/

bug#11218: with-demoted-errors use of condition-case-unless-debug; ert

[Stefan Monnier]

> (defun foo ()
>   (with-demoted-errors (error "error!"))
>   t)
>
> (ert-deftest test-foo ()
>   "Test foo"
>   (should (foo)))
>
> M-x ert RET test-foo RET
>
> Selector: test-foo
> Passed: 0
> Failed: 1 (1 unexpected)
> Total:  1/1
>
> If you replace with-demoted-errors with ignore-errors, the test passes.

Looks like a bug in ERT.


        Stefan

bug#11218: with-demoted-errors use of condition-case-unless-debug; ert

[Glenn Morris]

>> If I want to debug a condition-case, I'll set debug-on-signal non-nil.

That's not an ert bug. :)

bug#11218: with-demoted-errors use of condition-case-unless-debug; ert

[Glenn Morris]

Glenn Morris wrote:

>>> If I want to debug a condition-case, I'll set debug-on-signal non-nil.
>
> That's not an ert bug. :)

To expand: why does with-demoted-errors use a different form of
condition-case, and hence treat debugging differently, to the
pre-existing ignore-errors? The former doesn't let you have control; the
latter does.

bug#11218: with-demoted-errors use of condition-case-unless-debug; ert

[Stefan Monnier]

>>>> If I want to debug a condition-case, I'll set debug-on-signal non-nil.
>> That's not an ert bug. :)
> To expand: why does with-demoted-errors use a different form of
> condition-case, and hence treat debugging differently, to the
> pre-existing ignore-errors? The former doesn't let you have control; the
> latter does.

debug-on-signal is inconvenient because it triggers in many cases where
it shouldn't (because the error is actually normal and handled
properly).

`with-demoted-errors' doesn't actually *handle* the error, it just
demotes it so as not to annoy the user.  So I find it makes sense to get
into the debugger when an error is demoted and the user has asked to
enter debugger on error.


        Stefan

bug#11218: with-demoted-errors use of condition-case-unless-debug; ert

[Glenn Morris]

Stefan Monnier wrote:

>> If you replace with-demoted-errors with ignore-errors, the test passes.
>
> Looks like a bug in ERT.

I don't know if it's a "bug" per se...

ert--run-test-internal binds debug-on-error to t, and redefines the
debugger to ert--run-test-debugger. As the doc of that function says:

  This function records failures and errors and either terminates the
  test silently or calls the interactive debugger, as appropriate.

(Whether or not the "real" debugger is invoked depends on the value of
ert-debug-on-error.)

So basically AFAICS it's an ert design choice that it works by borrowing
the debugger. I don't imagine it would be easy to change that.

bug#11218: with-demoted-errors use of condition-case-unless-debug; ert

[Stefan Monnier]

>>> If you replace with-demoted-errors with ignore-errors, the test passes.
>> Looks like a bug in ERT.
> I don't know if it's a "bug" per se...
> ert--run-test-internal binds debug-on-error to t, and redefines the
> debugger to ert--run-test-debugger. As the doc of that function says:

I wonder why ERT doesn't just use condition-case to catch and record
the errors.


        Stefan

bug#11218: with-demoted-errors use of condition-case-unless-debug; ert

[Christian Ohler]

On 4/13/12 6:44 AM, Stefan Monnier wrote:
>>>> If you replace with-demoted-errors with ignore-errors, the test passes.
>>> Looks like a bug in ERT.
>> I don't know if it's a "bug" per se...
>> ert--run-test-internal binds debug-on-error to t, and redefines the
>> debugger to ert--run-test-debugger. As the doc of that function says:
>
> I wonder why ERT doesn't just use condition-case to catch and record
> the errors.

Two reasons that I remember off the top of my head: Recording 
backtraces, and recording additional information provided with 
`ert-info'.  Using condition-case would let ERT regain control only 
after the stack is unwound, at which point that information is gone.

Christian.

bug#11218: with-demoted-errors use of condition-case-unless-debug; ert

[Stefan Monnier]

>>>>> If you replace with-demoted-errors with ignore-errors, the test passes.
>>>> Looks like a bug in ERT.
>>> I don't know if it's a "bug" per se...
>>> ert--run-test-internal binds debug-on-error to t, and redefines the
>>> debugger to ert--run-test-debugger. As the doc of that function says:
>> I wonder why ERT doesn't just use condition-case to catch and record
>> the errors.
> Two reasons that I remember off the top of my head: Recording backtraces,
> and recording additional information provided with `ert-info'.

So the next question is: why does ERT record a backtrace and that extra
information provided by ert-info?


        Stefan

bug#11218: with-demoted-errors use of condition-case-unless-debug; ert

[Christian Ohler]

Stefan Monnier, 2012-04-19:

>>>>>> If you replace with-demoted-errors with ignore-errors, the test passes.
>>>>> Looks like a bug in ERT.
>>>> I don't know if it's a "bug" per se...
>>>> ert--run-test-internal binds debug-on-error to t, and redefines the
>>>> debugger to ert--run-test-debugger. As the doc of that function says:
>>> I wonder why ERT doesn't just use condition-case to catch and record
>>> the errors.
>> Two reasons that I remember off the top of my head: Recording backtraces,
>> and recording additional information provided with `ert-info'.
>
> So the next question is: why does ERT record a backtrace and that extra
> information provided by ert-info?

To be able to show them to the user, together with the test failure.  If 
you have one or more failing tests, it's much more useful to see the 
error messages and backtraces rather than just the error messages.

Unless there's another way to accomplish this, I think it makes sense 
for ERT to bind `debugger'.  Since `debugger' is only called if 
`debug-on-error' is set, ERT has to bind that, too.

I'm not sure where that leaves us.  You could say the problem is that 
`debug-on-error' is overloaded with two different meanings (one in 
eval.c, one in `condition-case-no-debug'), or you could say that ERT 
wants a different `debugger' hook that does not depend on `debug-on-errors'.

But perhaps `with-demoted-errors' and its interaction with 
`debug-on-error' is inherently problematic -- how would we like it to 
interact with tests and debugging?  Assuming ERT did not rebind 
`debug-on-error', the test

   (ert-deftest foo ()
     (with-demoted-errors (error "a"))
     (error "b"))

would normally fail with "b", but if we enable debugging to track down 
why, it will fail with "a" instead (with no way to continue execution to 
get to the error we are interested in).  Confusing.  As far as ERT tests 
are concerned, one could argue that the current behavior (always fail 
with "a") is preferable because it's more consistent; it just means that 
errors in tests can't be demoted.

Fundamentally, having conditionals in the code that explicitly check 
`debug-on-error' seems like asking for trouble.  If the program checks 
"am I being debugged" and behaves differently in that case, we should 
expect testing and debugging tools to break...

And ERT is not the only tool that triggers the problem: While 
investigating this bug, I set `debug-on-error' to nil globally, typed 
M-: (with-demoted-errors (error "foo")) RET, and was confused for a 
moment that I found myself in the debugger with error "foo" -- 
`with-demoted-errors' had no effect.  This is because M-: rebinds 
`debug-on-error' (at least by default).  Just like ERT.  Edebug does the 
same, too.

Christian.

bug#11218: with-demoted-errors use of condition-case-unless-debug; ert

[Stefan Monnier]

>> So the next question is: why does ERT record a backtrace and that extra
>> information provided by ert-info?
> To be able to show them to the user, together with the test failure.  If you
> have one or more failing tests, it's much more useful to see the error
> messages and backtraces rather than just the error messages.

I don't see the point of showing that info when running the tests.
If the user wants that info, he should be able to easily say "run this
test with debug-on-error" so he doesn't only get a backtrace but also
gets put in the debugger where he can inspect the state.

>   (ert-deftest foo ()
>     (with-demoted-errors (error "a"))
>     (error "b"))

> would normally fail with "b", but if we enable debugging to track down why,
> it will fail with "a" instead (with no way to continue execution to get to
> the error we are interested in).

No: you'll be put in the debugger for the error "a" but if you hit `c'
it will continue and re-enter the debugger for error "b".

> Fundamentally, having conditionals in the code that explicitly check
> debug-on-error' seems like asking for trouble.

I agree it's not something we should do in general, but
with-demoted-error was designed largely for the specific case where
errors should not happen (i.e. if one happens, we want to be able to
debug it easily by setting debug-on-error), but at the same time you
know very well how the program should respond in case of an unexpected
error (so the user who's not in a position to fix the bug can get his
job done).


        Stefan

bug#11218: with-demoted-errors use of condition-case-unless-debug; ert

[Christian Ohler]

On 4/20/12 5:13 AM, Stefan Monnier wrote:
>>> So the next question is: why does ERT record a backtrace and that extra
>>> information provided by ert-info?
>> To be able to show them to the user, together with the test failure.  If you
>> have one or more failing tests, it's much more useful to see the error
>> messages and backtraces rather than just the error messages.
>
> I don't see the point of showing that info when running the tests.
> If the user wants that info, he should be able to easily say "run this
> test with debug-on-error" so he doesn't only get a backtrace but also
> gets put in the debugger where he can inspect the state.

ERT's keyboard shortcut for that is "d".  But it's not a good solution if:

* the test is nondeterministic (maybe due to subprocess interaction),
* the test takes a long time to run,
* you have multiple failing tests and want to quickly compare if they 
are crashing in the same place,
* ERT is running in batch mode (e.g. make check).

It's better to collect as much data as reasonably possible when the 
error first occurs.


>>    (ert-deftest foo ()
>>      (with-demoted-errors (error "a"))
>>      (error "b"))
>
>> would normally fail with "b", but if we enable debugging to track down why,
>> it will fail with "a" instead (with no way to continue execution to get to
>> the error we are interested in).
>
> No: you'll be put in the debugger for the error "a" but if you hit `c'
> it will continue and re-enter the debugger for error "b".

My Emacs is a few months old, but that doesn't work for me.  Has it 
changed recently?

I'm using

   (defun foo ()
     (with-demoted-errors (error "a"))
     (error "b"))

and typing

   C-u <escape> C-x <escape> : ( f o o ) <return> c c

and all I find in *Messages* is

   error: "a"
   Continue...
   edebug-signal: a

No mention of "b".

If `debug-on-error' is t, `with-demoted-errors' becomes essentially 
equivalent to `progn', and (error "b") can ever be reached in

   (defun foo ()
     (progn (error "a"))
     (error "b"))

since `error' never returns (and I think that's an important guarantee 
that the debugger shouldn't violate, at least not without confirmation).

Christian.

bug#11218: with-demoted-errors use of condition-case-unless-debug; ert

[Stefan Monnier]

>> I don't see the point of showing that info when running the tests.
>> If the user wants that info, he should be able to easily say "run this
>> test with debug-on-error" so he doesn't only get a backtrace but also
>> gets put in the debugger where he can inspect the state.

> ERT's keyboard shortcut for that is "d".  But it's not a good solution if:

> * the test is nondeterministic (maybe due to subprocess interaction),
> * the test takes a long time to run,
> * you have multiple failing tests and want to quickly compare if they are
> crashing in the same place,
> * ERT is running in batch mode (e.g. make check).

> It's better to collect as much data as reasonably possible when the error
> first occurs.

None of these arguments are really specific to ERT.  It just seems like
you like to run with debug-on-error, which is fine (I do too).
But don't impose it on all ERT users.

>> No: you'll be put in the debugger for the error "a" but if you hit `c'
>> it will continue and re-enter the debugger for error "b".
> My Emacs is a few months old, but that doesn't work for me.  Has it
> changed recently?
[...]
> If `debug-on-error' is t, `with-demoted-errors' becomes essentially
> equivalent to `progn', and (error "b") can ever be reached in

You're right, sorry: brain malfunction.
But that just means you first have to fix the bug that signals "a" before
getting to the bug that signals "b".


        Stefan

bug#11218: with-demoted-errors use of condition-case-unless-debug; ert

[Lennart Borgman]

On Sat, Apr 21, 2012 at 04:34, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>
> None of these arguments are really specific to ERT.  It just seems like
> you like to run with debug-on-error, which is fine (I do too).

Just a reminder: running with debug-on-error is a pain since some
commands raise an error on user input instead of throwing to command
level.

bug#11218: with-demoted-errors use of condition-case-unless-debug; ert

[Stefan Monnier]

>> None of these arguments are really specific to ERT.  It just seems like
>> you like to run with debug-on-error, which is fine (I do too).
> Just a reminder: running with debug-on-error is a pain since some
> commands raise an error on user input instead of throwing to command
> level.

YMMV but I've been running with debug-on-error for many years and
haven't found it to be a pain.


        Stefan

bug#11218: with-demoted-errors use of condition-case-unless-debug; ert

[Christian Ohler]

Stefan Monnier, 2012-04-20:

>>> I don't see the point of showing that info when running the tests.
>>> If the user wants that info, he should be able to easily say "run this
>>> test with debug-on-error" so he doesn't only get a backtrace but also
>>> gets put in the debugger where he can inspect the state.
>
>> ERT's keyboard shortcut for that is "d".  But it's not a good solution if:
>
>> * the test is nondeterministic (maybe due to subprocess interaction),
>> * the test takes a long time to run,
>> * you have multiple failing tests and want to quickly compare if they are
>> crashing in the same place,
>> * ERT is running in batch mode (e.g. make check).
>
>> It's better to collect as much data as reasonably possible when the error
>> first occurs.
>
> None of these arguments are really specific to ERT.  It just seems like
> you like to run with debug-on-error, which is fine (I do too).

ERT's default behavior is very different from using Emacs with 
debug-on-error enabled, so in terms of what the user sees, I don't think 
this analogy works very well.


> But don't impose it on all ERT users.

This is not a matter of user preference.  Collecting additional 
information about why the test failed is useful to virtually everyone 
who bothers running the test; there should be no reason not to.  (Even 
if the user doesn't care about the details, it will at least allow them 
to file a better bug report.)  The problem is that the implementation 
doesn't interact well with `with-demoted-errors'.  But that problem is 
not user- or situation-dependent, so it shouldn't be a setting.


>>> No: you'll be put in the debugger for the error "a" but if you hit `c'
>>> it will continue and re-enter the debugger for error "b".
>> My Emacs is a few months old, but that doesn't work for me.  Has it
>> changed recently?
> [...]
>> If `debug-on-error' is t, `with-demoted-errors' becomes essentially
>> equivalent to `progn', and (error "b") can ever be reached in
>
> You're right, sorry: brain malfunction.
> But that just means you first have to fix the bug that signals "a" before
> getting to the bug that signals "b".

If it were that simple, this would be the solution to the OP's problem 
as well: Just fix the errors that are being demoted.

It's a bad solution, though; the user shouldn't have to do this. 
Enabling the debugger should not change the behavior of the code being 
debugged.  It's a design flaw in `with-demoted-errors' that its behavior 
depends on the same variable that controls the debugger.  The proper 
solution is to decouple the behaviors, perhaps by splitting the 
variable, perhaps by some other means.

Christian.

bug#11218: with-demoted-errors use of condition-case-unless-debug; ert

[Stefan Monnier]

> Collecting additional information about why the test failed is useful
> to virtually everyone who bothers running the test; there should be no
> reason not to.

I disagree here.  I think it's just as well if the "check" and the
"debug" are done in 2 different steps.

> The proper solution is to decouple the behaviors, perhaps by splitting
> the variable, perhaps by some other means.

No, the proper solution is to make with-demoted-error (when run with
debug-on-error) continue execution (after hitting `c' in the debugger)
as if debug-on-error were nil.


        Stefan

bug#11218: with-demoted-errors use of condition-case-unless-debug; ert

[Christian Ohler]

Stefan Monnier, 2012-05-06:

>> Collecting additional information about why the test failed is useful
>> to virtually everyone who bothers running the test; there should be no
>> reason not to.
>
> I disagree here.  I think it's just as well if the "check" and the
> "debug" are done in 2 different steps.

ERT originally worked like you describe, until I got tired of having to 
re-run each failed test just to get a backtrace.  The computer has the 
information in memory at the time the test fails; it should store it 
somewhere for me to look at later, rather than making me wait while it 
tries to recompute it.  The backtrace is so basic that I practically 
always want to see it.

In batch mode, ERT also has to collect the backtrace on every failure; 
the idea of re-running with debugging enabled makes no sense in that 
context (if enabling debugging to collect backtraces in batch mode was 
an optional flag to be set on a second run after noticing failures on 
the first run, why not set that flag the first time around?).  Also, a 
continuous build system is much more useful if errors come with more 
information since that increases the chances that the bug can be fixed 
without first building a specific version on a specific architecture to 
reproduce it.


>> The proper solution is to decouple the behaviors, perhaps by splitting
>> the variable, perhaps by some other means.
>
> No, the proper solution is to make with-demoted-error (when run with
> debug-on-error) continue execution (after hitting `c' in the debugger)
> as if debug-on-error were nil.

That sounds like a good idea; I would add that it should be possible to 
invoke this "restart" (in CL terms) programmatically.  (This does 
decouple the behaviors: it allows errors to be demoted regardless of the 
setting of `debug-on-error'.)

Christian.

bug#11218: with-demoted-errors use of condition-case-unless-debug; ert

[Noam Postavsky]

Christian Ohler <ohler@gnu.org> writes:

> On 4/13/12 6:44 AM, Stefan Monnier wrote:
>>>>> If you replace with-demoted-errors with ignore-errors, the test passes.
>>>> Looks like a bug in ERT.
>>> I don't know if it's a "bug" per se...
>>> ert--run-test-internal binds debug-on-error to t, and redefines the
>>> debugger to ert--run-test-debugger. As the doc of that function says:
>>
>> I wonder why ERT doesn't just use condition-case to catch and record
>> the errors.
>
> Two reasons that I remember off the top of my head: Recording
> backtraces, and recording additional information provided with
> `ert-info'.  Using condition-case would let ERT regain control only
> after the stack is unwound, at which point that information is gone.

I just posted a patch which changes ERT to use `signal-hook-function' to
record backtraces instead.  It fixes the case in the OP.

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=30745#20

Note that condition-case also needs an enhancement to be able to catch
any signal (Bug#24618), so that might be a third reason why ERT didn't
go this route originally.

bug#11218: with-demoted-errors use of condition-case-unless-debug; ert

[Philipp Stephani]

Am Mi., 15. Aug. 2018 um 03:11 Uhr schrieb Noam Postavsky <npostavs@gmail.com>:
>
> Christian Ohler <ohler@gnu.org> writes:
>
> > On 4/13/12 6:44 AM, Stefan Monnier wrote:
> >>>>> If you replace with-demoted-errors with ignore-errors, the test passes.
> >>>> Looks like a bug in ERT.
> >>> I don't know if it's a "bug" per se...
> >>> ert--run-test-internal binds debug-on-error to t, and redefines the
> >>> debugger to ert--run-test-debugger. As the doc of that function says:
> >>
> >> I wonder why ERT doesn't just use condition-case to catch and record
> >> the errors.
> >
> > Two reasons that I remember off the top of my head: Recording
> > backtraces, and recording additional information provided with
> > `ert-info'.  Using condition-case would let ERT regain control only
> > after the stack is unwound, at which point that information is gone.
>
> I just posted a patch which changes ERT to use `signal-hook-function' to
> record backtraces instead.  It fixes the case in the OP.
>
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=30745#20
>
> Note that condition-case also needs an enhancement to be able to catch
> any signal (Bug#24618), so that might be a third reason why ERT didn't
> go this route originally.
>

This issue continues to bug me because normal and expected use of
with-demoted-errors in (unrelated) libraries breaks tests in annoying
ways. Is there any way I can help move this forward? Is your patch
still blocked on anything?