Subject: bug#24940: [PATCH] Add should-call, should-not-call, and their tests

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

> From: Gemini Lasswell <[email-masked]>
> 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.