GNU bug report logs - #24402
25.1.50; testcover-start breaks should-error

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 24402 in the body.
You can then email your comments to 24402 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Gemini Lasswell <gazally <at> runbox.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 25.1.50; testcover-start breaks should-error
Date: Fri, 9 Sep 2016 18:26:40 -0700

[Message part 1 (text/plain, inline)]

Enabling code coverage on a file containing an ert test that uses should-error causes the test to fail.

Steps to reproduce:
1. Start emacs (I used: open nextstep/Emacs.app —args -Q)

2. Create a buffer containing the following code and save it as bug.el:
(require 'ert)
(require 'testcover)

(ert-deftest should-error-bug ()
  (should-error (my-error)))

(defun my-error ()
  (/ 1 0))

3. M-x eval-buffer RET

4. M-x ert RET t RET

5. The test passes.

6. M-x testcover-start RET bug.el RET

7. M-x ert RET t RET

8. The test fails. TAB TAB d produced the attached debugger output.

[bug.txt (text/plain, attachment)]

[Message part 3 (text/plain, inline)]

In GNU Emacs 25.1.50.8 (x86_64-apple-darwin15.6.0, NS appkit-1404.47 Version 10.11.6 (Build 15G1004))
 of 2016-09-09 built on rainbow.local
Windowing system distributor 'Apple', version 10.3.1404
Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.
You can run the command ‘eval-buffer’ with M-x ev-b RET
Ran 1 tests, 1 results were as expected
Edebug: test <at> should-error-bug
Edebug: my-error
You can run the command ‘testcover-start’ with M-x tes-s RET
Edebug: my-error
Ran 1 tests, 0 results were as expected, 1 unexpected

Configured using:
 'configure --with-ns --without-gnutls'

Configured features:
JPEG IMAGEMAGICK NOTIFY ACL LIBXML2 ZLIB TOOLKIT_SCROLL_BARS NS

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: ERT-Results

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  buffer-read-only: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message subr-x puny dired dired-loaddefs
format-spec rfc822 mml mml-sec password-cache epa derived epg epg-config
gnus-util rmail rmail-loaddefs mm-decode mm-bodies mm-encode mail-parse
rfc2231 mailabbrev gmm-utils mailheader sendmail rfc2047 rfc2045
ietf-drums mm-util mail-prsvr mail-utils cl-seq cl-macs testcover edebug
ert pp find-func seq byte-opt gv bytecomp byte-compile cl-extra
help-mode cconv ewoc easymenu debug cl-loaddefs pcase cl-lib time-date
mule-util tooltip eldoc electric uniquify ediff-hook vc-hooks
lisp-float-type mwheel term/ns-win ns-win ucs-normalize term/common-win
tool-bar dnd fontset image regexp-opt fringe tabulated-list newcomment
elisp-mode lisp-mode prog-mode register page menu-bar rfn-eshadow timer
select scroll-bar mouse jit-lock font-lock syntax facemenu font-core
term/tty-colors frame cl-generic cham georgian utf-8-lang misc-lang
vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms cp51932
hebrew greek romanian slovak czech european ethiopic indian cyrillic
chinese charscript case-table epa-hook jka-cmpr-hook help simple abbrev
obarray minibuffer cl-preloaded nadvice loaddefs button faces cus-face
macroexp files text-properties overlay sha1 md5 base64 format env
code-pages mule custom widget hashtable-print-readable backquote kqueue
cocoa ns multi-tty make-network-process emacs)

Message #8 received at 24402 <at> debbugs.gnu.org (full text, mbox):

From: Gemini Lasswell <gazally <at> runbox.com>
To: 24402 <at> debbugs.gnu.org
Subject: More Information
Date: Mon, 19 Sep 2016 21:27:54 -0700

I’ve done some investigating of why this is happening. testcover-start
transforms:
    (should-error (my-error))
into:
    (should-error (testcover-after 2 (my-error)))

Then the macro expansion of should-error separates the form which it
is passed into a function symbol and list of arguments, and applies
the function to the arguments inside of a condition-case that traps
errors. The problem is that the arguments are evaluated first, outside
of the condition-case, so errors in their evaluation do not get
caught. This problem is not specific to testcover. In this example,
the first test passes and the second fails:

(defun div-by (n)
  (/ 100 n))

(defmacro log-div-by (n)
  `(message "div-by: %d" (div-by ,n)))

(ert-deftest test-div-by ()
  (should-error (div-by 0)))

(ert-deftest test-log-div-by ()
  (should-error (log-div-by 0)))
  

Message #11 received at 24402 <at> debbugs.gnu.org (full text, mbox):

From: Alex <agrambot <at> gmail.com>
To: Gemini Lasswell <gazally <at> runbox.com>
Cc: 24402 <at> debbugs.gnu.org
Subject: should-error doesn't catch all errors (Was:bug#24402: More
 Information)
Date: Mon, 03 Jul 2017 21:28:55 -0600

tags + 24402 confirmed
quit

Gemini Lasswell <gazally <at> runbox.com> writes:

> I’ve done some investigating of why this is happening. testcover-start
> transforms:
>     (should-error (my-error))
> into:
>     (should-error (testcover-after 2 (my-error)))
>
> Then the macro expansion of should-error separates the form which it
> is passed into a function symbol and list of arguments, and applies
> the function to the arguments inside of a condition-case that traps
> errors. The problem is that the arguments are evaluated first, outside
> of the condition-case, so errors in their evaluation do not get
> caught. This problem is not specific to testcover. In this example,
> the first test passes and the second fails:
>
> (defun div-by (n)
>   (/ 100 n))
>
> (defmacro log-div-by (n)
>   `(message "div-by: %d" (div-by ,n)))
>
> (ert-deftest test-div-by ()
>   (should-error (div-by 0)))
>
> (ert-deftest test-log-div-by ()
>   (should-error (log-div-by 0)))
>   

I just ran into this as well. Consider these two forms:

(should-error (cl-fourth "1234") :type 'wrong-type-argument)

(should-error (car (cdr (cdr (cdr "1234")))) :type 'wrong-type-argument)

Only the second raises an error, even though cl-fourth is equivalent to
the car/cdr chain.

Message #18 received at 24402 <at> debbugs.gnu.org (full text, mbox):

From: Alex <agrambot <at> gmail.com>
To: Gemini Lasswell <gazally <at> runbox.com>
Cc: 24402 <at> debbugs.gnu.org
Subject: Re: bug#24402: should-error doesn't catch all errors
Date: Tue, 04 Jul 2017 18:00:04 -0600

[Message part 1 (text/plain, inline)]

tags 24402 patch
quit

Alex <agrambot <at> gmail.com> writes:

> Gemini Lasswell <gazally <at> runbox.com> writes:
>
>> I’ve done some investigating of why this is happening. testcover-start
>> transforms:
>>     (should-error (my-error))
>> into:
>>     (should-error (testcover-after 2 (my-error)))
>>
>> Then the macro expansion of should-error separates the form which it
>> is passed into a function symbol and list of arguments, and applies
>> the function to the arguments inside of a condition-case that traps
>> errors. The problem is that the arguments are evaluated first, outside
>> of the condition-case, so errors in their evaluation do not get
>> caught. This problem is not specific to testcover. In this example,
>> the first test passes and the second fails:
>>
>> (defun div-by (n)
>>   (/ 100 n))
>>
>> (defmacro log-div-by (n)
>>   `(message "div-by: %d" (div-by ,n)))
>>
>> (ert-deftest test-div-by ()
>>   (should-error (div-by 0)))
>>
>> (ert-deftest test-log-div-by ()
>>   (should-error (log-div-by 0)))
>>   
>
> I just ran into this as well. Consider these two forms:
>
> (should-error (cl-fourth "1234") :type 'wrong-type-argument)
>
> (should-error (car (cdr (cdr (cdr "1234")))) :type 'wrong-type-argument)
>
> Only the second raises an error, even though cl-fourth is equivalent to
> the car/cdr chain.

Here's a diff that appears to fix this. It's a bit crude, but I'd rather
not mess around too much with ert's internals.

[ert-args.diff (text/x-diff, attachment)]

[Message part 3 (text/plain, inline)]

There's a similar issue with macro-expansions inside of
should/should-error/should-not that could/should be fixed by wrapping
the macroexpand call at the top of ert--expand-should-1 in a similar
condition-case. Here's another diff that does just that:

[ert-args2.diff (text/x-diff, attachment)]

[Message part 5 (text/plain, inline)]

I ran "make check" and found only one test that the above diff breaks:
ert-test-test-result-expected-p.

I can't seem to figure out why it doesn't work. The test fails because
of these two:

(let ((test (make-ert-test :body (lambda () (ert-fail "failed")))))
  (should-not (ert-test-result-expected-p test (ert-run-test test))))
(let ((test (make-ert-test :body (lambda () (ert-fail "failed"))
                           :expected-result-type ':failed)))
  (should (ert-test-result-expected-p test (ert-run-test test))))

I tried to re-throw the ert-test-failed signal and still the above two
forms raise error an error.

Message #23 received at 24402 <at> debbugs.gnu.org (full text, mbox):

From: Tino Calancha <tino.calancha <at> gmail.com>
To: Alex <agrambot <at> gmail.com>
Cc: Gemini Lasswell <gazally <at> runbox.com>, 24402 <at> debbugs.gnu.org
Subject: Re: bug#24402: should-error doesn't catch all errors
Date: Wed, 05 Jul 2017 22:43:09 +0900

Alex <agrambot <at> gmail.com> writes:

> I ran "make check" and found only one test that the above diff breaks:
> ert-test-test-result-expected-p.
>
> I can't seem to figure out why it doesn't work. The test fails because
> of these two:
>
> (let ((test (make-ert-test :body (lambda () (ert-fail "failed")))))
>   (should-not (ert-test-result-expected-p test (ert-run-test test))))
> (let ((test (make-ert-test :body (lambda () (ert-fail "failed"))
>                            :expected-result-type ':failed)))
>   (should (ert-test-result-expected-p test (ert-run-test test))))
>
> I tried to re-throw the ert-test-failed signal and still the above two
> forms raise error an error.

Hi!
I just arrived from teletransportation from Bug#27559.  Very fast! (and
cheap!).

Thank you for looking on this.  I think you go in the right direction to
fix this problem.

* I have updated your patch and all the test suite pass (even your
  proposed tests in Bug#27559 without requiring "(eval '....)").

* Bear in mind that I am far to be an expert on `ert.el', and i am
  already in my second beer, so please double check that
  the patch have sense.
  
--8<-----------------------------cut here---------------start------------->8---
commit a07f99f062f3da3418060ef30e1a00030fa0b947
Author: Tino Calancha <tino.calancha <at> gmail.com>
Date:   Wed Jul 5 22:11:46 2017 +0900

    Tweak Alex's 2nd patch

diff --git a/lisp/emacs-lisp/ert.el b/lisp/emacs-lisp/ert.el
index eb2b2e3e11..2d131cf99e 100644
--- a/lisp/emacs-lisp/ert.el
+++ b/lisp/emacs-lisp/ert.el
@@ -276,13 +276,15 @@ ert--special-operator-p
 (defun ert--expand-should-1 (whole form inner-expander)
   "Helper function for the `should' macro and its variants."
   (let ((form
-         (macroexpand form (append (bound-and-true-p
-                                    byte-compile-macro-environment)
-                                   (cond
-                                    ((boundp 'macroexpand-all-environment)
-                                     macroexpand-all-environment)
-                                    ((boundp 'cl-macro-environment)
-                                     cl-macro-environment))))))
+         (condition-case err
+             (macroexpand-all form (append (bound-and-true-p
+                                            byte-compile-macro-environment)
+                                           (cond
+                                            ((boundp 'macroexpand-all-environment)
+                                             macroexpand-all-environment)
+                                            ((boundp 'cl-macro-environment)
+                                             cl-macro-environment))))
+           (error `(signal ',(car err) ',(cdr err))))))
     (cond
      ((or (atom form) (ert--special-operator-p (car form)))
       (let ((value (cl-gensym "value-")))
@@ -303,8 +305,14 @@ ert--expand-should-1
               (args (cl-gensym "args-"))
               (value (cl-gensym "value-"))
               (default-value (cl-gensym "ert-form-evaluation-aborted-")))
-          `(let ((,fn (function ,fn-name))
-                 (,args (list ,@arg-forms)))
+          `(let* ((,fn (function ,fn-name))
+                  (,args (condition-case err
+                             (list ,@arg-forms)
+                           (error (if (or (eq (car err) 'ert-test-failed)
+                                          (eq (car err) 'ert-test-skipped))
+                                      (list ,@arg-forms)
+                                    (setq ,fn #'signal)
+                                    (list (car err) (cdr err)))))))
              (let ((,value ',default-value))
                ,(funcall inner-expander
                          `(setq ,value (apply ,fn ,args))
--8<-----------------------------cut here---------------end--------------->8---
In GNU Emacs 26.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.22.11)
 of 2017-07-05
Repository revision: 5d62247323f53f3ae9c7d9f51e951635887b2fb6

Message #26 received at 24402 <at> debbugs.gnu.org (full text, mbox):

From: Alex <agrambot <at> gmail.com>
To: Tino Calancha <tino.calancha <at> gmail.com>
Cc: Gemini Lasswell <gazally <at> runbox.com>, 24402 <at> debbugs.gnu.org
Subject: Re: bug#24402: should-error doesn't catch all errors
Date: Wed, 05 Jul 2017 13:56:48 -0600

Tino Calancha <tino.calancha <at> gmail.com> writes:

> Hi!
> I just arrived from teletransportation from Bug#27559.  Very fast! (and
> cheap!).
>
> Thank you for looking on this.  I think you go in the right direction to
> fix this problem.
>
> * I have updated your patch and all the test suite pass (even your
>   proposed tests in Bug#27559 without requiring "(eval '....)").
>
> * Bear in mind that I am far to be an expert on `ert.el', and i am
>   already in my second beer, so please double check that
>   the patch have sense.
>   
>
> commit a07f99f062f3da3418060ef30e1a00030fa0b947
> Author: Tino Calancha <tino.calancha <at> gmail.com>
> Date:   Wed Jul 5 22:11:46 2017 +0900
>
>     Tweak Alex's 2nd patch
>
> diff --git a/lisp/emacs-lisp/ert.el b/lisp/emacs-lisp/ert.el
> index eb2b2e3e11..2d131cf99e 100644
> --- a/lisp/emacs-lisp/ert.el
> +++ b/lisp/emacs-lisp/ert.el
> @@ -276,13 +276,15 @@ ert--special-operator-p
>  (defun ert--expand-should-1 (whole form inner-expander)
>    "Helper function for the `should' macro and its variants."
>    (let ((form
> -         (macroexpand form (append (bound-and-true-p
> -                                    byte-compile-macro-environment)
> -                                   (cond
> -                                    ((boundp 'macroexpand-all-environment)
> -                                     macroexpand-all-environment)
> -                                    ((boundp 'cl-macro-environment)
> -                                     cl-macro-environment))))))
> +         (condition-case err
> +             (macroexpand-all form (append (bound-and-true-p
> +                                            byte-compile-macro-environment)
> +                                           (cond
> +                                            ((boundp 'macroexpand-all-environment)
> +                                             macroexpand-all-environment)
> +                                            ((boundp 'cl-macro-environment)
> +                                             cl-macro-environment))))
> +           (error `(signal ',(car err) ',(cdr err))))))
>      (cond
>       ((or (atom form) (ert--special-operator-p (car form)))
>        (let ((value (cl-gensym "value-")))
> @@ -303,8 +305,14 @@ ert--expand-should-1
>                (args (cl-gensym "args-"))
>                (value (cl-gensym "value-"))
>                (default-value (cl-gensym "ert-form-evaluation-aborted-")))
> -          `(let ((,fn (function ,fn-name))
> -                 (,args (list ,@arg-forms)))
> +          `(let* ((,fn (function ,fn-name))
> +                  (,args (condition-case err
> +                             (list ,@arg-forms)
> +                           (error (if (or (eq (car err) 'ert-test-failed)
> +                                          (eq (car err) 'ert-test-skipped))
> +                                      (list ,@arg-forms)
> +                                    (setq ,fn #'signal)
> +                                    (list (car err) (cdr err)))))))
>               (let ((,value ',default-value))
>                 ,(funcall inner-expander
>                           `(setq ,value (apply ,fn ,args))

Thanks, that helps with my understanding of the issue. Sadly, I don't
think your tweak will work in all cases, namely whenever (list
,@arg-forms) has side-effects. Luckily this doesn't happen in any
current tests, but I think those types of tests should be supported.

I believe the following is why my previous diff doesn't work. Consider:

(let ((test (make-ert-test :body (lambda () (ert-fail "failed")))))
  (ert-run-test test))

The above returns a struct/record and does not error.

(let ((test (make-ert-test :body (lambda () (ert-fail "failed")))))
  (condition-case err
      (ert-run-test test)
    (error "woops")))

Even though ert-run-test by itself does not error, the error handler is
ran. I believe this is because `ert--run-test-internal' binds `debugger'
around the evaluation of the test to somehow suppress this error.

Using condition-case-unless-debug gets around this, but now anytime
debug-on-error is non-nil the condition-case won't catch the error. I'm
not sure how to solve this.

Message #29 received at 24402 <at> debbugs.gnu.org (full text, mbox):

From: Tino Calancha <tino.calancha <at> gmail.com>
To: Alex <agrambot <at> gmail.com>
Cc: Gemini Lasswell <gazally <at> runbox.com>, 24402 <at> debbugs.gnu.org
Subject: Re: bug#24402: should-error doesn't catch all errors
Date: Fri, 07 Jul 2017 19:15:13 +0900

Alex <agrambot <at> gmail.com> writes:

> I don't think your tweak will work in all cases, namely whenever
> (list  ,@arg-forms) has side-effects. Luckily this doesn't happen in any
> current tests, but I think those types of tests should be supported.
Good point.

> I believe the following is why my previous diff doesn't work. Consider:
>
> (let ((test (make-ert-test :body (lambda () (ert-fail "failed")))))
>   (ert-run-test test))
>
> The above returns a struct/record and does not error.
>
> (let ((test (make-ert-test :body (lambda () (ert-fail "failed")))))
>   (condition-case err
>       (ert-run-test test)
>     (error "woops")))
>
> Even though ert-run-test by itself does not error, the error handler is
> ran. I believe this is because `ert--run-test-internal' binds `debugger'
> around the evaluation of the test to somehow suppress this error.
>
> Using condition-case-unless-debug gets around this, but now anytime
> debug-on-error is non-nil the condition-case won't catch the error. I'm
> not sure how to solve this.
I've being thinking about this, and i cannot find something better than
your second patch.

My suggestion is:

1. We apply your 2nd patch.
2. We ammend the failing tests wrapping '> I don't think your tweak will work in all cases, namely whenever
> (list  ,@arg-forms) has side-effects. Luckily this doesn't happen in any
> current tests, but I think those types of tests should be supported.
Good point.

> I believe the following is why my previous diff doesn't work. Consider:
>
> (let ((test (make-ert-test :body (lambda () (ert-fail "failed")))))
>   (ert-run-test test))
>
> The above returns a struct/record and does not error.
>
> (let ((test (make-ert-test :body (lambda () (ert-fail "failed")))))
>   (condition-case err
>       (ert-run-test test)
>     (error "woops")))
>
> Even though ert-run-test by itself does not error, the error handler is
> ran. I believe this is because `ert--run-test-internal' binds `debugger'
> around the evaluation of the test to somehow suppress this error.
>
> Using condition-case-unless-debug gets around this, but now anytime
> debug-on-error is non-nil the condition-case won't catch the error. I'm
> not sure how to solve this.
I've being thinking about this, and i cannot find something better than
your second patch.

My suggestion is:

1. We apply your 2nd patch.
2. We handle the failing tests wrapping '(ert-fail "failed")' into
  `ignore-errors' as in below patch.

Then, IMO we are in a better situation than we are know:
That fix this bug, and if i am nt missing something, at a low price: just
tweaking a bit 2 innocents tests.

What do you think?


--8<-----------------------------cut here---------------start------------->8---

diff --git a/test/lisp/emacs-lisp/ert-tests.el b/test/lisp/emacs-lisp/ert-tests.el
index 317838b250..f3e4db192a 100644
--- a/test/lisp/emacs-lisp/ert-tests.el
+++ b/test/lisp/emacs-lisp/ert-tests.el
@@ -413,12 +413,14 @@ ert-test--which-file
   (let ((test (make-ert-test :body (lambda ()))))
     (should (ert-test-result-expected-p test (ert-run-test test))))
   ;; unexpected failure
-  (let ((test (make-ert-test :body (lambda () (ert-fail "failed")))))
-    (should-not (ert-test-result-expected-p test (ert-run-test test))))
-  ;; expected failure
-  (let ((test (make-ert-test :body (lambda () (ert-fail "failed"))
-                             :expected-result-type ':failed)))
+  (let ((test (make-ert-test
+               :body (lambda () (ignore-errors (ert-fail "failed"))))))
     (should (ert-test-result-expected-p test (ert-run-test test))))
+  ;; expected failure
+  (let ((test (make-ert-test
+               :body (lambda () (ignore-errors (ert-fail "failed")))
+               :expected-result-type ':failed)))
+    (should-not (ert-test-result-expected-p test (ert-run-test test))))
   ;; `not' expected type
   (let ((test (make-ert-test :body (lambda ())
                              :expected-result-type '(not :failed))))
--8<-----------------------------cut here---------------end--------------->8---

Message #32 received at 24402 <at> debbugs.gnu.org (full text, mbox):

From: Alex <agrambot <at> gmail.com>
To: Tino Calancha <tino.calancha <at> gmail.com>
Cc: Gemini Lasswell <gazally <at> runbox.com>, eliz <at> gnu.org,
 npostavs <at> users.sourceforge.net, 24402 <at> debbugs.gnu.org
Subject: Re: bug#24402: should-error doesn't catch all errors
Date: Sun, 09 Jul 2017 01:04:24 -0600

Tino Calancha <tino.calancha <at> gmail.com> writes:

> I've being thinking about this, and i cannot find something better than
> your second patch.
>
> My suggestion is:
>
> 1. We apply your 2nd patch.
> 2. We handle the failing tests wrapping '(ert-fail "failed")' into
>   `ignore-errors' as in below patch.
>
> Then, IMO we are in a better situation than we are know:
> That fix this bug, and if i am nt missing something, at a low price: just
> tweaking a bit 2 innocents tests.
>
> What do you think?
>
>
> diff --git a/test/lisp/emacs-lisp/ert-tests.el b/test/lisp/emacs-lisp/ert-tests.el
> index 317838b250..f3e4db192a 100644
> --- a/test/lisp/emacs-lisp/ert-tests.el
> +++ b/test/lisp/emacs-lisp/ert-tests.el
> @@ -413,12 +413,14 @@ ert-test--which-file
>    (let ((test (make-ert-test :body (lambda ()))))
>      (should (ert-test-result-expected-p test (ert-run-test test))))
>    ;; unexpected failure
> -  (let ((test (make-ert-test :body (lambda () (ert-fail "failed")))))
> -    (should-not (ert-test-result-expected-p test (ert-run-test test))))
> -  ;; expected failure
> -  (let ((test (make-ert-test :body (lambda () (ert-fail "failed"))
> -                             :expected-result-type ':failed)))
> +  (let ((test (make-ert-test
> +               :body (lambda () (ignore-errors (ert-fail "failed"))))))
>      (should (ert-test-result-expected-p test (ert-run-test test))))
> +  ;; expected failure
> +  (let ((test (make-ert-test
> +               :body (lambda () (ignore-errors (ert-fail "failed")))
> +               :expected-result-type ':failed)))
> +    (should-not (ert-test-result-expected-p test (ert-run-test test))))
>    ;; `not' expected type
>    (let ((test (make-ert-test :body (lambda ())
>                               :expected-result-type '(not :failed))))

I agree that it's a low price, and that it fixes more than it breaks,
but it does involve switching the logic in a simple basic test.

Also note that ignore-errors expands into a condition-case, so instead
of changing the tests, the patch could be tweaked to return nil if the
test signals ert-test-{failed, skipped}. Or the two tests could use
condition-case directly and make sure that ert-test-failed is/isn't
signalled.

I'd like to get some more opinions on this bug, in the hopes that
there's a better solution. Eli/Noam (or anyone happening upon this), do
you have any ideas on how to solve this?

Message #35 received at 24402 <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> users.sourceforge.net
To: Alex <agrambot <at> gmail.com>
Cc: Gemini Lasswell <gazally <at> runbox.com>, 24402 <at> debbugs.gnu.org,
 Tino Calancha <tino.calancha <at> gmail.com>
Subject: Re: bug#24402: should-error doesn't catch all errors
Date: Tue, 11 Jul 2017 08:18:43 -0400

Alex <agrambot <at> gmail.com> writes:

> I believe the following is why my previous diff doesn't work. Consider:
>
> (let ((test (make-ert-test :body (lambda () (ert-fail "failed")))))
>   (ert-run-test test))
>
> The above returns a struct/record and does not error.
>
> (let ((test (make-ert-test :body (lambda () (ert-fail "failed")))))
>   (condition-case err
>       (ert-run-test test)
>     (error "woops")))
>
> Even though ert-run-test by itself does not error, the error handler is
> ran. I believe this is because `ert--run-test-internal' binds `debugger'
> around the evaluation of the test to somehow suppress this error.

Yes, ert binds `debugger' in order to get full backtrace information
when there is an error.  This means it won't see errors caught by
condition-case.  That's good when it ignores errors caught by test code
using condition-case, but does give rise to problems.  There is some
relevant discussion in Bugs #11218 and #24617.

Espcially the suggestion in #24617 of using `signal-hook-function' to
record error info instead of using `debugger', I think doing this could
simplify things a lot.  It is definitely going to require messing around
with ert's internals though...

Message #38 received at 24402 <at> debbugs.gnu.org (full text, mbox):

From: Alex <agrambot <at> gmail.com>
To: npostavs <at> users.sourceforge.net
Cc: Gemini Lasswell <gazally <at> runbox.com>, 24402 <at> debbugs.gnu.org,
 Tino Calancha <tino.calancha <at> gmail.com>
Subject: Re: bug#24402: should-error doesn't catch all errors
Date: Tue, 11 Jul 2017 21:47:44 -0600

[Message part 1 (text/plain, inline)]

npostavs <at> users.sourceforge.net writes:

> Yes, ert binds `debugger' in order to get full backtrace information
> when there is an error.  This means it won't see errors caught by
> condition-case.  That's good when it ignores errors caught by test code
> using condition-case, but does give rise to problems.  There is some
> relevant discussion in Bugs #11218 and #24617.
>
> Espcially the suggestion in #24617 of using `signal-hook-function' to
> record error info instead of using `debugger', I think doing this could
> simplify things a lot.  It is definitely going to require messing around
> with ert's internals though...

Thanks for the info. I may have discovered a workaround, but I'm not
sure if there's any negative side-effects. All the tests pass, though.

What do you think of it? It's obviously not ideal, but I think it at
least fixes the issues at hand.

[ert-3.diff (text/x-diff, inline)]

diff --git a/lisp/emacs-lisp/ert.el b/lisp/emacs-lisp/ert.el
index eb2b2e3e11..beb32c48ce 100644
--- a/lisp/emacs-lisp/ert.el
+++ b/lisp/emacs-lisp/ert.el
@@ -266,6 +266,14 @@ ert--signal-should-execution
   (when ert--should-execution-observer
     (funcall ert--should-execution-observer form-description)))
 
+;; See Bug#24402 for why this exists
+(defun ert--signal-hook (error-symbol data)
+  "Stupid hack to stop `condition-case' from catching ert signals.
+It should only be stopped when ran from inside ert--run-test-internal."
+  (when (and (not (symbolp debugger))   ; only run on anonymous procedures
+             (memq error-symbol '(ert-test-failed ert-test-skipped)))
+    (funcall debugger 'error data)))
+
 (defun ert--special-operator-p (thing)
   "Return non-nil if THING is a symbol naming a special operator."
   (and (symbolp thing)
@@ -276,13 +284,15 @@ ert--special-operator-p
 (defun ert--expand-should-1 (whole form inner-expander)
   "Helper function for the `should' macro and its variants."
   (let ((form
-         (macroexpand form (append (bound-and-true-p
-                                    byte-compile-macro-environment)
-                                   (cond
-                                    ((boundp 'macroexpand-all-environment)
-                                     macroexpand-all-environment)
-                                    ((boundp 'cl-macro-environment)
-                                     cl-macro-environment))))))
+         (condition-case err
+             (macroexpand-all form (append (bound-and-true-p
+                                            byte-compile-macro-environment)
+                                           (cond
+                                            ((boundp 'macroexpand-all-environment)
+                                             macroexpand-all-environment)
+                                            ((boundp 'cl-macro-environment)
+                                             cl-macro-environment))))
+           (error `(signal ',(car err) ',(cdr err))))))
     (cond
      ((or (atom form) (ert--special-operator-p (car form)))
       (let ((value (cl-gensym "value-")))
@@ -303,8 +313,13 @@ ert--expand-should-1
               (args (cl-gensym "args-"))
               (value (cl-gensym "value-"))
               (default-value (cl-gensym "ert-form-evaluation-aborted-")))
-          `(let ((,fn (function ,fn-name))
-                 (,args (list ,@arg-forms)))
+          `(let* ((,fn (function ,fn-name))
+                  (,args (condition-case err
+                             (let ((signal-hook-function #'ert--signal-hook))
+                               (list ,@arg-forms))
+                           (error (progn (setq ,fn #'signal)
+                                         (list (car err)
+                                               (cdr err)))))))
              (let ((,value ',default-value))
                ,(funcall inner-expander
                          `(setq ,value (apply ,fn ,args))
diff --git a/test/lisp/emacs-lisp/ert-tests.el b/test/lisp/emacs-lisp/ert-tests.el
index 317838b250..b6a7eb68da 100644
--- a/test/lisp/emacs-lisp/ert-tests.el
+++ b/test/lisp/emacs-lisp/ert-tests.el
@@ -294,6 +294,15 @@ ert-self-test-and-exit
                   "the error signaled was a subtype of the expected type")))))
     ))
 
+(ert-deftest ert-test-should-error-argument ()
+  "Errors due to evaluating arguments should not break tests."
+  (should-error (identity (/ 1 0))))
+
+(ert-deftest ert-test-should-error-macroexpansion ()
+  "Errors due to expanding macros should not break tests."
+  (cl-macrolet ((test () (error "Foo")))
+    (should-error (test))))
+
 (ert-deftest ert-test-skip-unless ()
   ;; Don't skip.
   (let ((test (make-ert-test :body (lambda () (skip-unless t)))))

Message #41 received at 24402 <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> users.sourceforge.net
To: Alex <agrambot <at> gmail.com>
Cc: Gemini Lasswell <gazally <at> runbox.com>,
 Tino Calancha <tino.calancha <at> gmail.com>, 24402 <at> debbugs.gnu.org
Subject: Re: bug#24402: should-error doesn't catch all errors
Date: Wed, 12 Jul 2017 08:30:56 -0400

Alex <agrambot <at> gmail.com> writes:

> npostavs <at> users.sourceforge.net writes:
>
>> Yes, ert binds `debugger' in order to get full backtrace information
>> when there is an error.  This means it won't see errors caught by
>> condition-case.  That's good when it ignores errors caught by test code
>> using condition-case, but does give rise to problems.  There is some
>> relevant discussion in Bugs #11218 and #24617.
>>
>> Espcially the suggestion in #24617 of using `signal-hook-function' to
>> record error info instead of using `debugger', I think doing this could
>> simplify things a lot.  It is definitely going to require messing around
>> with ert's internals though...
>
> Thanks for the info. I may have discovered a workaround, but I'm not
> sure if there's any negative side-effects. All the tests pass, though.
>
> What do you think of it? It's obviously not ideal, but I think it at
> least fixes the issues at hand.

Does it also work when loading the elc version of the test file?  (try
'make check TEST_LOAD_EL=no')

What about tests like this?

    (ert-deftest check-error-handling ()
      (should
       (eq 42
           (condition-case ()
               (/ 1 0)
             (arith-error 42)))))

Message #44 received at 24402 <at> debbugs.gnu.org (full text, mbox):

From: Alex <agrambot <at> gmail.com>
To: npostavs <at> users.sourceforge.net
Cc: Gemini Lasswell <gazally <at> runbox.com>,
 Tino Calancha <tino.calancha <at> gmail.com>, 24402 <at> debbugs.gnu.org
Subject: Re: bug#24402: should-error doesn't catch all errors
Date: Wed, 12 Jul 2017 10:45:14 -0600

npostavs <at> users.sourceforge.net writes:

> Alex <agrambot <at> gmail.com> writes:
>
>> npostavs <at> users.sourceforge.net writes:
>>
>>> Yes, ert binds `debugger' in order to get full backtrace information
>>> when there is an error.  This means it won't see errors caught by
>>> condition-case.  That's good when it ignores errors caught by test code
>>> using condition-case, but does give rise to problems.  There is some
>>> relevant discussion in Bugs #11218 and #24617.
>>>
>>> Espcially the suggestion in #24617 of using `signal-hook-function' to
>>> record error info instead of using `debugger', I think doing this could
>>> simplify things a lot.  It is definitely going to require messing around
>>> with ert's internals though...
>>
>> Thanks for the info. I may have discovered a workaround, but I'm not
>> sure if there's any negative side-effects. All the tests pass, though.
>>
>> What do you think of it? It's obviously not ideal, but I think it at
>> least fixes the issues at hand.
>
> Does it also work when loading the elc version of the test file?  (try
> 'make check TEST_LOAD_EL=no')

Oh, it doesn't load the elc version by default? That's surprising; I
think that should be documented in the test README.

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). The other failing tests are
subr-test-backtrace-integration-test and cl-lib-defstruct-record.

> What about tests like this?
>
>     (ert-deftest check-error-handling ()
>       (should
>        (eq 42
>            (condition-case ()
>                (/ 1 0)
>              (arith-error 42)))))

It works for me, yes. As long as `debugger' is set to a symbol. I can
make it a bit more robust by using an additional defvar in
ert--run-test-internal.

Are you asking because it doesn't work for you?

Message #47 received at 24402 <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> users.sourceforge.net
To: Alex <agrambot <at> gmail.com>
Cc: Gemini Lasswell <gazally <at> runbox.com>,
 Tino Calancha <tino.calancha <at> gmail.com>, 24402 <at> debbugs.gnu.org
Subject: Re: bug#24402: should-error doesn't catch all errors
Date: Wed, 12 Jul 2017 21:31:19 -0400

Alex <agrambot <at> gmail.com> writes:

> npostavs <at> users.sourceforge.net writes:
>
>> Does it also work when loading the elc version of the test file?  (try
>> 'make check TEST_LOAD_EL=no')
>
> Oh, it doesn't load the elc version by default? That's surprising; I
> think that should be documented in the test README.
>
> 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.

> The other failing tests are
> subr-test-backtrace-integration-test and cl-lib-defstruct-record.

Hmm, I'll see if I can fix these.

>> What about tests like this?
>>
>>     (ert-deftest check-error-handling ()
>>       (should
>>        (eq 42
>>            (condition-case ()
>>                (/ 1 0)
>>              (arith-error 42)))))
>
> It works for me, yes. As long as `debugger' is set to a symbol. I can
> make it a bit more robust by using an additional defvar in
> ert--run-test-internal.
>
> Are you asking because it doesn't work for you?

No, I'm just trying to explore the edges of this solution.  Isn't
`debugger' bound to a non-symbol while running the the tests?  I'm
confused as to why this solution works.

Message #50 received at 24402 <at> debbugs.gnu.org (full text, mbox):

From: Alex <agrambot <at> gmail.com>
To: npostavs <at> users.sourceforge.net
Cc: Gemini Lasswell <gazally <at> runbox.com>,
 Tino Calancha <tino.calancha <at> gmail.com>, 24402 <at> debbugs.gnu.org
Subject: Re: bug#24402: should-error doesn't catch all errors
Date: Wed, 12 Jul 2017 21:04:38 -0600

[Message part 1 (text/plain, inline)]

npostavs <at> users.sourceforge.net writes:

> Alex <agrambot <at> gmail.com> writes:
>
>> npostavs <at> users.sourceforge.net writes:
>>
>>> Does it also work when loading the elc version of the test file?  (try
>>> 'make check TEST_LOAD_EL=no')
>>
>> Oh, it doesn't load the elc version by default? That's surprising; I
>> think that should be documented in the test README.
>>
>> 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?

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?

[ert-inline.diff (text/x-diff, attachment)]

[Message part 3 (text/plain, inline)]

>> The other failing tests are
>> subr-test-backtrace-integration-test and cl-lib-defstruct-record.
>
> Hmm, I'll see if I can fix these.

Thanks. I noticed when byte-compiling cl-lib-tests, I got this warning:
Unused lexical variable ‘cl-struct-foo’.

>>> What about tests like this?
>>>
>>>     (ert-deftest check-error-handling ()
>>>       (should
>>>        (eq 42
>>>            (condition-case ()
>>>                (/ 1 0)
>>>              (arith-error 42)))))
>>
>> It works for me, yes. As long as `debugger' is set to a symbol. I can
>> make it a bit more robust by using an additional defvar in
>> ert--run-test-internal.
>>
>> Are you asking because it doesn't work for you?
>
> No, I'm just trying to explore the edges of this solution.  Isn't
> `debugger' bound to a non-symbol while running the the tests?  I'm
> confused as to why this solution works.

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}.

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.

Message #53 received at 24402 <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> users.sourceforge.net
To: Alex <agrambot <at> gmail.com>
Cc: Gemini Lasswell <gazally <at> runbox.com>,
 Tino Calancha <tino.calancha <at> gmail.com>, 24402 <at> debbugs.gnu.org
Subject: Re: bug#24402: should-error doesn't catch all errors
Date: Wed, 12 Jul 2017 23:44:19 -0400

Alex <agrambot <at> gmail.com> 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.

Message #56 received at 24402 <at> debbugs.gnu.org (full text, mbox):

From: Alex <agrambot <at> gmail.com>
To: npostavs <at> users.sourceforge.net
Cc: Gemini Lasswell <gazally <at> runbox.com>,
 Tino Calancha <tino.calancha <at> gmail.com>, 24402 <at> debbugs.gnu.org
Subject: Re: bug#24402: should-error doesn't catch all errors
Date: Thu, 13 Jul 2017 16:45:39 -0600

[Message part 1 (text/plain, inline)]

npostavs <at> users.sourceforge.net writes:

> Alex <agrambot <at> gmail.com> writes:
>
>> 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.

I agree, but that sounds like it'll require a fair bit of refactoring
and knowledge of ert internals.

OOC, is there a robust way to check whether or not you're currently
byte-compiling?

>> 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?

I tried a nested case using two ert-deftests and it worked, so it looks
okay here too.

>> 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.

I was going to ask if you would merge in a few days, but it appears that
what should have been a simple rebase to master caused unforeseen
consequences. For instance, for some reason I now get a segmentation
fault when executing 'make cl-lib-tests TEST_LOAD_EL=no'. I even reset
to the commit I was at before and it still segfaults. Can you reproduce
this with the following patch on master?

[0001-Catch-argument-and-expansion-errors-in-ert.patch (text/x-diff, attachment)]

Message #59 received at 24402 <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> users.sourceforge.net
To: Alex <agrambot <at> gmail.com>
Cc: Gemini Lasswell <gazally <at> runbox.com>,
 Tino Calancha <tino.calancha <at> gmail.com>, 24402 <at> debbugs.gnu.org
Subject: Re: bug#24402: should-error doesn't catch all errors
Date: Thu, 13 Jul 2017 19:49:26 -0400

Alex <agrambot <at> gmail.com> writes:

> npostavs <at> users.sourceforge.net writes:
>> 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.
>
> I agree, but that sounds like it'll require a fair bit of refactoring
> and knowledge of ert internals.

I don't think so, just a conditional to decide whether or not to call
the extra expansion.  Do you think there is anything else?

> OOC, is there a robust way to check whether or not you're currently
> byte-compiling?

AFAIK, the usual trick is (bound-and-true-p byte-compile-current-file).
It's probably good enough for most things.

> I was going to ask if you would merge in a few days, but it appears that
> what should have been a simple rebase to master caused unforeseen
> consequences. For instance, for some reason I now get a segmentation
> fault when executing 'make cl-lib-tests TEST_LOAD_EL=no'. I even reset
> to the commit I was at before and it still segfaults. Can you reproduce
> this with the following patch on master?

Nope, I just get the failures on cl-lib-defstruct-record we already
mentioned.

Message #62 received at 24402 <at> debbugs.gnu.org (full text, mbox):

From: Alex <agrambot <at> gmail.com>
To: npostavs <at> users.sourceforge.net
Cc: Gemini Lasswell <gazally <at> runbox.com>,
 Tino Calancha <tino.calancha <at> gmail.com>, 24402 <at> debbugs.gnu.org
Subject: Re: bug#24402: should-error doesn't catch all errors
Date: Thu, 13 Jul 2017 22:42:10 -0600

npostavs <at> users.sourceforge.net writes:

> Alex <agrambot <at> gmail.com> writes:
>
>> npostavs <at> users.sourceforge.net writes:
>>> 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.
>>
>> I agree, but that sounds like it'll require a fair bit of refactoring
>> and knowledge of ert internals.
>
> I don't think so, just a conditional to decide whether or not to call
> the extra expansion.  Do you think there is anything else?

I was mostly referring to not binding `debugger', but also evaluating
the code "normally" (i.e., not doing expansions first in one
condition-case, evaluating arguments in another, and then the whole form
in a third one).

>> OOC, is there a robust way to check whether or not you're currently
>> byte-compiling?
>
> AFAIK, the usual trick is (bound-and-true-p byte-compile-current-file).
> It's probably good enough for most things.

I believe the below patch does that, though it has some issues.

>> I was going to ask if you would merge in a few days, but it appears that
>> what should have been a simple rebase to master caused unforeseen
>> consequences. For instance, for some reason I now get a segmentation
>> fault when executing 'make cl-lib-tests TEST_LOAD_EL=no'. I even reset
>> to the commit I was at before and it still segfaults. Can you reproduce
>> this with the following patch on master?
>
> Nope, I just get the failures on cl-lib-defstruct-record we already
> mentioned.

The segfault appears to have been because I didn't wipe out the elc
files when testing different implementations.

I spent a lot longer than I'd like to admit finding this out. Is there a
reason why "make clean" in the test directory doesn't wipe out elc
files? I don't understand why there's a separate bootstrap-clean that
does this. Can this and TEST_LOAD_EL please be documented in the test
README?

Anyway, I got everything back in order. Sadly, there's a couple extra
tests that now fail for me in the patch that *doesn't* expand inline
functions, and these don't fail for me in a clean master. They are in
eieio-tests (23 and 24).

With the inline expansion, I also get some errors in ert-tests. All of
the errors, with the exception of subr-tests error, seem to be from
cl-defstruct and cl-typep (which is defined by define-inline).

Do you have any ideas? There should be 5 unexpected errors without the
inline expansion, and 6 errors with it. Note that all tests pass in both
cases without "TEST_LOAD_EL=no".

If it's easy to fix the eieio tests and not the other ones, then it
might be better to leave the inline-function expansion out for now.

Message #65 received at 24402 <at> debbugs.gnu.org (full text, mbox):

From: Alex <agrambot <at> gmail.com>
To: npostavs <at> users.sourceforge.net
Cc: 24402 <at> debbugs.gnu.org
Subject: Re: bug#24402: should-error doesn't catch all errors
Date: Thu, 13 Jul 2017 22:45:15 -0600

[Message part 1 (text/plain, inline)]

Alex <agrambot <at> gmail.com> writes:

> I believe the below patch does that, though it has some issues.

Here's the patch, sorry:

[0001-Catch-argument-and-expansion-errors-in-ert.patch (text/x-diff, attachment)]

Message #68 received at 24402 <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> users.sourceforge.net
To: Alex <agrambot <at> gmail.com>
Cc: Gemini Lasswell <gazally <at> runbox.com>,
 Tino Calancha <tino.calancha <at> gmail.com>, 24402 <at> debbugs.gnu.org
Subject: Re: bug#24402: should-error doesn't catch all errors
Date: Sat, 15 Jul 2017 17:57:14 -0400

Alex <agrambot <at> gmail.com> writes:

> The segfault appears to have been because I didn't wipe out the elc
> files when testing different implementations.

I suspect getting a segfault might indicate an actual bug somewhere.

> I spent a lot longer than I'd like to admit finding this out. Is there a
> reason why "make clean" in the test directory doesn't wipe out elc
> files? I don't understand why there's a separate bootstrap-clean that
> does this. Can this and TEST_LOAD_EL please be documented in the test
> README?

I think it was basically copied from the other Makefiles, where cleaning
all elc files would mean a very long subsequent compilation.  It might
make sense to break the pattern for the test/ subdirectory though.

> Anyway, I got everything back in order. Sadly, there's a couple extra
> tests that now fail for me in the patch that *doesn't* expand inline
> functions, and these don't fail for me in a clean master. They are in
> eieio-tests (23 and 24).

I'm seeing eieio-tests failing also in master.  This seems to be an
actual bug, in the definition of `cl-typep' I think.  I've opened a new
bug for this (Bug#27718).

> With the inline expansion, I also get some errors in ert-tests. All of
> the errors, with the exception of subr-tests error, seem to be from
> cl-defstruct and cl-typep (which is defined by define-inline).
>
> Do you have any ideas? There should be 5 unexpected errors without the
> inline expansion, and 6 errors with it. Note that all tests pass in both
> cases without "TEST_LOAD_EL=no".
>
> If it's easy to fix the eieio tests and not the other ones, then it
> might be better to leave the inline-function expansion out for now.

I have a fix for the subr-tests failed, as for the others, I don't know
enough about the compilation process to untangle it yet.  I think we
should just leave the inline-function expansion part out for now, at
which point I believe your patch won't be making anything worse, so it
should be okay to install.

Message #71 received at 24402 <at> debbugs.gnu.org (full text, mbox):

From: Alex <agrambot <at> gmail.com>
To: npostavs <at> users.sourceforge.net
Cc: Gemini Lasswell <gazally <at> runbox.com>,
 Tino Calancha <tino.calancha <at> gmail.com>, 24402 <at> debbugs.gnu.org
Subject: Re: bug#24402: should-error doesn't catch all errors
Date: Sat, 15 Jul 2017 21:49:06 -0600

[Message part 1 (text/plain, inline)]

npostavs <at> users.sourceforge.net writes:

> Alex <agrambot <at> gmail.com> writes:
>
>> The segfault appears to have been because I didn't wipe out the elc
>> files when testing different implementations.
>
> I suspect getting a segfault might indicate an actual bug somewhere.

Probably, but I can't reproduce it anymore since I got rid of the
offending .elc files.

>> I spent a lot longer than I'd like to admit finding this out. Is there a
>> reason why "make clean" in the test directory doesn't wipe out elc
>> files? I don't understand why there's a separate bootstrap-clean that
>> does this. Can this and TEST_LOAD_EL please be documented in the test
>> README?
>
> I think it was basically copied from the other Makefiles, where cleaning
> all elc files would mean a very long subsequent compilation.  It might
> make sense to break the pattern for the test/ subdirectory though.

That makes sense to me, but if others disagree then the behaviour being
documented is the next best thing.

>> Anyway, I got everything back in order. Sadly, there's a couple extra
>> tests that now fail for me in the patch that *doesn't* expand inline
>> functions, and these don't fail for me in a clean master. They are in
>> eieio-tests (23 and 24).
>
> I'm seeing eieio-tests failing also in master.  This seems to be an
> actual bug, in the definition of `cl-typep' I think.  I've opened a new
> bug for this (Bug#27718).

Oddly enough, I can't reproduce this on master. I cloned a new copy, ran
"./autogen.sh && ./configure && make -j4", then ran "make eieio-tests
TEST_LOAD_EL=no" with no error. I cloned from 30444c695, then tried
again with 7a0fb008. I also tried "make check-expensive TEST_LOAD_EL=no"
and got only 2 errors (dom-tests and cl-lib-tests).

Perhaps odder is that I can still reproduce your recipe in Bug#27718 in
that same repository.

>> With the inline expansion, I also get some errors in ert-tests. All of
>> the errors, with the exception of subr-tests error, seem to be from
>> cl-defstruct and cl-typep (which is defined by define-inline).
>>
>> Do you have any ideas? There should be 5 unexpected errors without the
>> inline expansion, and 6 errors with it. Note that all tests pass in both
>> cases without "TEST_LOAD_EL=no".
>>
>> If it's easy to fix the eieio tests and not the other ones, then it
>> might be better to leave the inline-function expansion out for now.
>
> I have a fix for the subr-tests failed, as for the others, I don't know
> enough about the compilation process to untangle it yet.  I think we
> should just leave the inline-function expansion part out for now, at
> which point I believe your patch won't be making anything worse, so it
> should be okay to install.

Sounds good. It would be lucky if a fix to Bug#27718 also resolves the
other inline function errors so that we could use the previous patch.
Here's an updated patch with inline function excluded:

[0001-Catch-argument-and-macroexpansion-errors-in-ert.patch (text/x-diff, attachment)]

Message #74 received at 24402 <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> users.sourceforge.net
To: Alex <agrambot <at> gmail.com>
Cc: Gemini Lasswell <gazally <at> runbox.com>,
 Tino Calancha <tino.calancha <at> gmail.com>, 24402 <at> debbugs.gnu.org
Subject: Re: bug#24402: should-error doesn't catch all errors
Date: Sun, 16 Jul 2017 20:46:02 -0400

Alex <agrambot <at> gmail.com> writes:

> Oddly enough, I can't reproduce this on master. I cloned a new copy, ran
> "./autogen.sh && ./configure && make -j4", then ran "make eieio-tests
> TEST_LOAD_EL=no" with no error. I cloned from 30444c695, then tried
> again with 7a0fb008. I also tried "make check-expensive TEST_LOAD_EL=no"
> and got only 2 errors (dom-tests and cl-lib-tests).
>
> Perhaps odder is that I can still reproduce your recipe in Bug#27718 in
> that same repository.

Uh, actually, now I'm getting the same results as you (i.e., make
eieio-tests TEST_LOAD_EL=no passes on master for me, though the #27718
test case does fail).  Perhaps I accidentally ran an eieio-tests.elc
file produced by a patched Emacs using a non-patched Emacs before?

>> I have a fix for the subr-tests failed, as for the others, I don't know
>> enough about the compilation process to untangle it yet.  I think we
>> should just leave the inline-function expansion part out for now, at
>> which point I believe your patch won't be making anything worse, so it
>> should be okay to install.
>
> Sounds good. It would be lucky if a fix to Bug#27718 also resolves the
> other inline function errors so that we could use the previous patch.
> Here's an updated patch with inline function excluded:

Okay, so the patch does make a couple more tests fail, but I think it's
actually an improvement since #27718 shows that the scenario was already
failing outside of ert-deftest, i.e., in this case your patch is making
code inside the test more like code outside the test.  We can just mark
those as expected failures when compiled.

I will merge to master in a few days, assuming there are no objections.

Message #77 received at 24402 <at> debbugs.gnu.org (full text, mbox):

From: Gemini Lasswell <gazally <at> runbox.com>
To: Alex <agrambot <at> gmail.com>
Cc: 24402 <at> debbugs.gnu.org, Tino Calancha <tino.calancha <at> gmail.com>,
 npostavs <at> users.sourceforge.net
Subject: Re: bug#24402: should-error doesn't catch all errors
Date: Wed, 19 Jul 2017 14:23:12 -0700

[Message part 1 (text/plain, inline)]

Hello,

First, thank you for working on this bug!

I have a branch of Emacs which attempts to run all the tests under
Testcover, and I've tried it with Alex's latest patch. The patch has
fixed the problems with Testcover and should-error except for one weird
case. Here is a file that will let you reproduce the weird case on
master with the patch applied:

[cyc-test.el (text/plain, attachment)]

[Message part 3 (text/plain, inline)]

This is an excerpt of test/src/fns-tests.el with a few lines added at
the end to invoke Testcover. It contains two nearly identical tests
which should both pass, but one passes and one fails. If you then edit
the file and comment out test-cycle-assoc (the one that passes) and run
the test again, then the failing test will pass. I've reproduced it both
in batch mode (without TEST_LOAD_EL=no) and interactively.

Message #80 received at 24402 <at> debbugs.gnu.org (full text, mbox):

From: Alex <agrambot <at> gmail.com>
To: Gemini Lasswell <gazally <at> runbox.com>
Cc: 24402 <at> debbugs.gnu.org, Tino Calancha <tino.calancha <at> gmail.com>,
 npostavs <at> users.sourceforge.net
Subject: Re: bug#24402: should-error doesn't catch all errors
Date: Wed, 19 Jul 2017 16:40:03 -0600

Gemini Lasswell <gazally <at> runbox.com> writes:

> Hello,
>
> First, thank you for working on this bug!
>
> I have a branch of Emacs which attempts to run all the tests under
> Testcover, and I've tried it with Alex's latest patch. The patch has
> fixed the problems with Testcover and should-error except for one weird
> case. Here is a file that will let you reproduce the weird case on
> master with the patch applied:
>
>
>
> This is an excerpt of test/src/fns-tests.el with a few lines added at
> the end to invoke Testcover. It contains two nearly identical tests
> which should both pass, but one passes and one fails. If you then edit
> the file and comment out test-cycle-assoc (the one that passes) and run
> the test again, then the failing test will pass. I've reproduced it both
> in batch mode (without TEST_LOAD_EL=no) and interactively.

Hmm, this isn't the behaviour that I see. I get the following errors
both with and without my patch, and both interactively and in batch
mode:

===============================================================================================
Edebug: cyc1
Edebug: test <at> test-cycle-assq
Edebug: test <at> test-cycle-assoc
Edebug: cyc1
Edebug: test <at> test-cycle-assq
Edebug: test <at> test-cycle-assoc
...
Edebug: cyc1
Edebug: test <at> test-cycle-assq
Compiler-macro error for cl--block-wrapper: (error "Lisp nesting exceeds ‘max-lisp-eval-depth’")
Edebug: test <at> test-cycle-assoc
Compiler-macro error for cl--block-wrapper: (error "Lisp nesting exceeds ‘max-lisp-eval-depth’")
Edebug: cyc1
Edebug: test <at> test-cycle-assq
Compiler-macro error for cl--block-wrapper: (error "Lisp nesting exceeds ‘max-lisp-eval-depth’")
Eager macro-expansion failure: (error "Lisp nesting exceeds ‘max-lisp-eval-depth’")
Compiler-macro error for cl--block-wrapper: (error "Lisp nesting exceeds ‘max-lisp-eval-depth’")
Eager macro-expansion failure: (error "Lisp nesting exceeds ‘max-lisp-eval-depth’")
Compiler-macro error for cl--block-wrapper: (error "Lisp nesting exceeds ‘max-lisp-eval-depth’")
Entering debugger...
cl--generic-make-next-function: Symbol’s function definition is void: t
===============================================================================================

The ... above represents Edebug cycling, for some reason, between the 3
functions.

Noam, can you reproduce this?

Message #83 received at 24402 <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> users.sourceforge.net
To: Gemini Lasswell <gazally <at> runbox.com>
Cc: Tino Calancha <tino.calancha <at> gmail.com>, Alex <agrambot <at> gmail.com>,
 24402 <at> debbugs.gnu.org
Subject: Re: bug#24402: should-error doesn't catch all errors
Date: Wed, 19 Jul 2017 19:04:57 -0400

[Message part 1 (text/plain, inline)]

Gemini Lasswell <gazally <at> runbox.com> writes:

> This is an excerpt of test/src/fns-tests.el with a few lines added at
> the end to invoke Testcover. It contains two nearly identical tests
> which should both pass, but one passes and one fails.

If I run them again in the same Emacs session then they both fail.  The
problem seems to be that testcover saves values produced in tests, and
when it tries to compare the 2 circular lists produced by these tests, a
`circular-list' error is thrown.  In other words, this is actually a
totally separate bug (although it's hidden until a fix for #24402 is
applied).

The following appears to fix it, though perhaps we should use a smarter
equal function that would consider the circular lists to actually be
equal instead of bailing out and returning nil on circularity.

[v1-0001-Don-t-error-on-circular-values-in-testcover.patch (text/x-diff, inline)]

From 7160e8e45ef9e074360e394f99120d11b7ed4b8a Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs <at> gmail.com>
Date: Wed, 19 Jul 2017 18:48:50 -0400
Subject: [PATCH v1] Don't error on circular values in testcover

* lisp/emacs-lisp/testcover.el (testcover-after, testcover-1value):
Consider circular lists to be non-equal instead of signaling error.
---
 lisp/emacs-lisp/testcover.el | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lisp/emacs-lisp/testcover.el b/lisp/emacs-lisp/testcover.el
index 433ad38a14..bc6004a709 100644
--- a/lisp/emacs-lisp/testcover.el
+++ b/lisp/emacs-lisp/testcover.el
@@ -463,7 +463,9 @@ (defun testcover-after (idx val)
   (cond
    ((eq (aref testcover-vector idx) 'unknown)
     (aset testcover-vector idx val))
-   ((not (equal (aref testcover-vector idx) val))
+   ((not (condition-case ()
+             (equal (aref testcover-vector idx) val)
+           (circular-list nil)))
     (aset testcover-vector idx 'ok-coverage)))
   val)
 
@@ -475,7 +477,9 @@ (defun testcover-1value (idx val)
    ((eq (aref testcover-vector idx) '1value)
     (aset testcover-vector idx (cons '1value val)))
    ((not (and (eq (car-safe (aref testcover-vector idx)) '1value)
-	      (equal (cdr (aref testcover-vector idx)) val)))
+	      (condition-case ()
+                  (equal (cdr (aref testcover-vector idx)) val)
+                (circular-list nil))))
     (error "Value of form marked with `1value' does vary: %s" val)))
   val)
 
-- 
2.11.1

[Message part 3 (text/plain, inline)]

Alex <agrambot <at> gmail.com> writes:

> Hmm, this isn't the behaviour that I see. I get the following errors
> both with and without my patch, and both interactively and in batch
> mode:
>
> ===============================================================================================
> Edebug: cyc1
> Edebug: test <at> test-cycle-assq
> Edebug: test <at> test-cycle-assoc
> Edebug: cyc1
> Edebug: test <at> test-cycle-assq
> Edebug: test <at> test-cycle-assoc
> ...
> Edebug: cyc1
> Edebug: test <at> test-cycle-assq
> Compiler-macro error for cl--block-wrapper: (error "Lisp nesting exceeds ‘max-lisp-eval-depth’")

Hmm, that's strange, I don't get that error (or any "Edebug: cyc1"
repetitions after the first) on master, or with your patch.

Message #86 received at 24402 <at> debbugs.gnu.org (full text, mbox):

From: Alex <agrambot <at> gmail.com>
To: npostavs <at> users.sourceforge.net
Cc: Gemini Lasswell <gazally <at> runbox.com>,
 Tino Calancha <tino.calancha <at> gmail.com>, 24402 <at> debbugs.gnu.org
Subject: Re: bug#24402: should-error doesn't catch all errors
Date: Wed, 19 Jul 2017 22:04:03 -0600

npostavs <at> users.sourceforge.net writes:

> Gemini Lasswell <gazally <at> runbox.com> writes:
>
>> This is an excerpt of test/src/fns-tests.el with a few lines added at
>> the end to invoke Testcover. It contains two nearly identical tests
>> which should both pass, but one passes and one fails.
>
> If I run them again in the same Emacs session then they both fail.  The
> problem seems to be that testcover saves values produced in tests, and
> when it tries to compare the 2 circular lists produced by these tests, a
> `circular-list' error is thrown.  In other words, this is actually a
> totally separate bug (although it's hidden until a fix for #24402 is
> applied).
>
> The following appears to fix it, though perhaps we should use a smarter
> equal function that would consider the circular lists to actually be
> equal instead of bailing out and returning nil on circularity.

That would be nice.

> Alex <agrambot <at> gmail.com> writes:
> 
> > Hmm, this isn't the behaviour that I see. I get the following errors
> > both with and without my patch, and both interactively and in batch
> > mode:
> >
> > ===============================================================================================
> > Edebug: cyc1
> > Edebug: test <at> test-cycle-assq
> > Edebug: test <at> test-cycle-assoc
> > Edebug: cyc1
> > Edebug: test <at> test-cycle-assq
> > Edebug: test <at> test-cycle-assoc
> > ...
> > Edebug: cyc1
> > Edebug: test <at> test-cycle-assq
> > Compiler-macro error for cl--block-wrapper: (error "Lisp nesting exceeds ‘max-lisp-eval-depth’")
> 
> Hmm, that's strange, I don't get that error (or any "Edebug: cyc1"
> repetitions after the first) on master, or with your patch.

Whoops, I changed the defvar to a setq and forgot to change it back.
Doing this results in the error I posted. (Which makes sense since it
keeps reloading the file, resulting in the "Edebug:" loop. The
compiler-macro errors and `t' being called as a function are a bit odd,
though).

I get the same behaviour as you and Gemini now. Sorry about the noise.

Message #89 received at 24402 <at> debbugs.gnu.org (full text, mbox):

From: Gemini Lasswell <gazally <at> runbox.com>
To: npostavs <at> users.sourceforge.net
Cc: Tino Calancha <tino.calancha <at> gmail.com>, Alex <agrambot <at> gmail.com>,
 24402 <at> debbugs.gnu.org
Subject: Re: bug#24402: should-error doesn't catch all errors
Date: Thu, 20 Jul 2017 12:23:25 -0700

[Message part 1 (text/plain, inline)]

npostavs <at> users.sourceforge.net writes:

> The following appears to fix it, though perhaps we should use a smarter
> equal function that would consider the circular lists to actually be
> equal instead of bailing out and returning nil on circularity.

Thanks for tracking this one down and making a patch, which looks good
to me. Does that smarter equal function already exist? It doesn't seem
worth the effort of writing one for this purpose only.

Here is a test to add to your patch, which fails without it and passes
with it:

[0001-Add-a-test-of-handling-of-circular-values-to-testcov.patch (text/plain, inline)]

From f401e3169ff3887b8215b6625d111d70e5340efe Mon Sep 17 00:00:00 2001
From: Gemini Lasswell <gazally <at> runbox.com>
Date: Thu, 20 Jul 2017 12:01:42 -0700
Subject: [PATCH] Add a test of handling of circular values to testcover-tests

* test/lisp/emacs-lisp-testcover-resources/testcases.el
(testcover-testcase-cyc1): New function.
(testcover-tests-circular-lists-bug-24402): New test.
---
 test/lisp/emacs-lisp/testcover-resources/testcases.el | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/test/lisp/emacs-lisp/testcover-resources/testcases.el b/test/lisp/emacs-lisp/testcover-resources/testcases.el
index 1eb791a993..c9a5a6daac 100644
--- a/test/lisp/emacs-lisp/testcover-resources/testcases.el
+++ b/test/lisp/emacs-lisp/testcover-resources/testcases.el
@@ -490,4 +490,14 @@ testcover-testcase-how-do-i-know-you
 
 (should (eq (testcover-testcase-how-do-i-know-you "Liz") 'unknown))
 
+;; ==== circular-lists-bug-24402 ====
+"Testcover captures and ignores circular list errors."
+;; ====
+(defun testcover-testcase-cyc1 (a)
+  (let ((ls (make-list 10 a%%%)))
+    (nconc ls ls)
+    ls))
+(testcover-testcase-cyc1 1)
+(testcover-testcase-cyc1 1)
+
 ;; testcases.el ends here.
-- 
2.12.2

Message #92 received at 24402 <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> users.sourceforge.net
To: Gemini Lasswell <gazally <at> runbox.com>
Cc: Tino Calancha <tino.calancha <at> gmail.com>, Alex <agrambot <at> gmail.com>,
 24402 <at> debbugs.gnu.org
Subject: Re: bug#24402: should-error doesn't catch all errors
Date: Mon, 07 Aug 2017 21:15:14 -0400

tags 24402 fixed
close 24402 26.1
quit

Sorry for the delay, I was moving apartment, and then I got a bit
distracted by my 'grep --null' mess.

Gemini Lasswell <gazally <at> runbox.com> writes:

> npostavs <at> users.sourceforge.net writes:
>
>> The following appears to fix it, though perhaps we should use a smarter
>> equal function that would consider the circular lists to actually be
>> equal instead of bailing out and returning nil on circularity.
>
> Thanks for tracking this one down and making a patch, which looks good
> to me. Does that smarter equal function already exist? It doesn't seem
> worth the effort of writing one for this purpose only.

Yeah, I left it as is with a TODO.  We can fix it later if it turns out
to cause an actual problem.

> Here is a test to add to your patch, which fails without it and passes
> with it:

Thanks, I pushed it to master along with the others.

[1: 054c198c12]: 2017-08-07 18:43:54 -0400
  Catch argument and macroexpansion errors in ert
  http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=054c198c120c1f01a8ff753892d52710b740acc6

[2: 95a04fd26c]: 2017-08-07 18:43:55 -0400
  ; Avoid test failures when running from compiled test files
  http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=95a04fd26c91e6c6c9191a629d26886f136e30fc

[3: 0508045ed7]: 2017-08-07 18:54:44 -0400
  Don't error on circular values in testcover
  http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=0508045ed7159bce5b5ea3b5fb72cf78b8b4ee8e

[4: 00f7e31110]: 2017-08-07 18:54:48 -0400
  Add a test of handling of circular values to testcover-tests
  http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=00f7e31110a27e568529192d7441d9631b9096bc

Message #103 received at 24402 <at> debbugs.gnu.org (full text, mbox):

From: phillip.lord <at> russet.org.uk (Phillip Lord)
To: 24402 <at> debbugs.gnu.org
Cc: 30745 <at> debbugs.gnu.org
Subject: Re: bug#30745: 26.0.91; ert should macros nest strangely
Date: Thu, 08 Mar 2018 14:49:27 +0000

Just noting that the fix for this bug has caused a regression.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.