From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#24939: [PATCH] Add tests for lisp/kmacro.el Date: Mon, 14 Nov 2016 17:49:47 +0200 Message-ID: <834m3ahxic.fsf@gnu.org> References: Reply-To: Eli Zaretskii NNTP-Posting-Host: blaine.gmane.org X-Trace: blaine.gmane.org 1479139752 12107 195.159.176.226 (14 Nov 2016 16:09:12 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Mon, 14 Nov 2016 16:09:12 +0000 (UTC) Cc: 24939@debbugs.gnu.org To: Gemini Lasswell Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Mon Nov 14 17:09:05 2016 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1c6JoE-0007Rz-AU for geb-bug-gnu-emacs@m.gmane.org; Mon, 14 Nov 2016 17:08:38 +0100 Original-Received: from localhost ([::1]:41066 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c6JoF-0000YT-UG for geb-bug-gnu-emacs@m.gmane.org; Mon, 14 Nov 2016 11:08:39 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:37065) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c6JWK-0001ai-95 for bug-gnu-emacs@gnu.org; Mon, 14 Nov 2016 10:50:09 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c6JWE-0001Cj-N9 for bug-gnu-emacs@gnu.org; Mon, 14 Nov 2016 10:50:08 -0500 Original-Received: from debbugs.gnu.org ([208.118.235.43]:41872) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1c6JWE-0001Cf-Ig for bug-gnu-emacs@gnu.org; Mon, 14 Nov 2016 10:50:02 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1c6JWE-0004v8-CY for bug-gnu-emacs@gnu.org; Mon, 14 Nov 2016 10:50:02 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Mon, 14 Nov 2016 15:50:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 24939 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 24939-submit@debbugs.gnu.org id=B24939.147913858718879 (code B ref 24939); Mon, 14 Nov 2016 15:50:02 +0000 Original-Received: (at 24939) by debbugs.gnu.org; 14 Nov 2016 15:49:47 +0000 Original-Received: from localhost ([127.0.0.1]:57271 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1c6JVy-0004uR-Go for submit@debbugs.gnu.org; Mon, 14 Nov 2016 10:49:46 -0500 Original-Received: from eggs.gnu.org ([208.118.235.92]:45995) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1c6JVw-0004uC-PV for 24939@debbugs.gnu.org; Mon, 14 Nov 2016 10:49:45 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c6JVo-00018y-9j for 24939@debbugs.gnu.org; Mon, 14 Nov 2016 10:49:39 -0500 Original-Received: from fencepost.gnu.org ([2001:4830:134:3::e]:38598) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c6JVo-00018u-62; Mon, 14 Nov 2016 10:49:36 -0500 Original-Received: from 84.94.185.246.cable.012.net.il ([84.94.185.246]:1893 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_128_CBC_SHA1:128) (Exim 4.82) (envelope-from ) id 1c6JVn-0005Hn-Hb; Mon, 14 Nov 2016 10:49:35 -0500 In-reply-to: (message from Gemini Lasswell on Sun, 13 Nov 2016 13:23:51 -0800) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 208.118.235.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.org gmane.emacs.bugs:125689 Archived-At: > From: Gemini Lasswell > 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.