* bug#57150: 29.0.50; [PATCH] Add test coverage for overlay modification hooks @ 2022-08-12 4:32 Matt Armstrong 2022-08-12 4:41 ` Matt Armstrong 2022-08-13 4:07 ` Ihor Radchenko 0 siblings, 2 replies; 15+ messages in thread From: Matt Armstrong @ 2022-08-12 4:32 UTC (permalink / raw) To: 57150 [-- Attachment #1: Type: text/plain, Size: 419 bytes --] 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. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-Add-test-coverage-for-overlay-modification-hooks.patch --] [-- Type: text/x-diff, Size: 4193 bytes --] From 9083709cbe12ab97b795bcac8343f9d11f2929b7 Mon Sep 17 00:00:00 2001 From: Matt Armstrong <matt@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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* bug#57150: 29.0.50; [PATCH] Add test coverage for overlay modification hooks 2022-08-12 4:32 bug#57150: 29.0.50; [PATCH] Add test coverage for overlay modification hooks Matt Armstrong @ 2022-08-12 4:41 ` Matt Armstrong 2022-08-12 6:19 ` Eli Zaretskii 2022-08-13 4:07 ` Ihor Radchenko 1 sibling, 1 reply; 15+ messages in thread From: Matt Armstrong @ 2022-08-12 4:41 UTC (permalink / raw) To: 57150 [-- Attachment #1: Type: text/plain, Size: 514 bytes --] Matt Armstrong <matt@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. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-Add-test-coverage-for-overlay-modification-hooks.patch --] [-- Type: text/x-diff, Size: 4206 bytes --] From 372c894e8d33eb5271b6c19b2e2e2843197a7353 Mon Sep 17 00:00:00 2001 From: Matt Armstrong <matt@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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* bug#57150: 29.0.50; [PATCH] Add test coverage for overlay modification hooks 2022-08-12 4:41 ` Matt Armstrong @ 2022-08-12 6:19 ` Eli Zaretskii 2022-08-12 17:57 ` Matt Armstrong 2022-08-14 4:52 ` Matt Armstrong 0 siblings, 2 replies; 15+ messages in thread From: Eli Zaretskii @ 2022-08-12 6:19 UTC (permalink / raw) To: Matt Armstrong; +Cc: 57150 > From: Matt Armstrong <matt@rfc20.org> > Date: Thu, 11 Aug 2022 21:41:21 -0700 > > Matt Armstrong <matt@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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* bug#57150: 29.0.50; [PATCH] Add test coverage for overlay modification hooks 2022-08-12 6:19 ` Eli Zaretskii @ 2022-08-12 17:57 ` Matt Armstrong 2022-08-12 18:53 ` Eli Zaretskii 2022-08-14 4:52 ` Matt Armstrong 1 sibling, 1 reply; 15+ messages in thread From: Matt Armstrong @ 2022-08-12 17:57 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 57150 Eli Zaretskii <eliz@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! ^ permalink raw reply [flat|nested] 15+ messages in thread
* bug#57150: 29.0.50; [PATCH] Add test coverage for overlay modification hooks 2022-08-12 17:57 ` Matt Armstrong @ 2022-08-12 18:53 ` Eli Zaretskii 0 siblings, 0 replies; 15+ messages in thread From: Eli Zaretskii @ 2022-08-12 18:53 UTC (permalink / raw) To: Matt Armstrong; +Cc: 57150 > From: Matt Armstrong <matt@rfc20.org> > Cc: 57150@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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* bug#57150: 29.0.50; [PATCH] Add test coverage for overlay modification hooks 2022-08-12 6:19 ` Eli Zaretskii 2022-08-12 17:57 ` Matt Armstrong @ 2022-08-14 4:52 ` Matt Armstrong 2022-08-14 7:15 ` Eli Zaretskii 2022-09-04 21:59 ` Lars Ingebrigtsen 1 sibling, 2 replies; 15+ messages in thread From: Matt Armstrong @ 2022-08-14 4:52 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 57150 [-- Attachment #1: Type: text/plain, Size: 3074 bytes --] Thanks Eli, another patch attached. More comments below. Eli Zaretskii <eliz@gnu.org> writes: >> From: Matt Armstrong <matt@rfc20.org> >> Date: Thu, 11 Aug 2022 21:41:21 -0700 >> >> Matt Armstrong <matt@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. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-Add-basic-test-coverage-for-overlay-modification-hoo.patch --] [-- Type: text/x-diff, Size: 10010 bytes --] From 3a3fbf96ce64e8a40a849c3b3f8453614a92b728 Mon Sep 17 00:00:00 2001 From: Matt Armstrong <matt@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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* bug#57150: 29.0.50; [PATCH] Add test coverage for overlay modification hooks 2022-08-14 4:52 ` Matt Armstrong @ 2022-08-14 7:15 ` Eli Zaretskii 2022-09-04 21:59 ` Lars Ingebrigtsen 1 sibling, 0 replies; 15+ messages in thread From: Eli Zaretskii @ 2022-08-14 7:15 UTC (permalink / raw) To: Matt Armstrong; +Cc: 57150 > From: Matt Armstrong <matt@rfc20.org> > Cc: 57150@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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* bug#57150: 29.0.50; [PATCH] Add test coverage for overlay modification hooks 2022-08-14 4:52 ` Matt Armstrong 2022-08-14 7:15 ` Eli Zaretskii @ 2022-09-04 21:59 ` Lars Ingebrigtsen 1 sibling, 0 replies; 15+ messages in thread From: Lars Ingebrigtsen @ 2022-09-04 21:59 UTC (permalink / raw) To: Matt Armstrong; +Cc: Eli Zaretskii, 57150 Matt Armstrong <matt@rfc20.org> writes: > From: Matt Armstrong <matt@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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* bug#57150: 29.0.50; [PATCH] Add test coverage for overlay modification hooks 2022-08-12 4:32 bug#57150: 29.0.50; [PATCH] Add test coverage for overlay modification hooks Matt Armstrong 2022-08-12 4:41 ` Matt Armstrong @ 2022-08-13 4:07 ` Ihor Radchenko 2022-08-14 0:44 ` Matt Armstrong 1 sibling, 1 reply; 15+ messages in thread From: Ihor Radchenko @ 2022-08-13 4:07 UTC (permalink / raw) To: Matt Armstrong; +Cc: 57150 Matt Armstrong <matt@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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* bug#57150: 29.0.50; [PATCH] Add test coverage for overlay modification hooks 2022-08-13 4:07 ` Ihor Radchenko @ 2022-08-14 0:44 ` Matt Armstrong 2022-08-15 4:14 ` Ihor Radchenko 0 siblings, 1 reply; 15+ messages in thread From: Matt Armstrong @ 2022-08-14 0:44 UTC (permalink / raw) To: Ihor Radchenko; +Cc: 57150 Ihor Radchenko <yantar92@gmail.com> writes: > Matt Armstrong <matt@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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* bug#57150: 29.0.50; [PATCH] Add test coverage for overlay modification hooks 2022-08-14 0:44 ` Matt Armstrong @ 2022-08-15 4:14 ` Ihor Radchenko 2022-08-15 11:26 ` Eli Zaretskii 0 siblings, 1 reply; 15+ messages in thread From: Ihor Radchenko @ 2022-08-15 4:14 UTC (permalink / raw) To: Matt Armstrong; +Cc: 57150 Matt Armstrong <matt@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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* bug#57150: 29.0.50; [PATCH] Add test coverage for overlay modification hooks 2022-08-15 4:14 ` Ihor Radchenko @ 2022-08-15 11:26 ` Eli Zaretskii 2022-08-17 2:50 ` Richard Stallman 0 siblings, 1 reply; 15+ messages in thread From: Eli Zaretskii @ 2022-08-15 11:26 UTC (permalink / raw) To: Ihor Radchenko; +Cc: matt, 57150 > Cc: 57150@debbugs.gnu.org > From: Ihor Radchenko <yantar92@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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* bug#57150: 29.0.50; [PATCH] Add test coverage for overlay modification hooks 2022-08-15 11:26 ` Eli Zaretskii @ 2022-08-17 2:50 ` Richard Stallman 2022-08-17 10:05 ` Ihor Radchenko 2022-08-17 11:51 ` Eli Zaretskii 0 siblings, 2 replies; 15+ messages in thread From: Richard Stallman @ 2022-08-17 2:50 UTC (permalink / raw) To: Eli Zaretskii; +Cc: matt, yantar92, 57150 [[[ 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) ^ permalink raw reply [flat|nested] 15+ messages in thread
* bug#57150: 29.0.50; [PATCH] Add test coverage for overlay modification hooks 2022-08-17 2:50 ` Richard Stallman @ 2022-08-17 10:05 ` Ihor Radchenko 2022-08-17 11:51 ` Eli Zaretskii 1 sibling, 0 replies; 15+ messages in thread From: Ihor Radchenko @ 2022-08-17 10:05 UTC (permalink / raw) To: rms; +Cc: matt, Eli Zaretskii, 57150 Richard Stallman <rms@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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* bug#57150: 29.0.50; [PATCH] Add test coverage for overlay modification hooks 2022-08-17 2:50 ` Richard Stallman 2022-08-17 10:05 ` Ihor Radchenko @ 2022-08-17 11:51 ` Eli Zaretskii 1 sibling, 0 replies; 15+ messages in thread From: Eli Zaretskii @ 2022-08-17 11:51 UTC (permalink / raw) To: rms; +Cc: matt, yantar92, 57150 > From: Richard Stallman <rms@gnu.org> > Cc: yantar92@gmail.com, matt@rfc20.org, 57150@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. ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2022-09-04 21:59 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-08-12 4:32 bug#57150: 29.0.50; [PATCH] Add test coverage for overlay modification hooks Matt Armstrong 2022-08-12 4:41 ` Matt Armstrong 2022-08-12 6:19 ` Eli Zaretskii 2022-08-12 17:57 ` Matt Armstrong 2022-08-12 18:53 ` Eli Zaretskii 2022-08-14 4:52 ` Matt Armstrong 2022-08-14 7:15 ` Eli Zaretskii 2022-09-04 21:59 ` Lars Ingebrigtsen 2022-08-13 4:07 ` Ihor Radchenko 2022-08-14 0:44 ` Matt Armstrong 2022-08-15 4:14 ` Ihor Radchenko 2022-08-15 11:26 ` Eli Zaretskii 2022-08-17 2:50 ` Richard Stallman 2022-08-17 10:05 ` Ihor Radchenko 2022-08-17 11:51 ` Eli Zaretskii
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).