Subject: bug#24939: [PATCH] Add tests for lisp/kmacro.el

From: Eli Zaretskii <[email-masked]>
Date: Mon, 14 Nov 2016 17:49:47 +0200

> From: Gemini Lasswell <[email-masked]>
> Date: Sun, 13 Nov 2016 13:23:51 -0800
> 
> There weren't any tests for kmacro.el, so I have written some.

Thanks!

> 1. These tests make extensive use of two macros, called
> kmacro-tests-should-call and kmacro-tests-should-not-call. They are
> context-creating macros which add advice to named functions for the
> duration of a test. I think that these two macros would be a useful
> addition to ERT, and I'll submit that idea as a separate patch.

Thanks, I respond to this point there.

> 2. I found several minor bugs in the process of writing these, leading
> to tests marked as :expected-result :failed. One is a way to create an
> empty keyboard macro using the mouse and the rest are ways to get
> kmacro-step-edit-macro to behave oddly. I haven't sent them to
> bug-gnu-emacs yet. When I do so would it be better to send them
> individually or put all the step-edit ones in one report?

It's a judgment call.  If the offending function is small enough (it
is in this case), and the required changes aren't expected to be too
complex, then one report is better.

Allow me a few comments to your proposed patch.

> +(defmacro kmacro-tests-should-not-call (func-or-funcs &rest body)
> +  "Verify that a function or functions are not called during execution.
> +FUNC-OR-FUNCS can either be a single function or a list of them.
> +Temporarily override them them during the execution of BODY with advice
                        ^^^^^^^^^
One "them" is redundant.

> +containing `should-not' and a non-nil value.  Use the function symbol
> +as the non-nil value to make tracking down what went wrong a bit
> +faster."

A general comment: I think your doc strings describe the
implementation too intimately.  I appreciate the effort that went into
doing that, but the result has 2 disadvantages: (1) the doc string
needs much more maintenance than it should, because any change to the
implementation will need the doc string to be updated (which runs a
high risk of the doc becoming out of sync), and (2) the user of the
function needs to wade through details she doesn't really need to
know.

A doc string should describe the arguments, the return value, and the
side effects, if any, of the function, without going into the
implementation.

> +(defmacro kmacro-tests-should-insert (value &rest body)
> +  "Verify that VALUE is inserted by the execution of BODY.
> +Execute BODY, then check that the string VALUE was inserted
> +into the current buffer at point.  As a side effect captures the
> +symbols p_ and bsize_."

Without explaining what p_ and bsize_ are, the last sentence of the
doc string are not really useful, IMO.

> +  (declare (debug (stringp body))
> +           (indent 1))
> +  `(let ((p_ (point))
> +         (bsize_ (buffer-size)))
> +     ,@body
> +     (should (equal (buffer-substring p_ (point)) ,value))
> +     (should (equal (- (buffer-size) bsize_) (string-width ,value)))))
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This doesn't look right to me: string-width takes the char-width of
each character into account, so it cannot be reliably compared to
buffer size.  I think you want to call length instead.

