emacs-eask
ERT tests which signal always fail
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
You can work around this by running
eask eval '(progn (load-file "./my-test.el") (ert-run-tests-batch-and-exit))'
Seems like a few alternatives for fixes:
- add a flag e.g.
eask--preserve-error-pjust for when errors should be propagated and use it inert.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)))
- always propagate errors unless
eask--ignore-error-pis 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.
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)
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.
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). 🙂
Ok so the "why" is in https://github.com/emacs-mirror/emacs/commit/fe0f15dbc962b37d98507a494fd7720bad584a7a
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?
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? :)
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.
Yeah, I think your solution is much cleaner! Would you like to open a PR for this? 😀
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 https://github.com/emacs-eask/cli/issues/273#issuecomment-2452912394, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHKJEXFDID7ZTMNOJPK2FFDZ6SA4XAVCNFSM6AAAAABQ4YHRGKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINJSHEYTEMZZGQ . You are receiving this because you authored the thread.Message ID: @.***>