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: Thu, 13 Jul 2017 16:45:39 -0600 Message-ID: <87lgns7y7g.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> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" X-Trace: blaine.gmane.org 1499985985 7335 195.159.176.226 (13 Jul 2017 22:46:25 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Thu, 13 Jul 2017 22:46:25 +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 Fri Jul 14 00:46:19 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 1dVms6-0001C3-NF for geb-bug-gnu-emacs@m.gmane.org; Fri, 14 Jul 2017 00:46:10 +0200 Original-Received: from localhost ([::1]:34519 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dVmsC-0005kU-9Y for geb-bug-gnu-emacs@m.gmane.org; Thu, 13 Jul 2017 18:46:16 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:55625) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dVms2-0005ji-4t for bug-gnu-emacs@gnu.org; Thu, 13 Jul 2017 18:46:07 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dVmry-0005nU-Ts for bug-gnu-emacs@gnu.org; Thu, 13 Jul 2017 18:46:06 -0400 Original-Received: from debbugs.gnu.org ([208.118.235.43]:34831) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1dVmry-0005nE-OA for bug-gnu-emacs@gnu.org; Thu, 13 Jul 2017 18:46:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1dVmry-0006yU-J1 for bug-gnu-emacs@gnu.org; Thu, 13 Jul 2017 18:46:02 -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 22:46:02 +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.149998595626783 (code B ref 24402); Thu, 13 Jul 2017 22:46:02 +0000 Original-Received: (at 24402) by debbugs.gnu.org; 13 Jul 2017 22:45:56 +0000 Original-Received: from localhost ([127.0.0.1]:37508 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dVmrr-0006xv-PT for submit@debbugs.gnu.org; Thu, 13 Jul 2017 18:45:56 -0400 Original-Received: from mail-it0-f65.google.com ([209.85.214.65]:36480) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dVmrq-0006xj-LI for 24402@debbugs.gnu.org; Thu, 13 Jul 2017 18:45:55 -0400 Original-Received: by mail-it0-f65.google.com with SMTP id k3so9730853ita.3 for <24402@debbugs.gnu.org>; Thu, 13 Jul 2017 15:45:54 -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=/qLlY7vc/PYLZa7Vyp+7IYAgg0FwXWrTWTPNDhJZ2Is=; b=tgLfsoTqPAFc8DAm/hHLrsdbBFr32VLguXOmNZBm6HYO0/f9fOefarCjeOcL65NJo4 xExZ+CafpOBkiSpfGYcmeCUeYUWY4orGieJv7E6AlRFbCRWVcFP1ap5uULZjyE3JlAJa IPhf2ZhKa5C95IaHtYwRa4GzPlLjqKTZZ4iSGm7K1DESL1/L5qDKfAF3BkXBw+pwIxUi NH98CoR91gmqYvNrwNg3jHYF/+0ZLwltmFiQyU66gLjXf78m6kIYKJ5L8AaeZqAEJd+j tNDQennW/vjoqcCO+TF18+Bl/kswGALBeMsY9x7c3hHoTzcO3e9KjUzEBuO1NNt9OY3i 9XPA== 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=/qLlY7vc/PYLZa7Vyp+7IYAgg0FwXWrTWTPNDhJZ2Is=; b=scQa4TMpLMBjRSaBQEQzts+byL+h8Z+jRH2Ce6j/VX5cTJI6NSGjdTAngh9vZA1yJA ZhS8/TFr24CAGnc7zyYwiEJnVuGzst6GGOOEaROB5NDNsKQsTAgUzLA1FzV/INRTHDe4 6Ug9vxVXINn5dDUPvotb7Z+nN+axeylmMPio5emDeAGjg31MJ3EkYdg5y33tblapyUoY W5XrNQhAQPmYM/A1WlHyTzJcIrMEvB7sF7Ov6T3dbExj+I9B7UTVaPuq9nCkCk7i55zi 96vcI6VYticRlDadws3zRIeQiW0eC4Y7j0N3uYbYRXYNa78lQdJPwDMDBQoeSU04G9j9 g4gA== X-Gm-Message-State: AIVw113A/TqHKKBCUb5RIqQFnQR2tl+MXDgJ/ugkv9UgmUO3brB8Jnjn A6b06YtRxOCS5Q== X-Received: by 10.107.138.148 with SMTP id c20mr6166964ioj.135.1499985948732; Thu, 13 Jul 2017 15:45:48 -0700 (PDT) Original-Received: from lylat (S010664777d9cebe3.ss.shawcable.net. [70.64.85.59]) by smtp.gmail.com with ESMTPSA id k16sm374724itb.1.2017.07.13.15.45.46 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 13 Jul 2017 15:45:47 -0700 (PDT) In-Reply-To: <87d195dmr0.fsf@users.sourceforge.net> (npostavs's message of "Wed, 12 Jul 2017 23:44: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:134521 Archived-At: --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable npostavs@users.sourceforge.net writes: > Alex 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 >> ). 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=E2=84=A2 can properly fi= x 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=3Dno'. I even reset to the commit I was at before and it still segfaults. Can you reproduce this with the following patch on master? --=-=-= Content-Type: text/x-diff Content-Disposition: attachment; filename=0001-Catch-argument-and-expansion-errors-in-ert.patch Content-Description: ert >From fdbf66f73672d9b69fc37273223ae90fff951eb2 Mon Sep 17 00:00:00 2001 From: Alexander Gramiak Date: Thu, 13 Jul 2017 14:54:35 -0600 Subject: [PATCH] Catch argument and expansion errors in ert This kludge catches errors caused by evaluating arguments in ert's should, should-not, and should-error macros; it also catches macro and inline function expansion errors inside of the above macros (Bug#24402). * lisp/emacs-lisp/ert.el: (ert--should-signal-hook): New function. (ert--expand-should-1): Catch expansion 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 | 47 ++++++++++++++++++++++++++++++--------- test/lisp/emacs-lisp/ert-tests.el | 9 ++++++++ 2 files changed, 46 insertions(+), 10 deletions(-) diff --git a/lisp/emacs-lisp/ert.el b/lisp/emacs-lisp/ert.el index eb2b2e3e11..c6e3ee8503 100644 --- a/lisp/emacs-lisp/ert.el +++ b/lisp/emacs-lisp/ert.el @@ -65,6 +65,7 @@ (require 'find-func) (require 'help) (require 'pp) +(require 'byte-opt) ; for ert--expand-should-1 kludge ;;; UI customization options. @@ -266,6 +267,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--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) @@ -276,13 +285,20 @@ 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)))))) + ;; catch all macro and inline function expansion errors + ;; FIXME: Code inside of tests should be evaluated like it is + ;; outside of tests, with the sole exception of error + ;; handling + (condition-case err + (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))) (let ((value (cl-gensym "value-"))) @@ -298,13 +314,20 @@ 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))) + ;; for inline functions + (byte-code-function-p fn-name))) (let ((fn (cl-gensym "fn-")) (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 +789,10 @@ ert--run-test-internal ;; 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 @@ 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))))) -- 2.13.2 --=-=-=--