GNU bug report logs - #57150
29.0.50; [PATCH] Add test coverage for overlay modification hooks

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 57150 in the body.
You can then email your comments to 57150 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Matt Armstrong <matt <at> rfc20.org>
To: bug-gnu-emacs <at> gnu.org
Subject: 29.0.50; [PATCH] Add test coverage for overlay modification hooks
Date: Thu, 11 Aug 2022 21:32:40 -0700

[Message part 1 (text/plain, inline)]

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.

[0001-Add-test-coverage-for-overlay-modification-hooks.patch (text/x-diff, inline)]

From 9083709cbe12ab97b795bcac8343f9d11f2929b7 Mon Sep 17 00:00:00 2001
From: Matt Armstrong <matt <at> rfc20.org>
Date: Thu, 11 Aug 2022 21:11:36 -0700
Subject: [PATCH] Add test coverage for overlay modification hooks

* test/src/buffer-tests.el: (overlay-modification-hooks) new
ert-deftest.
---
 test/src/buffer-tests.el | 66 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/test/src/buffer-tests.el b/test/src/buffer-tests.el
index 3c6a9208ff..ee25b43f9e 100644
--- a/test/src/buffer-tests.el
+++ b/test/src/buffer-tests.el
@@ -22,6 +22,72 @@
 (require 'ert)
 (require 'ert-x)
 (require 'cl-lib)
+(require 'pcase)
+
+(ert-deftest overlay-modification-hooks ()
+  "Test the `modification-hooks' overlay property.
+
+This also tests `insert-in-front-hooks' and `insert-behind-hooks'."
+  ;; Perform operation against the given needle (a position or string)
+  ;; and expect the `modification-hook', `insert-in-front-hooks' and
+  ;; `insert-behind-hooks' to be called with the given arguments.  All
+  ;; tests are performed against the same fixed buffer and overlay.
+  (pcase-dolist (`(,operation
+                   ,needle
+                   ,expected-insert-in-front-calls
+                   ,expected-modification-calls
+                   ,expected-insert-behind-calls)
+                 '((insert 1 nil nil nil)
+                   (insert 2 ((nil 2 2) (t 2 3 0)) nil nil)
+                   (insert 3 nil ((nil 3 3) (t 3 4 0)) nil)
+                   (insert 4 nil nil ((nil 4 4) (t 4 5 0)))
+                   (insert 5 nil nil nil)
+                   (replace "1" nil nil nil)
+                   (replace "2" nil ((nil 2 3) (t 2 3 1)) nil)
+                   (replace "3" nil ((nil 3 4) (t 3 4 1)) nil)
+                   (replace "4" nil nil nil)
+                   (replace "12" nil ((nil 1 3) (t 1 2 2)) nil)
+                   (replace "23" nil ((nil 2 4) (t 2 3 2)) nil)
+                   (replace "34" nil ((nil 3 5) (t 3 4 2)) nil)
+                   (replace "123" nil ((nil 1 4) (t 1 2 3)) nil)
+                   (replace "234" nil ((nil 2 5) (t 2 3 3)) nil)
+                   (replace "1234" nil ((nil 1 5) (t 1 2 4)) nil)))
+    ;; All three hooks ignore the overlay's `front-advance' and
+    ;; `rear-advance' option, so test ways and expect the same result.
+    (dolist (advance '(nil t))
+      (with-temp-buffer
+        (insert "1234")
+        (let (modification-calls insert-in-front-calls insert-behind-calls
+              (overlay (make-overlay 2 4 nil advance advance)))
+          (overlay-put overlay
+                       'modification-hooks
+                       (list (lambda (ov &rest args)
+                               (should inhibit-modification-hooks)
+                               (should (eq ov overlay))
+                               (push args modification-calls))))
+          (overlay-put overlay
+                       'insert-in-front-hooks
+                       (list (lambda (ov &rest args)
+                               (should inhibit-modification-hooks)
+                               (should (eq ov overlay))
+                               (push args insert-in-front-calls))))
+          (overlay-put overlay
+                       'insert-behind-hooks
+                       (list (lambda (ov &rest args)
+                               (should inhibit-modification-hooks)
+                               (should (eq ov overlay))
+                               (push args insert-behind-calls))))
+          (pcase-exhaustive operation
+            (`insert (goto-char needle)
+                     (insert "x"))
+            (`replace (goto-char (point-min))
+                      (replace-string needle "x")))
+          (should (equal (reverse insert-in-front-calls)
+                         expected-insert-in-front-calls))
+          (should (equal (reverse modification-calls)
+                         expected-modification-calls))
+          (should (equal (reverse insert-behind-calls)
+                         expected-insert-behind-calls)))))))
 
 (ert-deftest overlay-modification-hooks-message-other-buf ()
   "Test for bug#21824.
-- 
2.35.1

Message #8 received at 57150 <at> debbugs.gnu.org (full text, mbox):

From: Matt Armstrong <matt <at> rfc20.org>
To: 57150 <at> debbugs.gnu.org
Subject: Re: bug#57150: 29.0.50; [PATCH] Add test coverage for overlay
 modification hooks
Date: Thu, 11 Aug 2022 21:41:21 -0700

[Message part 1 (text/plain, inline)]

Matt Armstrong <matt <at> rfc20.org> 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.

[0001-Add-test-coverage-for-overlay-modification-hooks.patch (text/x-diff, inline)]

From 372c894e8d33eb5271b6c19b2e2e2843197a7353 Mon Sep 17 00:00:00 2001
From: Matt Armstrong <matt <at> rfc20.org>
Date: Thu, 11 Aug 2022 21:11:36 -0700
Subject: [PATCH] Add test coverage for overlay modification hooks

* test/src/buffer-tests.el: (overlay-modification-hooks) new
ert-deftest.
---
 test/src/buffer-tests.el | 67 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/test/src/buffer-tests.el b/test/src/buffer-tests.el
index 3c6a9208ff..381d8618aa 100644
--- a/test/src/buffer-tests.el
+++ b/test/src/buffer-tests.el
@@ -22,6 +22,73 @@
 (require 'ert)
 (require 'ert-x)
 (require 'cl-lib)
+(require 'pcase)
+
+(ert-deftest overlay-modification-hooks ()
+  "Test the `modification-hooks' overlay property.
+
+This also tests `insert-in-front-hooks' and `insert-behind-hooks'."
+  ;; Perform operation against the given needle (a position or string)
+  ;; and expect the `modification-hook', `insert-in-front-hooks' and
+  ;; `insert-behind-hooks' to be called with the given arguments.  All
+  ;; tests are performed against the same fixed buffer and overlay.
+  (pcase-dolist (`(,operation
+                   ,needle
+                   ,expected-insert-in-front-calls
+                   ,expected-modification-calls
+                   ,expected-insert-behind-calls)
+                 '((insert 1 nil nil nil)
+                   (insert 2 ((nil 2 2) (t 2 3 0)) nil nil)
+                   (insert 3 nil ((nil 3 3) (t 3 4 0)) nil)
+                   (insert 4 nil nil ((nil 4 4) (t 4 5 0)))
+                   (insert 5 nil nil nil)
+                   (replace "1" nil nil nil)
+                   (replace "2" nil ((nil 2 3) (t 2 3 1)) nil)
+                   (replace "3" nil ((nil 3 4) (t 3 4 1)) nil)
+                   (replace "4" nil nil nil)
+                   (replace "12" nil ((nil 1 3) (t 1 2 2)) nil)
+                   (replace "23" nil ((nil 2 4) (t 2 3 2)) nil)
+                   (replace "34" nil ((nil 3 5) (t 3 4 2)) nil)
+                   (replace "123" nil ((nil 1 4) (t 1 2 3)) nil)
+                   (replace "234" nil ((nil 2 5) (t 2 3 3)) nil)
+                   (replace "1234" nil ((nil 1 5) (t 1 2 4)) nil)))
+    ;; All three hooks ignore the overlay's `front-advance' and
+    ;; `rear-advance' option, so test both ways and expect the same
+    ;; result.
+    (dolist (advance '(nil t))
+      (with-temp-buffer
+        (insert "1234")
+        (let (modification-calls insert-in-front-calls insert-behind-calls
+              (overlay (make-overlay 2 4 nil advance advance)))
+          (overlay-put overlay
+                       'modification-hooks
+                       (list (lambda (ov &rest args)
+                               (should inhibit-modification-hooks)
+                               (should (eq ov overlay))
+                               (push args modification-calls))))
+          (overlay-put overlay
+                       'insert-in-front-hooks
+                       (list (lambda (ov &rest args)
+                               (should inhibit-modification-hooks)
+                               (should (eq ov overlay))
+                               (push args insert-in-front-calls))))
+          (overlay-put overlay
+                       'insert-behind-hooks
+                       (list (lambda (ov &rest args)
+                               (should inhibit-modification-hooks)
+                               (should (eq ov overlay))
+                               (push args insert-behind-calls))))
+          (pcase-exhaustive operation
+            (`insert (goto-char needle)
+                     (insert "x"))
+            (`replace (goto-char (point-min))
+                      (replace-string needle "x")))
+          (should (equal (reverse insert-in-front-calls)
+                         expected-insert-in-front-calls))
+          (should (equal (reverse modification-calls)
+                         expected-modification-calls))
+          (should (equal (reverse insert-behind-calls)
+                         expected-insert-behind-calls)))))))
 
 (ert-deftest overlay-modification-hooks-message-other-buf ()
   "Test for bug#21824.
-- 
2.35.1

Message #11 received at 57150 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Matt Armstrong <matt <at> rfc20.org>
Cc: 57150 <at> debbugs.gnu.org
Subject: Re: bug#57150: 29.0.50;
 [PATCH] Add test coverage for overlay modification hooks
Date: Fri, 12 Aug 2022 09:19:59 +0300

> From: Matt Armstrong <matt <at> rfc20.org>
> Date: Thu, 11 Aug 2022 21:41:21 -0700
> 
> Matt Armstrong <matt <at> rfc20.org> 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:

> +This also tests `insert-in-front-hooks' and `insert-behind-hooks'."
> +  ;; Perform operation against the given needle (a position or string)
> +  ;; and expect the `modification-hook', `insert-in-front-hooks' and
> +  ;; `insert-behind-hooks' to be called with the given arguments.  All
> +  ;; tests are performed against the same fixed buffer and overlay.

This is good, but it still lacks some details.

First, "tests are performed against the same fixed buffer and overlay"
is AFAIU inaccurate: pcase-dolist will create a new temporary buffer
and a new overlay for each test.  I guess the "all tests" part needs
some qualification?

Next, the names of the data structure elements, viz.:

> +  (pcase-dolist (`(,operation
> +                   ,needle
> +                   ,expected-insert-in-front-calls
> +                   ,expected-modification-calls
> +                   ,expected-insert-behind-calls)

are quite descriptive, but then one sees stuff like this:

> +                   (insert 2 ((nil 2 2) (t 2 3 0)) nil nil)
> +                   (insert 3 nil ((nil 3 3) (t 3 4 0)) nil)
> +                   (insert 4 nil nil ((nil 4 4) (t 4 5 0)))

and wonders what's the semantics and meanings of the elements of the
expected-* members that aren't atoms.  I don't see this explained
anywhere.

> +              (overlay (make-overlay 2 4 nil advance advance)))

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?

