bug#57150: 29.0.50; [PATCH] Add test coverage for overlay modification hooks

[Eli Zaretskii <eliz@gnu.org>]

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