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: Tue, 04 Jul 2017 18:00:04 -0600 Message-ID: <877eznda7v.fsf@lylat> References: <3654D8E9-D3CB-402B-922F-B132C1871E9F@runbox.com> <596E65D2-E780-43A1-A75B-603B61B6F9F4@runbox.com> <87zickhoco.fsf_-_@lylat> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" X-Trace: blaine.gmane.org 1499212878 542 195.159.176.226 (5 Jul 2017 00:01:18 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Wed, 5 Jul 2017 00:01:18 +0000 (UTC) User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.0.50 (gnu/linux) Cc: 24402@debbugs.gnu.org To: Gemini Lasswell Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Wed Jul 05 02:01:10 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 1dSXkh-00088O-QK for geb-bug-gnu-emacs@m.gmane.org; Wed, 05 Jul 2017 02:01:08 +0200 Original-Received: from localhost ([::1]:43495 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dSXkn-0005fE-58 for geb-bug-gnu-emacs@m.gmane.org; Tue, 04 Jul 2017 20:01:13 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:43603) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dSXkg-0005d1-LQ for bug-gnu-emacs@gnu.org; Tue, 04 Jul 2017 20:01:08 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dSXkc-0007BS-Mv for bug-gnu-emacs@gnu.org; Tue, 04 Jul 2017 20:01:06 -0400 Original-Received: from debbugs.gnu.org ([208.118.235.43]:50022) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1dSXkc-0007BF-Hu for bug-gnu-emacs@gnu.org; Tue, 04 Jul 2017 20:01:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1dSXkc-00012Q-BI for bug-gnu-emacs@gnu.org; Tue, 04 Jul 2017 20:01:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Alex Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Wed, 05 Jul 2017 00:01: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 Original-Received: via spool by 24402-submit@debbugs.gnu.org id=B24402.14992128203937 (code B ref 24402); Wed, 05 Jul 2017 00:01:02 +0000 Original-Received: (at 24402) by debbugs.gnu.org; 5 Jul 2017 00:00:20 +0000 Original-Received: from localhost ([127.0.0.1]:52699 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dSXjv-00011M-W5 for submit@debbugs.gnu.org; Tue, 04 Jul 2017 20:00:20 -0400 Original-Received: from mail-io0-f169.google.com ([209.85.223.169]:35628) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dSXjt-000112-5z; Tue, 04 Jul 2017 20:00:17 -0400 Original-Received: by mail-io0-f169.google.com with SMTP id h134so77799905iof.2; Tue, 04 Jul 2017 17:00:17 -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=j4EYMi3hESfB1xNFMZT+MbHgATz6XmpM38TvNqAyAd0=; b=NGrLfKb5p/1Jpq5v0Lo0wU/aaLRAuTqZ/zF1nkl2mJ9UAFmmNEBukOFOl4mDEGZGfu 2q3to23K5d4nwY6/brReDaRzGcW0znDzZHz8c0jb2cej/cW76ojTzC8NbV04FypdFLwr cLxsoLqH+14yjrNDeiB0m7rQvi5gsW+kDpL8BkEVZUiir5rXJEutWUhhbmZ1m05QN7Eg ncAxRm9MO2hv+isHN+wacZAQZj7Dcf7M0TlDBcCmPiLMTiVXqTA6Nb1HXufzC6dsjqOx Eurq9GEP+w4ICqnNAnvsxWcWD3scRVSA1ApSzUiuNcf5YbZZeogx9h7JXDLsRXYAAv10 VHCA== 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=j4EYMi3hESfB1xNFMZT+MbHgATz6XmpM38TvNqAyAd0=; b=n0vZFP2QCPb90SuF49NM9eRvhXP8dDPy5I3Sz5SN720zVdd2/Pnzr636aDx48PClyA 53omPygWOfD9lKkcxfBCMfqtatFKXKtMx/oQ0RMu1YviQ9N9UFzoncKrFoJ+4wsRQcYG 0JGe7yycrGRv0sBp96OCvKY4j4BPC39ICIF4UZyq/3ah9zb+mVduNCRwzX506nOpazt3 XofVlmfgAViBlgMd4wGHk/JsMXTpicaGoZnA6yeIDsjMAakbJEU2nz6Gp5ke6+dMk+/t OgcYgUbM9xWosPa02lGg4w9jSvTH9Uk9wvWDP0y78vZmo/vqUdmfnE9bKqo88PdJfYZR +Y8A== X-Gm-Message-State: AIVw113o0jw49et4TvJ26sAnIX3JfFAVFj54l0kjx0xXkh1p0lsl85Ah bZSjJxqcMrAj9fmV X-Received: by 10.107.165.13 with SMTP id o13mr10533583ioe.47.1499212811230; Tue, 04 Jul 2017 17:00:11 -0700 (PDT) Original-Received: from lylat (S010664777d9cebe3.ss.shawcable.net. [70.64.85.59]) by smtp.gmail.com with ESMTPSA id j12sm10508158iod.12.2017.07.04.17.00.09 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 04 Jul 2017 17:00:09 -0700 (PDT) In-Reply-To: <87zickhoco.fsf_-_@lylat> (Alex's message of "Mon, 03 Jul 2017 21:28:55 -0600") 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:134186 Archived-At: --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable tags 24402 patch quit Alex writes: > Gemini Lasswell writes: > >> I=E2=80=99ve 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))) >>=20=20=20 > > 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. --=-=-= Content-Type: text/x-diff Content-Disposition: attachment; filename=ert-args.diff Content-Description: first diff --git a/lisp/emacs-lisp/ert.el b/lisp/emacs-lisp/ert.el index 2c49a634e3..f5f3e30c0d 100644 --- a/lisp/emacs-lisp/ert.el +++ b/lisp/emacs-lisp/ert.el @@ -303,8 +303,12 @@ 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 (progn (setq ,fn #'signal) + (list (car err) + (cdr err))))))) (let ((,value ',default-value)) ,(funcall inner-expander `(setq ,value (apply ,fn ,args)) --=-=-= Content-Type: text/plain 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: --=-=-= Content-Type: text/x-diff Content-Disposition: attachment; filename=ert-args2.diff Content-Description: second diff --git a/lisp/emacs-lisp/ert.el b/lisp/emacs-lisp/ert.el index eb2b2e3e11..66ae31fd51 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,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 + (list ,@arg-forms) + ;; (ert-test-failed (signal (car err) (cdr err))) + (error (progn (setq ,fn #'signal) + (list (car err) + (cdr err))))))) (let ((,value ',default-value)) ,(funcall inner-expander `(setq ,value (apply ,fn ,args)) --=-=-= Content-Type: text/plain 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. --=-=-=--