Finally -- and that is another gripe of mine wrt our test suite --
when one of the tests fails, it is many times very hard to understand
which code failed, because there are almost no unique identifiers of
each 'should' or 'should-not' that are tested, and also because ERT
runs the tests in the order that is different from the code order.
The failure message shows the failing condition, which is good, but
due to highly macro-ized form of the code, it is many times very hard
to correlate the form that failed with the actual code, especially
since our tests tend to have very similar forms in different tests of
the same test source file.

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.

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.

Message #14 received at 57150 <at> debbugs.gnu.org (full text, mbox):

From: Matt Armstrong <matt <at> rfc20.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 57150 <at> debbugs.gnu.org
Subject: Re: bug#57150: 29.0.50; [PATCH] Add test coverage for overlay
 modification hooks
Date: Fri, 12 Aug 2022 10:57:07 -0700

Eli Zaretskii <eliz <at> gnu.org> 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:

‘l’
     Show the list of ‘should’ forms executed in the test
     (‘ert-results-pop-to-should-forms-for-test-at-point’).

‘m’
     Show any messages that were generated (with the Lisp function
     ‘message’) in a test or any of the code that it invoked
     (‘ert-results-pop-to-messages-for-test-at-point’).

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!

Message #17 received at 57150 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Matt Armstrong <matt <at> rfc20.org>
Cc: 57150 <at> debbugs.gnu.org
Subject: Re: bug#57150: 29.0.50; [PATCH] Add test coverage for overlay
 modification hooks
