From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Matt Armstrong Newsgroups: gmane.emacs.bugs Subject: bug#57150: 29.0.50; [PATCH] Add test coverage for overlay modification hooks Date: Fri, 12 Aug 2022 10:57:07 -0700 Message-ID: <874jyhgxak.fsf@rfc20.org> References: <87ilmygjyv.fsf@rfc20.org> <87edxmgjke.fsf@rfc20.org> <83o7wqnfu8.fsf@gnu.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="2606"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 57150@debbugs.gnu.org To: Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Fri Aug 12 19:58:47 2022 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1oMYvk-0000Qs-QI for geb-bug-gnu-emacs@m.gmane-mx.org; Fri, 12 Aug 2022 19:58:45 +0200 Original-Received: from localhost ([::1]:39756 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1oMYvj-0006Ts-Sh for geb-bug-gnu-emacs@m.gmane-mx.org; Fri, 12 Aug 2022 13:58:43 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:52006) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oMYv4-0006S3-M1 for bug-gnu-emacs@gnu.org; Fri, 12 Aug 2022 13:58:07 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:40908) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1oMYv4-0004tP-Am for bug-gnu-emacs@gnu.org; Fri, 12 Aug 2022 13:58:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1oMYv4-0001y7-3g for bug-gnu-emacs@gnu.org; Fri, 12 Aug 2022 13:58:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Matt Armstrong Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Fri, 12 Aug 2022 17:58:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 57150 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 57150-submit@debbugs.gnu.org id=B57150.16603270397517 (code B ref 57150); Fri, 12 Aug 2022 17:58:02 +0000 Original-Received: (at 57150) by debbugs.gnu.org; 12 Aug 2022 17:57:19 +0000 Original-Received: from localhost ([127.0.0.1]:58890 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1oMYuM-0001xB-Cy for submit@debbugs.gnu.org; Fri, 12 Aug 2022 13:57:18 -0400 Original-Received: from relay6-d.mail.gandi.net ([217.70.183.198]:49855) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1oMYuK-0001wx-G0 for 57150@debbugs.gnu.org; Fri, 12 Aug 2022 13:57:17 -0400 Original-Received: (Authenticated sender: matt@rfc20.org) by mail.gandi.net (Postfix) with ESMTPSA id 5464FC0003; Fri, 12 Aug 2022 17:57:09 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rfc20.org; s=gm1; t=1660327030; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=sRIBly5bB7NEEmAZGVSsEb0CtBIvRFD75PXZYlyxs/0=; b=Y+y6IqmO/Df7rhEDu8nzNCD0zAExdTMDuL9VFirLlB9lFAjfxWmqoB4kJBpbXD8miMm8zn CWI2+JFVXqMzmOcIcgUlJztYgZ62NCuvYiKhRXsBKoctnXt+d1IqJA6f21qBL4ZwokGiIH pzs5u3ctuyfjTwKAIyzlhr5fLSaa1PEu1BvjAbNuSi3gkcX1UMm9DkCXZIoK4Z6lkMCNoo fvwYW1FKbLZEufH8xrUDI5SmKPJZFpe4shtcZLEaczvGEfbt/VPwSvlLjbKdZkTJ+PJ+vG wbHmJhhpHdC9vcpgc71rgHq2vxMcKHXfgVEXrZMAFBjnXzkGeAgUPQvr+2a/xQ== Original-Received: from matt by naz with local (Exim 4.96) (envelope-from ) id 1oMYuB-000Fuu-0X; Fri, 12 Aug 2022 10:57:07 -0700 In-Reply-To: <83o7wqnfu8.fsf@gnu.org> X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list 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-mx.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.io gmane.emacs.bugs:239454 Archived-At: Eli Zaretskii writes: > A frequent gripe of mine about our test-suite tests is that the ideas > of the tests are not sufficiently explained, where they aren't 110% > obvious. I enthusiastically agree. I will work on making these tests more clear. First I'd like your opinion on a few general questions. [...] > So please also think about this issue, and see if there's any way of > making the failing tests tell more explicitly which part failed. > E.g., maybe each member in your test data structure should have a > unique ID (a name or an ordinal number), which could then be used in a > way that would display that ID with the failure message. Or > something. Two specific questions: Do you have a preference between the two general approaches to exercising many similar test cases: data driven vs. macro driven? If data driven tests are preferred, what do you think of using 'message' to log the individual test case details, as a way of knowing which part failed? I usually favor data driven tests. With this approach, everything can often be expressed in a single function. Short of that, the usual lisp constructs can be composed in simple ways (regular functions, lambdas, etc). I usually don't like macro driven test case generation because the pattern often obscures the connection between the inputs and the test case logic and expected results. Turning to your specific question about ways of "making the failing tests tell more explicitly which part failed". Other test frameworks have scoped based context messages. In pseudocode, something like: (ert-deftest example () (dolist arg '(a b c d) (context (arg) (should (equal arg (identity-function arg)))))) Printed failures would include both the normal 'should' output, but also the values passed to 'context'. I notice that ERT does have two interactive features that partially address this. These commands are available in its UI: =E2=80=98l=E2=80=99 Show the list of =E2=80=98should=E2=80=99 forms executed in the test (=E2=80=98ert-results-pop-to-should-forms-for-test-at-point=E2=80=99). =E2=80=98m=E2=80=99 Show any messages that were generated (with the Lisp function =E2=80=98message=E2=80=99) in a test or any of the code that it invoked (=E2=80=98ert-results-pop-to-messages-for-test-at-point=E2=80=99). In simpler tests I find that 'l' is enough, since I can count the 'should' calls to work out the iteration of whatever loop is used by the test. In more complex cases, perhaps using 'message' to display the context is enough? If you don't think 'l' and 'm' are *not* good enough, I might agree with you. If you think adding something like 'context' to ERT is worthwhile I can look at doing that. For this patch, perhaps using 'message' is best? In the case of the macro-generated tests in buffer-tests.el I think they are too complex. For example: (deftest-make-overlay-1 A (1 1)) expands to: (ert-deftest buffer-tests--make-overlay-1-A nil (with-temp-buffer (should (make-overlay 1 1)))) I see two clear disadvantages to the macro. First, if ERT reports that 'buffer-tests--make-overlay-1-A' fails it isn't obvious where that specific test is defined in code. The next person coming along cannot simply search for "buffer-tests--make-overlay-1-A". Second, it is not clear what 'deftest-make-overlay-1' does. In this case, the test cases are defined 150 lines away from where the macro is defined. Compare the two approaches: (deftest-make-overlay-1 A (1 1)) (deftest-make-overlay-1 B (7 26)) (deftest-make-overlay-1 C (29 7)) (deftest-make-overlay-1 D (most-positive-fixnum 1)) (deftest-make-overlay-1 E (most-negative-fixnum 1)) (deftest-make-overlay-1 F (1 most-positive-fixnum)) (deftest-make-overlay-1 G (1 most-negative-fixnum)) (deftest-make-overlay-1 H (1 1 nil t)) (deftest-make-overlay-1 I (1 1 nil nil)) (deftest-make-overlay-1 J (1 1 nil nil nil)) (deftest-make-overlay-1 K (1 1 nil nil t)) (deftest-make-overlay-1 L (1 1 nil t t)) (deftest-make-overlay-1 M (1 1 nil "yes" "yes")) (ert-deftest make-overlay-success-cases () (dolist (args '((1 1) (7 26) (29 7) (most-positive-fixnum 1) (most-negative-fixnum 1) (1 most-positive-fixnum) (1 most-negative-fixnum) (1 1 nil t) (1 1 nil nil) (1 1 nil nil nil) (1 1 nil nil t) (1 1 nil t t) (1 1 nil "yes" "yes"))) (message "Testing %s" (cons 'make-overlay args)) (with-temp-buffer (should (funcall #'make-overlay args))))) If you think the second form is clearer I can work on that transformation (or not, but still avoid macro-generated tests in new code). > P.S. I do indeed hope that we merge the branch with the faster > implementation of overlays some time soon, as overlays seem more and > more as a significant slowdown factor in Emacs. > > Thanks again for working on this. It is slow going!