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: Sat, 13 Aug 2022 21:52:22 -0700 Message-ID: <87k07bctq1.fsf@rfc20.org> References: <87ilmygjyv.fsf@rfc20.org> <87edxmgjke.fsf@rfc20.org> <83o7wqnfu8.fsf@gnu.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="30279"; 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 Sun Aug 14 06:53:16 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 1oN5ch-0007fC-Jm for geb-bug-gnu-emacs@m.gmane-mx.org; Sun, 14 Aug 2022 06:53:15 +0200 Original-Received: from localhost ([::1]:46724 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1oN5cf-0007KU-R2 for geb-bug-gnu-emacs@m.gmane-mx.org; Sun, 14 Aug 2022 00:53:13 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:37506) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oN5cU-0007KH-Uy for bug-gnu-emacs@gnu.org; Sun, 14 Aug 2022 00:53:02 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:45844) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1oN5cU-0006jE-LH for bug-gnu-emacs@gnu.org; Sun, 14 Aug 2022 00:53:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1oN5cU-00058E-F5 for bug-gnu-emacs@gnu.org; Sun, 14 Aug 2022 00:53: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: Sun, 14 Aug 2022 04:53: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.166045275819687 (code B ref 57150); Sun, 14 Aug 2022 04:53:02 +0000 Original-Received: (at 57150) by debbugs.gnu.org; 14 Aug 2022 04:52:38 +0000 Original-Received: from localhost ([127.0.0.1]:35593 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1oN5c5-00057S-78 for submit@debbugs.gnu.org; Sun, 14 Aug 2022 00:52:37 -0400 Original-Received: from relay6-d.mail.gandi.net ([217.70.183.198]:59569) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1oN5bz-00057A-U1 for 57150@debbugs.gnu.org; Sun, 14 Aug 2022 00:52:35 -0400 Original-Received: (Authenticated sender: matt@rfc20.org) by mail.gandi.net (Postfix) with ESMTPSA id 7E7F7C0002; Sun, 14 Aug 2022 04:52:25 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rfc20.org; s=gm1; t=1660452746; 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: in-reply-to:in-reply-to:references:references; bh=9wzNpJ19NPd8BUS2wDb8tn26Pcli63qnf3P8zsJVz74=; b=YlythECY2qATCNUka8EcgeBEYsRMEluhwwt9CbF+EjPtXK2ZsL5Bvj0MqWxYfvX4UeFG+q uLwuDSfBEXhkbhdpUMt3LadVCoIviAsNU2i9NdIIyZ3AqLrXRT9Zo8ovd3C4qS78wlf1BM AsIkFxDrYYq1zhLpp1USPTGo39j9qDNANe98VU6nKLtbZPrQgSokdnDtGvxF82Hud48BsW USBvgXeSaCWpGGmQ47VG4h1Bi9txZwVhLok/fuiX2qsEDM8SRYoPZDT1mbikpcJ1V7QzcO kW1wrPVf8+WxrnVkLdXJdrweGYQUB8DY4WPBh6oFNCNfOTH7ZeQpX8hLbDR6RA== Original-Received: from matt by naz with local (Exim 4.96) (envelope-from ) id 1oN5bq-000JA0-1m; Sat, 13 Aug 2022 21:52:22 -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:239611 Archived-At: --=-=-= Content-Type: text/plain Thanks Eli, another patch attached. More comments below. Eli Zaretskii writes: >> From: Matt Armstrong >> Date: Thu, 11 Aug 2022 21:41:21 -0700 >> >> Matt Armstrong writes: >> >> > I noticed there was no explicit test against the overlay modification >> > hook boundary conditions. The attached patch adds some, which might >> > be nice to have if a person (possibly me) someday optimizes overlay >> > performance with a change in data structure. >> > >> > In writing this I was surprised to learn that the hooks do not behave >> > differently if the overlay is `front-advance' or `rear-advance', so >> > this is explicitly tested. >> >> ...follow up with a comment typo fix. > > Thanks! Adding tests is always welcome, so this is fine, of course. > Please allow me a few minor comments, though. > > 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. The ERT framework and the tests themselves being highly > macro-ized don't help in that matter, either. So please try to > explain the idea of the tests and the details of the data structures > as much as is reasonably possible. In this case: I switched to an approach where each test case is specified by an alist with keys bound to variables with `let-alist'. This lets me log the test case "spec" with `message' while still conveniently unpacking the alist. I also switched to an approach where the "recorded" modification hook calls are stored on a property on the overlay itself, which then made it easy to extract this somewhat tricky code into helper functions. I added some message output, including a message printing the "test spec" alist before it executes. I verified that this new message output is part of the test log (as captured by "make check") and appears on the console for failed tests. I also left a few other "debug log" style messages in the test case that I found helpful during debugging. I don't feel strongly about leaving them in or not. I left a TODO, which is more of a question, about one test case that reveals a call to the modification hooks for a simple insert, which the elisp manual seems to suggest will not happen (because inserts do not modify characters)...perhaps the manual is worded too strongly? Am I misunderstanding something? Ironically, after all this was done, I am left wondering if a well designed macro fixture wouldn't be more clear, simply because it does produce a separate ERT test for each test case. Perhaps there are ways to design macro test fixtures in clear ways to avoid the downsides of the way they appear in the other buffer-tests.el tests today.... > This means you don't test so-called "empty" overlays, whose START and > END are identical. They are handled specially by the low-level > support code. Maybe add more tests for that? I added one test case for an empty overlay. I'm not confident I've covered all of the interesting cases, but nothing obvious occurred to me. --=-=-= Content-Type: text/x-diff Content-Disposition: inline; filename=0001-Add-basic-test-coverage-for-overlay-modification-hoo.patch >From 3a3fbf96ce64e8a40a849c3b3f8453614a92b728 Mon Sep 17 00:00:00 2001 From: Matt Armstrong Date: Thu, 11 Aug 2022 21:11:36 -0700 Subject: [PATCH] Add basic test coverage for overlay modification hooks * test/src/buffer-tests.el: (overlay-modification-hooks) new ert-deftest. (overlay-tests-start-recording-modification-hooks): New function. (overlay-tests-get-recorded-modification-hooks): New function. --- test/src/buffer-tests.el | 193 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 193 insertions(+) diff --git a/test/src/buffer-tests.el b/test/src/buffer-tests.el index 3c6a9208ff..558d05de14 100644 --- a/test/src/buffer-tests.el +++ b/test/src/buffer-tests.el @@ -22,6 +22,199 @@ (require 'ert) (require 'ert-x) (require 'cl-lib) +(require 'let-alist) + +(defun overlay-tests-start-recording-modification-hooks (overlay) + "Start recording modification hooks on OVERLAY. + +Always overwrites the `insert-in-front-hooks', +`modification-hooks' and `insert-behind-hooks' properties. Any +recorded history from a previous call is erased. + +The history is stored in a property on the overlay itself. Call +`overlay-tests-get-recorded-modification-hooks' to retrieve the +recorded calls conveniently." + (dolist (hooks-property '(insert-in-front-hooks + modification-hooks + insert-behind-hooks)) + (overlay-put + overlay + hooks-property + (list (lambda (ov &rest args) + (message " %S called on %S with args %S" hooks-property ov args) + (should inhibit-modification-hooks) + (should (eq ov overlay)) + (push (list hooks-property args) + (overlay-get overlay + 'recorded-modification-hook-calls))))) + (overlay-put overlay 'recorded-modification-hook-calls nil))) + +(defun overlay-tests-get-recorded-modification-hooks (overlay) + "Extract the recorded calls made to modification hooks on OVERLAY. + +Must be preceded by a call to +`overlay-tests-start-recording-modification-hooks' on OVERLAY. + +Returns a list. Each element of the list represents a recorded +call to a particular modification hook. + +Each call is itself a sub-list where the first element is a +symbol matching the modification hook property (one of +`insert-in-front-hooks', `modification-hooks' or +`insert-behind-hooks') and the second element is the list of +arguments passed to the hook. The first hook argument, the +overlay itself, is omitted to make test result verification +easier." + (reverse (overlay-get overlay + 'recorded-modification-hook-calls))) + +(ert-deftest overlay-modification-hooks () + "Test the basic functionality of overlay modification hooks. + +This exercises hooks registered on the `insert-in-front-hooks', +`modification-hooks' and `insert-behind-hooks' overlay +properties." + ;; This is a data driven test loop. Each test case is described + ;; by an alist. The test loop initializes a new temporary buffer + ;; for each case, creates an overlay, registers modification hooks + ;; on the overlay, modifies the buffer, and then verifies which + ;; modification hooks (if any) were called for the overlay, as + ;; well as which arguments were passed to the hooks. + ;; + ;; The following keys are available in the alist: + ;; + ;; `buffer-text': the initial buffer text of the temporary buffer. + ;; Defaults to "1234". + ;; + ;; `overlay-beg' and `overlay-end': the begin and end positions of + ;; the overlay under test. Defaults to 2 and 4 respectively. + ;; + ;; `insert-at': move to the given position and insert the string + ;; "x" into the test case's buffer. + ;; + ;; `replace': replace the first occurrence of the given string in + ;; the test case's buffer with "x". The test will fail if the + ;; string is not found. + ;; + ;; `expected-calls': a description of the expected buffer + ;; modification hooks. See + ;; `overlay-tests-get-recorded-modification-hooks' for the format. + ;; May be omitted, in which case the test will insist that no + ;; modification hooks are called. + ;; + ;; The test will fail itself in the degenerate case where no + ;; buffer modifications are requested. + (dolist (test-case + '( + ;; Remember that the default buffer text is "1234" and + ;; the default overlay begins at position 2 and ends at + ;; position 4. Most of the test cases below assume + ;; this. + + ;; TODO: (info "(elisp) Special Properties") says this + ;; about `modification-hooks': "Furthermore, insertion + ;; will not modify any existing character, so this hook + ;; will only be run when removing some characters, + ;; replacing them with others, or changing their + ;; text-properties." So, why are modification-hooks + ;; being called when inserting at position 3 below? + ((insert-at . 1)) + ((insert-at . 2) + (expected-calls . ((insert-in-front-hooks (nil 2 2)) + (insert-in-front-hooks (t 2 3 0))))) + ((insert-at . 3) + (expected-calls . ((modification-hooks (nil 3 3)) + (modification-hooks (t 3 4 0))))) + ((insert-at . 4) + (expected-calls . ((insert-behind-hooks (nil 4 4)) + (insert-behind-hooks (t 4 5 0))))) + ((insert-at . 5)) + + ;; Replacing text never calls `insert-in-front-hooks' + ;; or `insert-behind-hooks'. It calls + ;; `modification-hooks' if the overlay covers any text + ;; that has changed. + ((replace . "1")) + ((replace . "2") + (expected-calls . ((modification-hooks (nil 2 3)) + (modification-hooks (t 2 3 1))))) + ((replace . "3") + (expected-calls . ((modification-hooks (nil 3 4)) + (modification-hooks (t 3 4 1))))) + ((replace . "4")) + ((replace . "12") + (expected-calls . ((modification-hooks (nil 1 3)) + (modification-hooks (t 1 2 2))))) + ((replace . "23") + (expected-calls . ((modification-hooks (nil 2 4)) + (modification-hooks (t 2 3 2))))) + ((replace . "34") + (expected-calls . ((modification-hooks (nil 3 5)) + (modification-hooks (t 3 4 2))))) + ((replace . "123") + (expected-calls . ((modification-hooks (nil 1 4)) + (modification-hooks (t 1 2 3))))) + ((replace . "234") + (expected-calls . ((modification-hooks (nil 2 5)) + (modification-hooks (t 2 3 3))))) + ((replace . "1234") + (expected-calls . ((modification-hooks (nil 1 5)) + (modification-hooks (t 1 2 4))))) + + ;; Inserting at the position of a zero-length overlay + ;; calls both `insert-in-front-hooks' and + ;; `insert-behind-hooks'. + ((buffer-text . "") (overlay-beg . 1) (overlay-end . 1) + (insert-at . 1) + (expected-calls . ((insert-in-front-hooks + (nil 1 1)) + (insert-behind-hooks + (nil 1 1)) + (insert-in-front-hooks + (t 1 2 0)) + (insert-behind-hooks + (t 1 2 0))))))) + (message "BEGIN overlay-modification-hooks test-case %S" test-case) + + ;; All three hooks ignore the overlay's `front-advance' and + ;; `rear-advance' option, so test both ways while expecting the same + ;; result. + (dolist (advance '(nil t)) + (message " advance is %S" advance) + (let-alist test-case + (with-temp-buffer + ;; Set up the temporary buffer and overlay as specified by + ;; the test case. + (insert (or .buffer-text "1234")) + (let ((overlay (make-overlay + (or .overlay-beg 2) + (or .overlay-end 4) + nil + advance advance))) + (message " (buffer-string) is %S" (buffer-string)) + (message " overlay is %S" overlay) + (overlay-tests-start-recording-modification-hooks overlay) + + ;; Modify the buffer, possibly inducing calls to the + ;; overlay's modification hooks. + (should (or .insert-at .replace)) + (when .insert-at + (goto-char .insert-at) + (insert "x") + (message " inserted \"x\" at %S, buffer-string now %S" + .insert-at (buffer-string))) + (when .replace + (goto-char (point-min)) + (search-forward .replace) + (replace-match "x") + (message " replaced %S with \"x\"" .replace)) + + ;; Verify that the expected and actual modification hook + ;; calls match. + (should (equal + .expected-calls + (overlay-tests-get-recorded-modification-hooks + overlay))))))))) (ert-deftest overlay-modification-hooks-message-other-buf () "Test for bug#21824. -- 2.35.1 --=-=-=--