Date: Fri, 12 Aug 2022 21:53:23 +0300

> From: Matt Armstrong <matt <at> rfc20.org>
> Cc: 57150 <at> debbugs.gnu.org
> Date: Fri, 12 Aug 2022 10:57:07 -0700
> 
> Do you have a preference between the two general approaches to
> exercising many similar test cases: data driven vs. macro driven?

I tend to like the data-driven approach better, but I have nothing
against macros, assuming they solve the issues that I described.

> 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 have no problems with that, assuming that the text emitted by
'message' is visible when running the tests in batch mode.

> 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:
> 
> ‘l’
>      Show the list of ‘should’ forms executed in the test
>      (‘ert-results-pop-to-should-forms-for-test-at-point’).
> 
> ‘m’
>      Show any messages that were generated (with the Lisp function
>      ‘message’) in a test or any of the code that it invoked
>      (‘ert-results-pop-to-messages-for-test-at-point’).
> 
> 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.

I'm not sure I understand how these commands are relevant.  I'm
talking about running the tests in batch mode.  How do I make use of
those commands in that case?

> 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?

Anything is fine with me, if it shows enough information to identify
the particular "should" test that failed in the code.

Thanks.

Message #20 received at 57150 <at> debbugs.gnu.org (full text, mbox):

From: Ihor Radchenko <yantar92 <at> gmail.com>
To: Matt Armstrong <matt <at> rfc20.org>
Cc: 57150 <at> debbugs.gnu.org
Subject: Re: bug#57150: 29.0.50; [PATCH] Add test coverage for overlay
 modification hooks
