bug#24402: should-error doesn't catch all errors

[npostavs]

Gemini Lasswell <gazally@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.

>From 7160e8e45ef9e074360e394f99120d11b7ed4b8a Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@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

Alex <agrambot@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@test-cycle-assq
> Edebug: test@test-cycle-assoc
> Edebug: cyc1
> Edebug: test@test-cycle-assq
> Edebug: test@test-cycle-assoc
> ...
> Edebug: cyc1
> Edebug: test@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.