> +(defmacro kmacro-tests-should-match-message (value &rest body)
> +  "Verify that a message matching VALUE is issued while executing BODY.
> +Execute BODY, then check for a regexp match between
> +VALUE and any text written to *Messages* during the execution."
> +  (declare (debug (stringp body))
> +           (indent 1))
> +  `(with-current-buffer (get-buffer-create "*Messages*")
> +     (save-restriction
> +       (narrow-to-region (point-max) (point-max))
> +       ,@body
> +       (should (string-match-p ,value (buffer-string))))))

I don't like this implementation.  First, playing restriction games
with *Messages* is inherently unsafe, because that buffer is treated
specially by the code which puts messages there.  Second, this assumes
*Messages* will have each message verbatim, which is false, because
repeated messages aren't inserted.  And finally, some code can disable
message logging or use some mechanism for displaying echo-area
messages that bypasses *Messages*, in which case this macro will not
catch the message.

So I'd suggest instead to override or advice 'message', so you could
get your hands on the messages more reliably.  It is possible we
should have a more thorough infrastructure for collecting echo-area
messages, which probably means parts of it should be implemented in C,
but that's a separate project.

> +(kmacro-tests-deftest kmacro-tests-test-insert-counter-01-nil ()
> +  "Insert counter adds one with nil arg."
      ^^^^^^^^^^^^^^
I think you meant "Inserted counter", here and elsewhere.

> +(kmacro-tests-deftest kmacro-tests-test-start-macro-when-defining-macro ()
> +  "Starting a macro while defining a macro does not start a second macro."
> +  ;; Verify kmacro-start-macro calls start-kbd-macro
> +  (:stub start-kbd-macro)
> +  (kmacro-tests-should-call
> +      ((start-kbd-macro :once :before (lambda (append noexec)
> +                                        (should-not append)
> +                                        (should-not noexec))))
> +    (kmacro-tests-with-current-prefix-arg (kmacro-start-macro nil)))
> +  ;; And leaves macro in being-recorded state
> +  (should defining-kbd-macro)
> +  ;; And that it doesn't call it again
> +  (kmacro-tests-should-not-call start-kbd-macro
> +    (kmacro-tests-with-current-prefix-arg (kmacro-start-macro nil))))

I have a grave issue with this kind of testing: it is too tied to the
implementation, instead of testing the behavior and the effects.
E.g., the fact that start-kbd-macro is only called once isn't a
reliable evidence that "starting a macro while defining a macro does
not start a second macro"; a reliable evidence would be to verify that
the second macro isn't defined after the form finishes.

I think there's a fundamental issue here, regarding the purpose of our
test suite and consequently the methodology we should use for writing
the tests.

More about this in my response to your proposed ert-should-call and
ert-should-not-call additions.  Here I just note that most of your
tests are based on this paradigm, which I think makes these tests much
more complex and fragile than they need to be.

> +(kmacro-tests-deftest kmacro-tests-set-macro-counter-while-defining ()
> +  "Use of the prefix arg with kmacro-start sets kmacro-counter."
> +  (:stub start-kbd-macro)
> +  ;; Give kmacro-start-macro an argument
> +  (kmacro-tests-should-call
> +      ((start-kbd-macro :once :before (lambda (append noexec)
> +                                        (should-not append)
> +                                        (should-not noexec))))
> +    (kmacro-tests-with-current-prefix-arg (kmacro-start-macro 5)))
> +  (should defining-kbd-macro)
> +  ;; And verify that the counter is set to that value
> +  (ert-with-test-buffer (:name "counter-while-defining")
> +    (kmacro-tests-should-insert "5"
> +      (kmacro-tests-with-current-prefix-arg (kmacro-insert-counter nil)))
> +    ;; change it while defining macro
> +    (kmacro-tests-with-current-prefix-arg (kmacro-set-counter 1))
> +    (kmacro-tests-should-insert "1"
> +      (kmacro-tests-with-current-prefix-arg (kmacro-insert-counter nil)))
> +    ;; using universal arg to to set counter should reset to starting value

Please use a consistent style in comments: begin with a capital letter
and end with a period.  These are our code conventions.

> +      (message "")  ; clear the echo area
> +      (kmacro-tests-should-match-message "Type e to repeat macro"

Why is that call to 'message' necessary here?  I suspect this is one
symptom of the fragility of kmacro-tests-should-match-message that I
mentioned above.

> +(kmacro-tests-deftest kmacro-tests-test-ring-commands-when-no-macros ()
> +  "Ring commands give appropriate message when no macros exist."
> +  (dolist (cmd '((kmacro-cycle-ring-next nil)
> +                 (kmacro-cycle-ring-previous nil)
> +                 (kmacro-swap-ring)
> +                 (kmacro-delete-ring-head)
> +                 (kmacro-view-ring-2nd)
> +                 (kmacro-call-ring-2nd nil)
> +                 (kmacro-view-macro)))
> +    (kmacro-tests-should-match-message "No keyboard macro defined"
> +      (apply #'funcall cmd))))
         ^^^^^^^^^^^^^^^^^^^^^
Why not ert-simulate-command?

> +  ;; I'm attempting to make the test work even if keys have been
> +  ;; rebound, but if this is failing try emacs -Q first.

If this comment is still valid, then many other parts of the test have
the same problem, because they clearly assume the default key
bindings.  If these tests are to be run as part of the test suite in
"emacs -batch" or "emacs -Q", then these problems don't exist; but if
not, your "clean slate" should somehow restore the default key
bindings, or else call the functions by name.

Thanks again for working on this.