Date: Sat, 13 Aug 2022 12:07:49 +0800

Matt Armstrong <matt <at> rfc20.org> 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.

Just in case. Have you seen
https://lists.gnu.org/archive/html/emacs-devel/2019-12/msg00323.html
?

I am not sure how much progress have been made on the noverlay branch,
but there was at least some existing test suitcase in there. Not sure if
it has been merged or if your proposed patch is not covered.

Best,
Ihor

Message #23 received at 57150 <at> debbugs.gnu.org (full text, mbox):

From: Matt Armstrong <matt <at> rfc20.org>
To: Ihor Radchenko <yantar92 <at> gmail.com>
Cc: 57150 <at> debbugs.gnu.org
Subject: Re: bug#57150: 29.0.50; [PATCH] Add test coverage for overlay
 modification hooks
Date: Sat, 13 Aug 2022 17:44:59 -0700

Ihor Radchenko <yantar92 <at> gmail.com> writes:

> Matt Armstrong <matt <at> rfc20.org> 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.
>
> Just in case. Have you seen
> https://lists.gnu.org/archive/html/emacs-devel/2019-12/msg00323.htlm
> ?
>
> I am not sure how much progress have been made on the noverlay branch,
> but there was at least some existing test suitcase in there. Not sure if
> it has been merged or if your proposed patch is not covered.

Yes, thank you for pointing that out.  I have gone as far as importing
the entire history of emacs-devel into https://notmuchmail.org/ and read
as much history about the overlay problem as I could discover.

I have been in contact with both Andreas (the original feature/noverlay
author) and Vladimir (who later examined it).  Andreas had no plans to
return to looking at the problem and did not recall the precise state
the branch was left in.  Vladimir, despite his interest, was
unfortunately not able to get his employer to sign FSF papers.  Both
wished me luck.

Fortunately all of the test cases from the feature/noverlay branch were
merged to the master branch, I believe by Eli, shortly after they were
written years ago.

As for feature/noverlay itself, I have been exploring alternative
approaches that might be simpler or clearer than Andreas' approach, with
the clear advantage of seeing his implementation, of course.  Failing
any improvement I can always start right where he left off.

Message #26 received at 57150 <at> debbugs.gnu.org (full text, mbox):

From: Matt Armstrong <matt <at> rfc20.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 57150 <at> debbugs.gnu.org
Subject: Re: bug#57150: 29.0.50; [PATCH] Add test coverage for overlay
 modification hooks
Date: Sat, 13 Aug 2022 21:52:22 -0700

