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#24940: [PATCH] Add should-call, should-not-call, and their tests Date: Mon, 14 Nov 2016 17:50:41 +0200 Message-ID: <8337iuhxgu.fsf@gnu.org> References: Reply-To: Eli Zaretskii NNTP-Posting-Host: blaine.gmane.org X-Trace: blaine.gmane.org 1479139349 25145 195.159.176.226 (14 Nov 2016 16:02:29 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Mon, 14 Nov 2016 16:02:29 +0000 (UTC) Cc: 24940@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:02:25 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 1c6Ji9-0005fS-8Y for geb-bug-gnu-emacs@m.gmane.org; Mon, 14 Nov 2016 17:02:21 +0100 Original-Received: from localhost ([::1]:41036 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c6JiC-0003VK-D2 for geb-bug-gnu-emacs@m.gmane.org; Mon, 14 Nov 2016 11:02:24 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:37265) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c6JXE-0002OK-Uw for bug-gnu-emacs@gnu.org; Mon, 14 Nov 2016 10:51:06 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c6JXB-0001Nb-Rk for bug-gnu-emacs@gnu.org; Mon, 14 Nov 2016 10:51:05 -0500 Original-Received: from debbugs.gnu.org ([208.118.235.43]:41876) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1c6JXB-0001NX-OW for bug-gnu-emacs@gnu.org; Mon, 14 Nov 2016 10:51:01 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1c6JXB-0004xN-IZ for bug-gnu-emacs@gnu.org; Mon, 14 Nov 2016 10:51:01 -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:51:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 24940 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 24940-submit@debbugs.gnu.org id=B24940.147913863919013 (code B ref 24940); Mon, 14 Nov 2016 15:51:01 +0000 Original-Received: (at 24940) by debbugs.gnu.org; 14 Nov 2016 15:50:39 +0000 Original-Received: from localhost ([127.0.0.1]:57275 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1c6JWp-0004wb-28 for submit@debbugs.gnu.org; Mon, 14 Nov 2016 10:50:39 -0500 Original-Received: from eggs.gnu.org ([208.118.235.92]:46127) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1c6JWo-0004wP-FL for 24940@debbugs.gnu.org; Mon, 14 Nov 2016 10:50:38 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c6JWg-0001IC-9h for 24940@debbugs.gnu.org; Mon, 14 Nov 2016 10:50:33 -0500 Original-Received: from fencepost.gnu.org ([2001:4830:134:3::e]:38604) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c6JWg-0001I8-5s; Mon, 14 Nov 2016 10:50:30 -0500 Original-Received: from 84.94.185.246.cable.012.net.il ([84.94.185.246]:1900 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_128_CBC_SHA1:128) (Exim 4.82) (envelope-from ) id 1c6JWf-0005Op-J7; Mon, 14 Nov 2016 10:50:29 -0500 In-reply-to: (message from Gemini Lasswell on Sun, 13 Nov 2016 14:22:08 -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:125687 Archived-At: > From: Gemini Lasswell > Date: Sun, 13 Nov 2016 14:22:08 -0800 > > In the process of writing tests for kmacro.el, I wrote two macros, > should-call and should-not-call, that create context by temporarily > adding advice to named functions. They allow a test writer to express > expectations of how functions are used, to mock up responses of those > functions to the code under test, and to prevent functions from running > which might modify the global state of Emacs in an undesirable way > during a test. I don't think our test suite should in general test whether some functions are or aren't called during execution of the command/function under test. That's because we are not interested in the fine details of the implementation of the features being tested, we are interested in whether it does what it's supposed to do. "What it's supposed to do" means we should test the return values and the side effects, a.k.a. "the behavior", and not how the code works internally. Using your proposed approach (amply represented in the tests you submitted for kmacro.el) has several disadvantages, beyond the above fundamental issue: . it makes it impossible to write tests before the features are implemented (TDD), since the details of the implementation, i.e. who calls what and when, are not yet known . it tends to produce tests that are much more complex than they have to be, due to the need to disable or override internal code workings, so the tested code does what you want, without getting in your way . by messing with so much of the internal workings, there's a high risk that the tested code is significantly different from what runs in a real Emacs session, which invalidates the test results to some degree -- you are no longer testing the original code . the produced tests are extremely fragile, and will need a lot of maintenance whenever something changes in the tested features' implementation, or in the infrastructure they use, or in how certain exceptional situations are handled by that infrastructure I consider the should-call and should-not-call tests appropriate only when the behavior of the function is explicitly specified to make these calls. About the only applicable use case is when a function is specified to prompt the user under some circumstances and not the others. And even then it's fragile, because "prompt the user" is not equal to "call yes-or-no-p", even though the current implementation might do the latter. A better way, though perhaps much harder, would be to test something much more basic that is common to _any_ prompt, like look in the echo-area or the minibuffer buffers for suitable text, or maybe even C-level infrastructure that catches the situation where Emacs waits for user input. If we can agree that the above use cases are the only ones where such macros should be used, then these macros should be much leaner than what you propose, because there should be no need to provide many of the features in your proposal, like testing the arguments and the return value, or the number of times the subroutines are called. > I also rewrote one test from files-tests.el as an example of usage and > included it with the patch. It shows how the macros can help make the > logic of a test clearer by removing the clutter of extra variables used > to keep track of the arguments passed in function calls made by the code > under test. For more examples, see the kmacro-tests.el patch in > bug#24939. I think it's no coincidence you did that with a test whose purpose is to make sure the user is not prompted under certain conditions. I think these are about the only valid use cases for such functionality. However, note that your kmacro.el tests employ these macros much more extensively, mostly where we don't care about which functions are called, but about the produced effect. When there's an expected effect, we should test that effect directly, instead of relying on certain functions being called, and assuming they will produce the desired effect. Thanks.