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: Sat, 15 Jul 2017 21:49:06 -0600 Message-ID: <87inithwi5.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> <87k23d9gvt.fsf@lylat> <87d195dmr0.fsf@users.sourceforge.net> <87lgns7y7g.fsf@lylat> <877ezbew3d.fsf@users.sourceforge.net> <87d193ljdp.fsf@lylat> <87shhxcqit.fsf@users.sourceforge.net> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" X-Trace: blaine.gmane.org 1500177013 24757 195.159.176.226 (16 Jul 2017 03:50:13 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Sun, 16 Jul 2017 03:50:13 +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 Sun Jul 16 05:50:09 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 1dWaZL-00063K-6H for geb-bug-gnu-emacs@m.gmane.org; Sun, 16 Jul 2017 05:50:07 +0200 Original-Received: from localhost ([::1]:44026 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dWaZP-0002Om-4f for geb-bug-gnu-emacs@m.gmane.org; Sat, 15 Jul 2017 23:50:11 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:37648) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dWaZJ-0002NV-JA for bug-gnu-emacs@gnu.org; Sat, 15 Jul 2017 23:50:06 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dWaZG-0001fs-FY for bug-gnu-emacs@gnu.org; Sat, 15 Jul 2017 23:50:05 -0400 Original-Received: from debbugs.gnu.org ([208.118.235.43]:38238) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1dWaZG-0001fU-7K for bug-gnu-emacs@gnu.org; Sat, 15 Jul 2017 23:50:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1dWaZF-0005WQ-LD for bug-gnu-emacs@gnu.org; Sat, 15 Jul 2017 23:50:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Alex Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sun, 16 Jul 2017 03:50: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.150017696221174 (code B ref 24402); Sun, 16 Jul 2017 03:50:01 +0000 Original-Received: (at 24402) by debbugs.gnu.org; 16 Jul 2017 03:49:22 +0000 Original-Received: from localhost ([127.0.0.1]:40915 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dWaYb-0005VS-Ej for submit@debbugs.gnu.org; Sat, 15 Jul 2017 23:49:21 -0400 Original-Received: from mail-io0-f196.google.com ([209.85.223.196]:36827) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dWaYZ-0005VE-5J for 24402@debbugs.gnu.org; Sat, 15 Jul 2017 23:49:19 -0400 Original-Received: by mail-io0-f196.google.com with SMTP id h134so5151307iof.3 for <24402@debbugs.gnu.org>; Sat, 15 Jul 2017 20:49:19 -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=cvF7pPFKCThi01/TaAiaNF9TdCesBwbNQ6dS+DbI8Ow=; b=YP0ETkjGhaCuDTb36e0jnhvv1qrF2abeoA2H24uewvgIfMd/f4dpEJLVOzljlWNVEv qX/+ZFyNzsJ4pmt4Qw16/qIFDt5g1Qu2MOGC+3kZgNgVzyqfrOPnkTii6t+Ezqmnt+Wt p5h6A3HRnztL/M8WngdQc0DEp/7Z4b87JcVpOmdLOfdzGuXnQp1PhyA7qoT8LzRVSf40 fb3IF594O+0fq7b+yFm38bcUIpAC82f3It1q2nEyXrybX2Gu9nA/K4+FmgxW8f+70bEs c9Zqll9Wkc3IWS89Sit3ehiDLlspBZxHLVdoGiDmf7HgSOVEtUnad9t4ydk3wcAuHYuc f4nA== 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=cvF7pPFKCThi01/TaAiaNF9TdCesBwbNQ6dS+DbI8Ow=; b=eD0dNmFuqZG9lPG/+HumoYrXwb4U6LAOwDCUnPYeaMm5PbNnnr0xu7/pyyt8qHZey+ JtrNJakIZeE4s1G5qvHolUSG2PRuihSBEeAGrZX1znjdBCWr7KMyc4XtKFJsJxoZ6AYz FIW9y8VJzmbaaBmLs4t4Z1Sla2MGlqvM1B1e1+XaCG68FN9tLoOg9BoPDfdDduDF5pqO FXNyokZd27R5RYkTtFIoiiWZ7ZGx7oAMxVufYUW/SfAZOzQCyELI/qN6EBMO85tB5Pnn IDQBewgbQoMrndtorjmhw3uuhMiOM0yGvt/CxreDBKekOjbREMKpOjap4NKK+jA+AW8b oGSQ== X-Gm-Message-State: AIVw1102rwUFcGDad9MfnDLSuLFGhWQF8TqMVltOhvIf8619JMnqmYZo WjefDV4D5t+l+A== X-Received: by 10.107.162.6 with SMTP id l6mr16210862ioe.84.1500176953455; Sat, 15 Jul 2017 20:49:13 -0700 (PDT) Original-Received: from lylat (S010664777d9cebe3.ss.shawcable.net. [70.64.85.59]) by smtp.gmail.com with ESMTPSA id b17sm4854483itd.0.2017.07.15.20.49.10 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Sat, 15 Jul 2017 20:49:11 -0700 (PDT) In-Reply-To: <87shhxcqit.fsf@users.sourceforge.net> (npostavs's message of "Sat, 15 Jul 2017 17:57:14 -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:134610 Archived-At: --=-=-= Content-Type: text/plain npostavs@users.sourceforge.net writes: > Alex 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: --=-=-= Content-Type: text/x-diff Content-Disposition: attachment; filename=0001-Catch-argument-and-macroexpansion-errors-in-ert.patch >From 05f0734dd0b066058d263756a2e98cafc4ef8515 Mon Sep 17 00:00:00 2001 From: Alexander Gramiak Date: Thu, 13 Jul 2017 14:54:35 -0600 Subject: [PATCH] Catch argument and macroexpansion errors in ert This kludge catches errors caused by evaluating arguments in ert's should, should-not, and should-error macros; it also catches macroexpansion errors inside of the above macros (Bug#24402). * lisp/emacs-lisp/ert.el: (ert--should-signal-hook): New function. (ert--expand-should-1): Catch macroexpansion errors. * test/lisp/emacs-lisp/ert-tests.el (ert-test-should-error-argument) (ert-test-should-error-macroexpansion): Tests for argument and expansion errors. --- lisp/emacs-lisp/ert.el | 41 ++++++++++++++++++++++++++++++--------- test/lisp/emacs-lisp/ert-tests.el | 9 +++++++++ 2 files changed, 41 insertions(+), 9 deletions(-) diff --git a/lisp/emacs-lisp/ert.el b/lisp/emacs-lisp/ert.el index eb2b2e3e11..caa4a9f389 100644 --- a/lisp/emacs-lisp/ert.el +++ b/lisp/emacs-lisp/ert.el @@ -266,6 +266,14 @@ DATA is displayed to the user and should state the reason for skipping." (when ert--should-execution-observer (funcall ert--should-execution-observer form-description))) +;; See Bug#24402 for why this exists +(defun ert--should-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 debugger + (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) @@ -273,16 +281,22 @@ DATA is displayed to the user and should state the reason for skipping." (and (subrp definition) (eql (cdr (subr-arity definition)) 'unevalled))))) +;; FIXME: Code inside of here should probably be evaluated like it is +;; outside of tests, with the sole exception of error handling (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)))))) + ;; catch macroexpansion errors + (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 +317,13 @@ DATA is displayed to the user and should state the reason for skipping." (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--should-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)) @@ -766,6 +785,10 @@ This mainly sets up debugger-related bindings." ;; too expensive, we can remove it. (with-temp-buffer (save-window-excursion + ;; FIXME: Use `signal-hook-function' instead of `debugger' to + ;; handle ert errors. Once that's done, remove + ;; `ert--should-signal-hook'. See Bug#24402 and Bug#11218 for + ;; details. (let ((debugger (lambda (&rest args) (ert--run-test-debugger test-execution-info 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 @@ failed or if there was a problem." "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))))) -- 2.13.2 --=-=-=--