ERT tests which signal always fail

[joshbax189]

For example

;; my-test.el
(require 'ert)

(ert-deftest my-error/test ()
  "Tests errors"
  (should-error (error "an error")))

Running with Eask v0.10.1 in Emacs > version 30

> eask test ert my-test.el
# ... trace output omitted
Test my-error/test condition:
    (ert-test-failed
     ((should-error (error "an error")) :form (error "an error") :value
      nil :fail-reason "did not signal an error"))
   FAILED  1/1  my-error/test (0.000239 sec) at my-test.el:5

Ran 1 tests, 0 results as expected, 1 unexpected (2024-10-30 11:33:38-0700, 0.094291 sec)

1 unexpected results:
   FAILED  my-error/test

[joshbax189]

You can work around this by running

eask eval '(progn (load-file "./my-test.el") (ert-run-tests-batch-and-exit))'

[joshbax189]

Seems like a few alternatives for fixes:

1. add a flag e.g. eask--preserve-error-p just for when errors should be propagated and use it in ert.el

(defun eask--error (fnc &rest args)
  "On error.

Arguments FNC and ARGS are used for advice `:around'."
  (let ((msg (eask--ansi 'error (apply #'format-message args))))
    (unless eask-inhibit-error-message
      (eask--unsilent (eask-msg "%s" msg)))
    (run-hook-with-args 'eask-on-error-hook 'error msg)
    (eask--trigger-error))
  (when (or debug-on-error
            eask--preserve-error-p)
    (apply fnc args)))

2. always propagate errors unless eask--ignore-error-p is non-nil

(defun eask--error (fnc &rest args)
  "On error.

Arguments FNC and ARGS are used for advice `:around'."
  (let ((msg (eask--ansi 'error (apply #'format-message args))))
    (unless eask-inhibit-error-message
      (eask--unsilent (eask-msg "%s" msg)))
    (unless eask--ignore-error-p
      (run-hook-with-args 'eask-on-error-hook 'error msg)
      (eask--trigger-error)
      (apply fnc args))))

Solution 2 is probably better IMO, because having multiple similar flags/macros is confusing and the error behavior is likely required in other places than just ert.el. (It is a bit surprising that errors just disappear unless they are explicitly ignored.)
But using this solution might cause new errors to show up in code which isn't inside a eask-ignore-errors macro right now.

[jcs090218]

This bug should have been fixed a long while ago. 🤔

I've tried it in both local/global scopes:

$ eask test ert my-test.el -g

Under a project with Eask-file existence:

$ eask test ert test/ert my-test.el

Result:

Loading package information... done ✓
Loading /Users/jenchieh/Documents/_lang/elisp/openai/test/my-test.el (source)...
Running 1 tests (2024-10-30 19:14:16-0700, selector ‘t’)
an error
   passed  1/1  my-error/test (0.000091 sec)

Ran 1 tests, 1 results as expected, 0 unexpected (2024-10-30 19:14:16-0700, 0.000218 sec)

[joshbax189]

Hmm, perhaps it's to do with emacs version? I'm on 31.0.50. I'll see what happens when using other versions.

Edit: Ok seems like it works for 28 and 29, but fails for 30 and 31.

[jcs090218]

Yeah, the unreleased versions can still be unstable. 🤔

It sounds like an upstream issue from Emacs. It might be a good idea to report this to them (if we could find the culprit). 🙂

[joshbax189]

Ok so the "why" is in emacs-mirror/emacs@fe0f15d
In summary, ert previously bound debug-on-error while running, but now doesn't (since v30). Hence eask--error will not raise the error.

It seems this change was because some methods didn't quite work the same when debug-on-error was bound: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=11218 ... Since the change fixes several bugs, I don't think the maintainers will take kindly to us asking them to revert this!

IMO preventing all errors like this is likely the wrong approach, it could break code that loads _prepare.el in strange ways. Perhaps a better way would use condition-case to target just unhandled errors that would otherwise interrupt execution?

[jcs090218]

Ah, i see. Thanks for the helpful information! 🚀

How about?

