Thanks Eli, another patch attached. More comments below. Eli Zaretskii writes: >> From: Matt Armstrong >> Date: Thu, 11 Aug 2022 21:41:21 -0700 >> >> Matt Armstrong writes: >> >> > I noticed there was no explicit test against the overlay modification >> > hook boundary conditions. The attached patch adds some, which might >> > be nice to have if a person (possibly me) someday optimizes overlay >> > performance with a change in data structure. >> > >> > In writing this I was surprised to learn that the hooks do not behave >> > differently if the overlay is `front-advance' or `rear-advance', so >> > this is explicitly tested. >> >> ...follow up with a comment typo fix. > > Thanks! Adding tests is always welcome, so this is fine, of course. > Please allow me a few minor comments, though. > > A frequent gripe of mine about our test-suite tests is that the ideas > of the tests are not sufficiently explained, where they aren't 110% > obvious. The ERT framework and the tests themselves being highly > macro-ized don't help in that matter, either. So please try to > explain the idea of the tests and the details of the data structures > as much as is reasonably possible. In this case: I switched to an approach where each test case is specified by an alist with keys bound to variables with `let-alist'. This lets me log the test case "spec" with `message' while still conveniently unpacking the alist. I also switched to an approach where the "recorded" modification hook calls are stored on a property on the overlay itself, which then made it easy to extract this somewhat tricky code into helper functions. I added some message output, including a message printing the "test spec" alist before it executes. I verified that this new message output is part of the test log (as captured by "make check") and appears on the console for failed tests. I also left a few other "debug log" style messages in the test case that I found helpful during debugging. I don't feel strongly about leaving them in or not. I left a TODO, which is more of a question, about one test case that reveals a call to the modification hooks for a simple insert, which the elisp manual seems to suggest will not happen (because inserts do not modify characters)...perhaps the manual is worded too strongly? Am I misunderstanding something? Ironically, after all this was done, I am left wondering if a well designed macro fixture wouldn't be more clear, simply because it does produce a separate ERT test for each test case. Perhaps there are ways to design macro test fixtures in clear ways to avoid the downsides of the way they appear in the other buffer-tests.el tests today.... > This means you don't test so-called "empty" overlays, whose START and > END are identical. They are handled specially by the low-level > support code. Maybe add more tests for that? I added one test case for an empty overlay. I'm not confident I've covered all of the interesting cases, but nothing obvious occurred to me.