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

[Matt Armstrong]

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!