[Message part 1 (text/plain, inline)]

Thanks Eli, another patch attached.  More comments below.


Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Matt Armstrong <matt <at> rfc20.org>
>> Date: Thu, 11 Aug 2022 21:41:21 -0700
>> 
>> Matt Armstrong <matt <at> rfc20.org> 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.

[0001-Add-basic-test-coverage-for-overlay-modification-hoo.patch (text/x-diff, inline)]

From 3a3fbf96ce64e8a40a849c3b3f8453614a92b728 Mon Sep 17 00:00:00 2001
From: Matt Armstrong <matt <at> rfc20.org>
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

Message #29 received at 57150 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Matt Armstrong <matt <at> rfc20.org>
Cc: 57150 <at> debbugs.gnu.org
Subject: Re: bug#57150: 29.0.50; [PATCH] Add test coverage for overlay
 modification hooks
Date: Sun, 14 Aug 2022 10:15:08 +0300

> From: Matt Armstrong <matt <at> rfc20.org>
> Cc: 57150 <at> debbugs.gnu.org
> Date: Sat, 13 Aug 2022 21:52:22 -0700
> 
> Thanks Eli, another patch attached.  More comments below.

This is very good, thanks!

> 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....

It is okay to make several tests in a single ert-deftest test, as long
as each failing test is clearly explained and easily correlated to the
source code that failed.

Message #32 received at 57150 <at> debbugs.gnu.org (full text, mbox):

From: Ihor Radchenko <yantar92 <at> gmail.com>
To: Matt Armstrong <matt <at> rfc20.org>
Cc: 57150 <at> debbugs.gnu.org
Subject: Re: bug#57150: 29.0.50; [PATCH] Add test coverage for overlay
 modification hooks
Date: Mon, 15 Aug 2022 12:14:37 +0800

Matt Armstrong <matt <at> rfc20.org> writes:

> As for feature/noverlay itself, I have been exploring alternative
> approaches that might be simpler or clearer than Andreas' approach, with
> the clear advantage of seeing his implementation, of course.  Failing
> any improvement I can always start right where he left off.

This will be most welcome. At least, Org mode can greatly benefit from
faster overlays.

-- 
Ihor Radchenko,
Org mode contributor,
Learn more about Org mode at https://orgmode.org/.
Support Org development at https://liberapay.com/org-mode,
or support my work at https://liberapay.com/yantar92

Message #35 received at 57150 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Ihor Radchenko <yantar92 <at> gmail.com>
Cc: matt <at> rfc20.org, 57150 <at> debbugs.gnu.org
Subject: Re: bug#57150: 29.0.50;
 [PATCH] Add test coverage for overlay modification hooks
Date: Mon, 15 Aug 2022 14:26:23 +0300

> Cc: 57150 <at> debbugs.gnu.org
> From: Ihor Radchenko <yantar92 <at> gmail.com>
> Date: Mon, 15 Aug 2022 12:14:37 +0800
> 
> At least, Org mode can greatly benefit from faster overlays.

The problems with overlays in Emacs are much wider than just Org.  Our
performance sucks in many cases because of that.  Just two examples
that were mentioned lately in the "long-line optimizations"
discussion: show-paren-mode and isearch-lazy-highlight -- each one of
these two is capable of literally bringing Emacs to its knees,
response-time wise.

Message #38 received at 57150 <at> debbugs.gnu.org (full text, mbox):

From: Richard Stallman <rms <at> gnu.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: matt <at> rfc20.org, yantar92 <at> gmail.com, 57150 <at> debbugs.gnu.org
Subject: Re: bug#57150: 29.0.50;
 [PATCH] Add test coverage for overlay modification hooks
Date: Tue, 16 Aug 2022 22:50:38 -0400

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > The problems with overlays in Emacs are much wider than just Org.  Our
  > performance sucks in many cases because of that.  Just two examples
  > that were mentioned lately in the "long-line optimizations"
  > discussion: show-paren-mode and isearch-lazy-highlight -- each one of
  > these two is capable of literally bringing Emacs to its knees,
  > response-time wise.

The first question is, should these use text properties instead of
overlays?

Are they using many overlays to highlight many potential matches in a
long buffer?  If so, would it make sense to show, at any time, only those
that are near point?  When you move to a far-away part of the buffer,
it could add highlighting to the matches near there.  But maybe it
can avoid ever trying to highlight more than a few parts of the buffer.

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

