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

[Gemini Lasswell <gazally@runbox.com>]

Thanks for the thorough feedback! I'll work on a new & improved version
incorporating your comments.
Some responses/questions below.

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

This strategy is used in autorevert-tests.el and filenotify-tests.el,
which is where I copied it from. So those should be changed too.

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

That would definitely be helpful.

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

Actually not. I was attempting to write a regression for bug#11817,
which was about the "Type e to repeat macro" not showing up in the echo
area. But kmacro-call-macro doesn't put up the message if there's
already a message. I just looked into the history of that bit of code,
and kmacro-call-macro's check of (current-message) is actually another
bug fix, for bug#3412.

>> +(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'll make that change. What do you think about changing
ert-simulate-command to set current-prefix-arg? That would be very
helpful.

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

That comment is out of date, since kmacro-tests-keymap should fix the
problem. I'll remove it.