GNU bug report logs - #24940
[PATCH] Add should-call, should-not-call, and their tests

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 24940 in the body.
You can then email your comments to 24940 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Gemini Lasswell <gazally <at> runbox.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Add should-call, should-not-call, and their tests
Date: Sun, 13 Nov 2016 14:22:08 -0800

[Message part 1 (text/plain, inline)]

Hello,

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 think that these macros would be useful additions to ERT. Here is a
patch containing versions of the macros which are integrated into ERT
and which provide better failure reporting than the ones that I included
with the kmacro-tests.el patch in bug#24939.

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.

Let me know if you see ways to make this code better, or if there's any
part of adding functionality to Emacs that I've missed here.

[0001-Add-should-call-should-not-call-and-their-tests.patch (text/plain, attachment)]

Message #8 received at 24940 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Gemini Lasswell <gazally <at> runbox.com>
Cc: 24940 <at> debbugs.gnu.org
Subject: Re: bug#24940: [PATCH] Add should-call, should-not-call,
 and their tests
Date: Mon, 14 Nov 2016 17:50:41 +0200

> From: Gemini Lasswell <gazally <at> runbox.com>
> 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.

Message #11 received at 24940 <at> debbugs.gnu.org (full text, mbox):

From: phillip.lord <at> russet.org.uk (Phillip Lord)
To: Gemini Lasswell <gazally <at> runbox.com>
Cc: 24940 <at> debbugs.gnu.org
Subject: Re: bug#24940: [PATCH] Add should-call, should-not-call,
 and their tests
Date: Mon, 14 Nov 2016 16:18:22 +0000

That's interesting. I'm probably going to add my own package, assess, to
ELPA soon, and then to core, which has some similar functionality.

https://github.com/phillord/assess

Out of curiosity, would assess have fulfilled your use case also?

Gemini Lasswell <gazally <at> runbox.com> writes:
> 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 think that these macros would be useful additions to ERT. Here is a
> patch containing versions of the macros which are integrated into ERT
> and which provide better failure reporting than the ones that I included
> with the kmacro-tests.el patch in bug#24939.
>
> 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.
>
> Let me know if you see ways to make this code better, or if there's any
> part of adding functionality to Emacs that I've missed here.

Message #14 received at 24940 <at> debbugs.gnu.org (full text, mbox):

From: Gemini Lasswell <gazally <at> runbox.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 24940 <at> debbugs.gnu.org
Subject: Re: bug#24940: [PATCH] Add should-call, should-not-call,
 and their tests
Date: Mon, 14 Nov 2016 21:52:23 -0800

Eli Zaretskii <eliz <at> gnu.org> writes:

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

I'll modify the tests I've written to focus on "the behavior".

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

I agree that infrastructure to allow a test to set up input to be
supplied to the next prompt and to read the prompt message would be a
better solution.

Keyboard macros work to simulate user input for any test code that isn't
trying to test kmacro-step-edit-macro. If there was a hook that the C
code could run just prior to taking input from a keyboard macro, tests
could use it to check that the correct prompt appears at the correct
point in the macro.

Such a hook would also be helpful in the attempt to fix
kmacro-step-edit-macro's bugs. Mostly where it messes up now is when
several input events happen during one command, and a pre-input hook
would allow it to inspect keyboard macro execution event by event rather
than command by command.

The remaining problem that I see is that lots of code in lots of places
suppresses messages depending on called-interactively-p,
executing-kbd-macro and/or noninteractive, and although that doesn't
just affect tests that use keyboard macros, it does make it hard to test
how Emacs behaves interactively.

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

The leanest version I can think of would be:

(with-added-advice BINDINGS
   BODY)

where BINDINGS is a list of argument lists for advice-add. It would call
advice-add once for each element of BINDINGS, execute BODY, and then
remove all the added advice. Then it looks like something that belongs
in nadvice.el instead of ert.el.

Message #17 received at 24940 <at> debbugs.gnu.org (full text, mbox):

From: Gemini Lasswell <gazally <at> runbox.com>
To: phillip.lord <at> russet.org.uk (Phillip Lord)
Cc: 24940 <at> debbugs.gnu.org
Subject: Re: bug#24940: [PATCH] Add should-call, should-not-call,
 and their tests
Date: Mon, 14 Nov 2016 22:12:52 -0800

phillip.lord <at> russet.org.uk (Phillip Lord) writes:

> That's interesting. I'm probably going to add my own package, assess, to
> ELPA soon, and then to core, which has some similar functionality.
>
> https://github.com/phillord/assess
>
> Out of curiosity, would assess have fulfilled your use case also?

Yes it would have.

Message #20 received at 24940 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Gemini Lasswell <gazally <at> runbox.com>
Cc: 24940 <at> debbugs.gnu.org
Subject: Re: bug#24940: [PATCH] Add should-call, should-not-call,
 and their tests
Date: Tue, 15 Nov 2016 17:24:21 +0200

> From: Gemini Lasswell <gazally <at> runbox.com>
> Cc: 24940 <at> debbugs.gnu.org
> Date: Mon, 14 Nov 2016 21:52:23 -0800
> 
> I'll modify the tests I've written to focus on "the behavior".

Thank you.

> > 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.
> 
> I agree that infrastructure to allow a test to set up input to be
> supplied to the next prompt and to read the prompt message would be a
> better solution.

You are talking about mocking user input, whereas I was talking about
a feature that could tell whether Emacs asked for user input in any
way, even if the user herself provided that input.

Of course, mocking input is also an important part of a test suite.

> The remaining problem that I see is that lots of code in lots of places
> suppresses messages depending on called-interactively-p,
> executing-kbd-macro and/or noninteractive, and although that doesn't
> just affect tests that use keyboard macros, it does make it hard to test
> how Emacs behaves interactively.

We could have a separate knob for that, to be used by the test suite.

> > 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.
> 
> The leanest version I can think of would be:
> 
> (with-added-advice BINDINGS
>    BODY)
> 
> where BINDINGS is a list of argument lists for advice-add. It would call
> advice-add once for each element of BINDINGS, execute BODY, and then
> remove all the added advice.

No, I meant a macro that would return non-nil if some FUNCTION was
called during execution of that macro's body.

Message #23 received at 24940 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: phillip.lord <at> russet.org.uk (Phillip Lord)
Cc: Gemini Lasswell <gazally <at> runbox.com>, 24940 <at> debbugs.gnu.org
Subject: Re: bug#24940: [PATCH] Add should-call, should-not-call,
 and their tests
Date: Tue, 25 Jun 2019 01:43:04 +0200

phillip.lord <at> russet.org.uk (Phillip Lord) writes:

> That's interesting. I'm probably going to add my own package, assess, to
> ELPA soon, and then to core, which has some similar functionality.
>
> https://github.com/phillord/assess

It looks like it wasn't added to ELPA or core?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #26 received at 24940 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Gemini Lasswell <gazally <at> runbox.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 24940 <at> debbugs.gnu.org
Subject: Re: bug#24940: [PATCH] Add should-call, should-not-call, and their
 tests
Date: Tue, 11 Aug 2020 15:23:18 +0200

Gemini Lasswell <gazally <at> runbox.com> writes:

>> 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.
>
> I'll modify the tests I've written to focus on "the behavior".

Gemini, this was apparently never done?  Did you abandon this patch in
favour of Phillip's assess package?  (Which I see is in Melpa now.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.