Message #41 received at 57150 <at> debbugs.gnu.org (full text, mbox):

From: Ihor Radchenko <yantar92 <at> gmail.com>
To: rms <at> gnu.org
Cc: matt <at> rfc20.org, Eli Zaretskii <eliz <at> gnu.org>, 57150 <at> debbugs.gnu.org
Subject: Re: bug#57150: 29.0.50; [PATCH] Add test coverage for overlay
 modification hooks
Date: Wed, 17 Aug 2022 18:05:00 +0800

Richard Stallman <rms <at> gnu.org> writes:

> The first question is, should these use text properties instead of
> overlays?

It may be possible, but there are non-trivial implications when one
attempts to replace overlays with text properties. Some
implications I can remember quickly:

(1) Overlays do not overwrite original properties and thus removing
    overlays automatically recovers the original properties. Not so
    easily when using text properties.
(2) Overlays are not copied when the text is killed. Text properties
    are. Overlays are also not carried between indirect buffers.
(3) Inserting text inside overlays automatically inherits their
    properties. To do the same with text properties, one must use
    special functions like `insert-and-inherit', which is not common in
    packages.

To conclude, overlays and text-properties are similar in many aspects,
but also handled differently in other aspects. These differences make it
very annoying to change from overlays to text property approaches.

> Are they using many overlays to highlight many potential matches in a
> long buffer?  If so, would it make sense to show, at any time, only those
> that are near point?  When you move to a far-away part of the buffer,
> it could add highlighting to the matches near there.  But maybe it
> can avoid ever trying to highlight more than a few parts of the buffer.

I guess it can be a valid optimization in some cases. But it is not
always possible. For example, consider folded headlines in Org mode.
They have some initial folding state set when opening the file, but the
state can be changed any time by the user. It is technically challenging
to put overlays on the fly in such scenario.

-- 
Ihor Radchenko,
Org mode contributor,
Learn more about Org mode at https://orgmode.org/.
Support Org development at https://liberapay.com/org-mode,
or support my work at https://liberapay.com/yantar92

Message #44 received at 57150 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: rms <at> gnu.org
Cc: matt <at> rfc20.org, yantar92 <at> gmail.com, 57150 <at> debbugs.gnu.org
Subject: Re: bug#57150: 29.0.50;
 [PATCH] Add test coverage for overlay modification hooks
Date: Wed, 17 Aug 2022 14:51:26 +0300

> From: Richard Stallman <rms <at> gnu.org>
> Cc: yantar92 <at> gmail.com, matt <at> rfc20.org, 57150 <at> debbugs.gnu.org
> Date: Tue, 16 Aug 2022 22:50:38 -0400
> 
>   > The problems with overlays in Emacs are much wider than just Org.  Our
>   > performance sucks in many cases because of that.  Just two examples
>   > that were mentioned lately in the "long-line optimizations"
>   > discussion: show-paren-mode and isearch-lazy-highlight -- each one of
>   > these two is capable of literally bringing Emacs to its knees,
>   > response-time wise.
> 
> The first question is, should these use text properties instead of
> overlays?

Text properties are less convenient when used for ephemeral
highlighting.

> Are they using many overlays to highlight many potential matches in a
> long buffer?

isearch-lazy-highlight does.

> If so, would it make sense to show, at any time, only those
> that are near point?  When you move to a far-away part of the buffer,
> it could add highlighting to the matches near there.  But maybe it
> can avoid ever trying to highlight more than a few parts of the buffer.

isearch.el already attempts to do that, but its method of detecting
what's out of the window doesn't work under truncate-lines.

Message #47 received at 57150 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Matt Armstrong <matt <at> rfc20.org>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 57150 <at> debbugs.gnu.org
Subject: Re: bug#57150: 29.0.50; [PATCH] Add test coverage for overlay
 modification hooks
Date: Sun, 04 Sep 2022 23:59:18 +0200

Matt Armstrong <matt <at> rfc20.org> writes:

> From: Matt Armstrong <matt <at> rfc20.org>
> Date: Thu, 11 Aug 2022 21:11:36 -0700
> Subject: [PATCH] Add basic test coverage for overlay modification hooks

Thanks; pushed to Emacs 29.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.