diff --git a/lisp/_prepare.el b/lisp/_prepare.el
index e9ab76c..a354e17 100644
--- a/lisp/_prepare.el
+++ b/lisp/_prepare.el
@@ -172,7 +172,10 @@ workspace has no valid Eask-file; it will load global workspace instead."

 This is added because we don't want to pollute `error' and `warn' functions."
   (eask-command-p '("load" "exec" "emacs" "eval" "repl"
-                    "run/script" "run/command")))
+                    "run/script" "run/command"
+                    ;; NOTE: These test commands handle the exit code themselves;
+                    ;; therefore, we don't need to handle it for them!
+                    "test/ert" "test/ert-runner")))

 (defun eask-checker-p ()
   "Return t if running Eask as the checker.

Since both ert and ert-runner handle the exit code themselves, so I can safely tells that we don't need to help handle that.

IMO preventing all errors like this is likely the wrong approach, it could break code that loads _prepare.el in strange ways. Perhaps a better way would use condition-case to target just unhandled errors that would otherwise interrupt execution?

Could you provide more information about this solution? :)

[joshbax189]

Ok some of the strange ways it breaks...

Because (error "x") is defined as (signal 'error "x"), not all errors are stopped by the advice, just errors that are created by actually calling the function error

(defun my-error-advice (f &rest args)
  (message "blocked error"))

(advice-add 'error :around #'my-error-advice)

(error "hello") ;; => "blocked error"

(car 123) ;; => error type mismatch, not blocked

Because error and signal are functions that never return, by converting to a function that returns a value (or nil), lots of code executes quite differently

(ignore-errors
  (car 123) ;; => nil, as expected

(ignore-errors
  (error "an error")) ;; => "blocked error", the result of the message call

(condition-case nil
  (car 123)
 (error (message "fixing the error"))) ;; => "fixing the error"

(condition-case nil
  (error "123")
 (error (message "fixing the error"))) ;; => "blocked error", the catch handler is not run

(unwind-protect
  (unsafe-file-operation) ;; assume it calls error
                                      ;; assume the advice calls (kill-emacs)
 ;; this is the "finally" block
 (clean-up-files))   ;; never runs because of the kill-emacs call above

Solution

What I'd propose to do instead is wrap the outermost execution level in a condition-case to catch total failure errors, and then wrap each "continue-able" step in a condition-case that checks --allow-errors and either handles the error or rethrows.
E.g. these would wrap steps like lint a single file, removing individual files in clean, so that later files would still be worked on.

Here's a sketch of how that might look:

Around the outermost call:

(defmacro eask-start (&rest body)
  "Execute BODY with workspace setup."
  (declare (indent 0) (debug t))
   `(condition-case err
     ;; original macro
     (error
        ;; this is eask--error
        (let ((msg (eask--ansi 'error (apply #'format-message args))))
          (unless eask-inhibit-error-message
            (eask--unsilent (eask-msg "%s" msg)))
          (run-hook-with-args 'eask-on-error-hook 'error msg))
          ;; since it is the outer call, there is nothing to continue so ignore --allow-errors value and exit
          (kill-emacs 1)))

This catches any unhandled errors of any type by printing a formatted message and exiting with a non-zero status.

To implement the allow-error option, we provide something like:

(defmacro eask-maybe-allow-error (&rest body)
  `(condition-case err
     ,@body
     (error
       (unless (eask-allow-error-p)
          (error err)) ;; rethrow and let the handler in eask-start deal with it
       ;; otherwise print message, run hook and set exit status as  before
       (add-hook 'eask-after-command-hook #'eask--exit))))

And then, around steps like that in declare.el

(defun eask-lint-declare--file (filename)
  "Run check-declare on FILENAME."
  (eask-maybe-allow-error
  (let* ((filename (expand-file-name filename))
         (file (eask-root-del filename))
         (errors))
  ;; etc
      (eask-msg "No issues found"))))

Most of the work would be in finding the right places within commands to use eask-maybe-allow-error and checking that the option really works when there are errors.

[jcs090218]

Yeah, I think your solution is much cleaner! Would you like to open a PR for this? 😀

[joshbax189]

Cheers! Will do.

…

On Sat, 2 Nov 2024, 00:59 Jen-Chieh Shen, ***@***.***> wrote: Yeah, I think your solution is much cleaner! Would you like to open a PR for this? 😀 — Reply to this email directly, view it on GitHub <#273 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHKJEXFDID7ZTMNOJPK2FFDZ6SA4XAVCNFSM6AAAAABQ4YHRGKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINJSHEYTEMZZGQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>