From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Alex Newsgroups: gmane.emacs.bugs Subject: bug#24402: should-error doesn't catch all errors Date: Wed, 12 Jul 2017 21:04:38 -0600 Message-ID: <87k23d9gvt.fsf@lylat> References: <3654D8E9-D3CB-402B-922F-B132C1871E9F@runbox.com> <596E65D2-E780-43A1-A75B-603B61B6F9F4@runbox.com> <87zickhoco.fsf_-_@lylat> <877eznda7v.fsf@lylat> <874lur0zki.fsf@calancha-pc> <87o9sywtbz.fsf@lylat> <87fue3f9p8.fsf@users.sourceforge.net> <87vamyl3j3.fsf@lylat> <87tw2het1b.fsf@users.sourceforge.net> <874luhbo4l.fsf@lylat> <87lgntdswo.fsf@users.sourceforge.net> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" X-Trace: blaine.gmane.org 1499915117 2032 195.159.176.226 (13 Jul 2017 03:05:17 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Thu, 13 Jul 2017 03:05:17 +0000 (UTC) User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.0.50 (gnu/linux) Cc: Gemini Lasswell , 24402@debbugs.gnu.org, Tino Calancha To: npostavs@users.sourceforge.net Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Thu Jul 13 05:05:11 2017 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dVURD-0008SY-1r for geb-bug-gnu-emacs@m.gmane.org; Thu, 13 Jul 2017 05:05:11 +0200 Original-Received: from localhost ([::1]:56976 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dVURF-0003xG-EM for geb-bug-gnu-emacs@m.gmane.org; Wed, 12 Jul 2017 23:05:13 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:55998) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dVUR9-0003v4-27 for bug-gnu-emacs@gnu.org; Wed, 12 Jul 2017 23:05:08 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dVUR4-000165-36 for bug-gnu-emacs@gnu.org; Wed, 12 Jul 2017 23:05:07 -0400 Original-Received: from debbugs.gnu.org ([208.118.235.43]:33371) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1dVUR3-00015G-U7 for bug-gnu-emacs@gnu.org; Wed, 12 Jul 2017 23:05:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1dVUR3-00009x-Kq for bug-gnu-emacs@gnu.org; Wed, 12 Jul 2017 23:05:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Alex Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Thu, 13 Jul 2017 03:05:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 24402 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: confirmed patch Original-Received: via spool by 24402-submit@debbugs.gnu.org id=B24402.1499915096597 (code B ref 24402); Thu, 13 Jul 2017 03:05:01 +0000 Original-Received: (at 24402) by debbugs.gnu.org; 13 Jul 2017 03:04:56 +0000 Original-Received: from localhost ([127.0.0.1]:36048 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dVUQx-00009Z-MF for submit@debbugs.gnu.org; Wed, 12 Jul 2017 23:04:55 -0400 Original-Received: from mail-it0-f66.google.com ([209.85.214.66]:32960) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dVUQt-00009K-KU for 24402@debbugs.gnu.org; Wed, 12 Jul 2017 23:04:52 -0400 Original-Received: by mail-it0-f66.google.com with SMTP id 188so5164559itx.0 for <24402@debbugs.gnu.org>; Wed, 12 Jul 2017 20:04:51 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:references:date:in-reply-to:message-id :user-agent:mime-version; bh=ys3HxOf0wNXEFIG/xW823tdo15GFlua9/ISfGQMykEk=; b=jmy/D/CkkoRnFhB/53Be+z9GqJ7c7iKVhGlCPCQMn6VplemJABykY5C5/vEGUkIuaY EajE2p7d7ar86vaoqY2rWQ2k7lVDqzKgaWMdIqEXtbxnmOHhugKdKoX7Lbz86bCtfHHB vydAWiLvAdVZu2Or2OjJy3a+YzxakhVfGhARSb4poAZfNmg0QjUrftwq+RY1bfd2pQIJ L35EMk9vrmv48ttI5EOxeUMOTgDtoR1k0WXvHzLcvsmu/dgybLBFlxyHeLLVXhc22/fc C0Migo0Pp0/ZGRS2ZlXaI4P1ciAS29Pd70LYA0z6wPO3/LZmYb5Q9bm9PUC4vlDscejq kX3w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:references:date:in-reply-to :message-id:user-agent:mime-version; bh=ys3HxOf0wNXEFIG/xW823tdo15GFlua9/ISfGQMykEk=; b=lnEiDJ1PvFikGREL08GjAsj45nwFGBRn8bk9yRFv+i7Ra2ZfHSq0DmYVSYeIrIoY6A vOyj9BXo0uNNOUANVIJD8RgON3r0vTVd80GP7eUzJD0AtXIk5YRp9oUPKtcZkJHOZ8bK D5VfI9EJkIXsCmJRXDmoXSAZAydgS8LOlfxu9o1fY9LzMQ3NffFOI3gd5vr6T796j+uR nwahjwtDuiQhqtd0ctob3bi+jdx+on5FHkhWuS6/1safMUzJyb+0t2YFbsX+7cEGyT5u +yw3fbMiA4KkZj7cXIHQRkLhAlmrkZRj5nsINReQFs7mO4pF+yO/0FZYc84mM2PBOM+0 LcQg== X-Gm-Message-State: AIVw112JGIOo3yTLbyCa+JxmgUuFxYfGRy/oPxr9qI0Xau4xOLc9XYRq q6AF27gh2T2MjQ== X-Received: by 10.36.123.203 with SMTP id q194mr12641574itc.19.1499915086090; Wed, 12 Jul 2017 20:04:46 -0700 (PDT) Original-Received: from lylat (S010664777d9cebe3.ss.shawcable.net. [70.64.85.59]) by smtp.gmail.com with ESMTPSA id q62sm2406855iof.57.2017.07.12.20.04.43 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 12 Jul 2017 20:04:44 -0700 (PDT) In-Reply-To: <87lgntdswo.fsf@users.sourceforge.net> (npostavs's message of "Wed, 12 Jul 2017 21:31:19 -0400") X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 208.118.235.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.org gmane.emacs.bugs:134494 Archived-At: --=-=-= Content-Type: text/plain npostavs@users.sourceforge.net writes: > Alex writes: > >> npostavs@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 ). 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? --=-=-= Content-Type: text/x-diff Content-Disposition: attachment; filename=ert-inline.diff Content-Description: inline diff --git a/lisp/emacs-lisp/ert.el b/lisp/emacs-lisp/ert.el index beb32c48ce..f4d2f725ac 100644 --- a/lisp/emacs-lisp/ert.el +++ b/lisp/emacs-lisp/ert.el @@ -281,17 +281,19 @@ ert--special-operator-p (and (subrp definition) (eql (cdr (subr-arity definition)) 'unevalled))))) +(require 'byte-opt) + (defun ert--expand-should-1 (whole form inner-expander) "Helper function for the `should' macro and its variants." (let ((form (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)))) + (byte-compile-inline-expand (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))) @@ -308,7 +310,8 @@ ert--expand-should-1 (cl-assert (or (symbolp fn-name) (and (consp fn-name) (eql (car fn-name) 'lambda) - (listp (cdr fn-name))))) + (listp (cdr fn-name))) + (byte-code-function-p fn-name))) (let ((fn (cl-gensym "fn-")) (args (cl-gensym "args-")) (value (cl-gensym "value-")) --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable >> 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 =E2=80=98cl-struct-foo=E2=80=99. >>> 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=E2=84=A2 can properly fix t= his eventually. --=-=-=--