bug#67862: 30.0.50; Handler-bind and ert-test-error-debug

[J.P.]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> - For `my-filter`, process.c:6329 it's the `!NILP (Vdebug_on_error)` test in:
>
>     internal_condition_case_1 (read_process_output_call,
>                              list3 (outstream, make_lisp_proc (p), text),
>                              !NILP (Vdebug_on_error) ? Qnil : Qerror,
>                              read_process_output_error_handler);
>
> - For `my-timer`, it's the `condition-case-unless-debug` in timer.el:319:
>
>         (condition-case-unless-debug err
>             ;; Timer functions should not change the current buffer.
>             ;; If they do, all kinds of nasty surprises can happen,
>             ;; and it can be hellish to track down their source.
>             (save-current-buffer
>               (apply (timer--function timer) (timer--args timer)))
>           (error (message "Error running timer%s: %S"
>                           (if (symbolp (timer--function timer))
>                               (format-message " `%s'" (timer--function timer))
>                             "")
>                           err)))
>
> The reason to catch errors in those two cases is that these codes are
> normally run asynchronously, so it makes sense to catch errors rather
> than propagate them up to some unrelated ambient code being executed.
>
> For `my-filter` I can see a good argument that errors should be
> propagated when you pass `proc` explicitly to `accept-process-output`
> and the filter/sentinel that signals the error is indeed this very
> process: in that case, there is a very clear connection between the
> filter/sentinel and the ambient code which would justify propagating
> the error.

Appreciate the insight re `debug-on-error'. Indeed, running `my-filter'
with the variable enabled regains the original behavior WRT the
backtrace, the summary, and the exit code. And I suppose those worried
about such details (like ERC's bot, which depends on JUnit reports from
EMBA pipelines) can just enable it themselves.

>
> For `my-timer`, on the other hand, hmm...

For this one, enabling `debug-on-error' at least seems to restore the
original behavior in terms of exiting nonzero, which should free users
from having to grep for "Error running timer" in the output of passing
tests.

  -*- mode: compilation; default-directory: "~/emacs/master/test/" -*-
  Compilation started at Fri Feb  2 14:51:58

  make lisp/emacs-lisp/ert-tests.log
    ELC+ELN  lisp/emacs-lisp/ert-tests.elc
    GEN      lisp/emacs-lisp/ert-tests.log
  Running 1 tests (2024-02-02 14:52:02-0800, selector `my-timer')
  Debugger entered--Lisp error: (error "Encountered a problem")
    error("Encountered a problem")
    (closure (ert--test-body-was-run t) nil (error "Encountered a problem"))()
    apply((closure (ert--test-body-was-run t) nil (error "Encountered a 
problem")) nil)
    [...]
    ert-run-tests-batch(my-timer)
    ert-run-tests-batch-and-exit(my-timer)
    eval((ert-run-tests-batch-and-exit 'my-timer) t)
    command-line-1(("-L" ":." "-l" "ert" "-l"
     "lisp/emacs-lisp/ert-tests.el" "--eval"
     "(ert-run-tests-batch-and-exit (quote my-timer))"))
    command-line()
    normal-top-level()

  make: *** [Makefile:181: lisp/emacs-lisp/ert-tests.log] Error 255

  Compilation exited abnormally with code 2 at Fri Feb  2 14:52:02, duration 
4.30 s

I guess folks who depend on these always running to completion will
still have to adapt a bit, but in light of this workaround, I'd say
these changes are much less disruptive than originally feared, which is
a welcome relief. Thanks.