GNU bug report logs - #25158
[PATCH] A better way for test code to access messages

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 25158 in the body.
You can then email your comments to 25158 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] A better way for test code to access messages
Date: Sat, 10 Dec 2016 09:29:05 -0800

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

Here is a quote from Bug#24939's discussion, regarding the technique
used by autorevert-tests.el, filenotify-tests.el and the proposed
kmacro-tests.el to collect messages issued during part of a test by
temporarily narrowing *Messages*:

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

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

The attached patch adds a new macro called ert-with-message-capture to
ert-x.el which temporarily adds advice to 'message' to collect messages.
I've also modified autorevert-tests.el and filenotify-tests.el to use
the new macro. If or when the more thorough infrastructure is
implemented, that could replace the use of advice in this macro but the
tests which use it should not have to change.

Michael, in modifying autorevert-tests.el, at the start of
auto-revert-test02-auto-revert-deleted-file, *Messages* was narrowed,
and then narrowed again before the call to auto-revert--wait-for-revert,
so it looked safe to delete the first narrowing instead of replacing it
with ert-with-message-capture. Let me know if I've missed something
there.

[0001-Add-ert-with-message-capture.patch (text/plain, attachment)]

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

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Gemini Lasswell <gazally <at> runbox.com>
Cc: 25158 <at> debbugs.gnu.org
Subject: Re: bug#25158: [PATCH] A better way for test code to access messages
Date: Sat, 10 Dec 2016 20:27:38 +0100

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

> Michael, in modifying autorevert-tests.el, at the start of
> auto-revert-test02-auto-revert-deleted-file, *Messages* was narrowed,
> and then narrowed again before the call to auto-revert--wait-for-revert,
> so it looked safe to delete the first narrowing instead of replacing it
> with ert-with-message-capture. Let me know if I've missed something
> there.

In autorevert-tests.el, the *Messages* buffer is narrowed systematically
prior any write-region call. I don't remember the details, but AFAIR
there were some other messages which have poisoned the tests.

I let it to you to change wherever you feel it is necessary. Tests will
show whether there are undesired side effects.

Best regards, Michael.

Message #13 received at 25158-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Gemini Lasswell <gazally <at> runbox.com>
Cc: 25158-done <at> debbugs.gnu.org, michael.albinus <at> gmx.de
Subject: Re: bug#25158: [PATCH] A better way for test code to access messages
Date: Sat, 04 Feb 2017 13:39:53 +0200

> Cc: Eli Zaretskii <eliz <at> gnu.org>, Michael Albinus <michael.albinus <at> gmx.de>
> From: Gemini Lasswell <gazally <at> runbox.com>
> Date: Sat, 10 Dec 2016 09:29:05 -0800
> 
> Here is a quote from Bug#24939's discussion, regarding the technique
> used by autorevert-tests.el, filenotify-tests.el and the proposed
> kmacro-tests.el to collect messages issued during part of a test by
> temporarily narrowing *Messages*:
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> > 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.
> >
> 
> The attached patch adds a new macro called ert-with-message-capture to
> ert-x.el which temporarily adds advice to 'message' to collect messages.
> I've also modified autorevert-tests.el and filenotify-tests.el to use
> the new macro. If or when the more thorough infrastructure is
> implemented, that could replace the use of advice in this macro but the
> tests which use it should not have to change.
> 
> Michael, in modifying autorevert-tests.el, at the start of
> auto-revert-test02-auto-revert-deleted-file, *Messages* was narrowed,
> and then narrowed again before the call to auto-revert--wait-for-revert,
> so it looked safe to delete the first narrowing instead of replacing it
> with ert-with-message-capture. Let me know if I've missed something
> there.

Making these changes didn't introduce any regressions in the relevant
tests, so I pushed them.

Gemini, please consider documenting the new macro in the ERT manual
and in NEWS.

Thanks.

Message #16 received at 25158-done <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 25158-done <at> debbugs.gnu.org, Gemini Lasswell <gazally <at> runbox.com>
Subject: Re: bug#25158: [PATCH] A better way for test code to access messages
Date: Sun, 05 Feb 2017 14:12:17 +0100

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

> Making these changes didn't introduce any regressions in the relevant
> tests, so I pushed them.

There is a new error when running under GNU/Linux from a shell:

--8<---------------cut here---------------start------------->8---
# make SELECTOR=\'file-notify-test03-autorevert-remote -C test filenotify-tests
[...]
Testing lisp/filenotify-tests.el
Running 1 tests (2017-02-05 11:42:33+0100)


Reverting buffer `file-notify-test141896xu'.

Error: (wrong-type-argument stringp nil)
Reverting buffer `file-notify-test141896xu'.

Error: (wrong-type-argument stringp nil)
   passed  1/1  file-notify-test03-autorevert-remote
--8<---------------cut here---------------end--------------->8---

The test passes, but the error message looks suspicious. I've tried to
find where it comes from, and it seems to be the new macro
`ert-with-message-capture'. I didn't debug further; maybe Gemini could
have a look?

The bug is not visible when running the test interactively.

> Thanks.

Best regards, Michael.

Message #19 received at 25158-done <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 25158-done <at> debbugs.gnu.org, Gemini Lasswell <gazally <at> runbox.com>
Subject: Re: bug#25158: [PATCH] A better way for test code to access messages
Date: Tue, 07 Feb 2017 21:46:57 +0100

Michael Albinus <michael.albinus <at> gmx.de> writes:

> Eli Zaretskii <eliz <at> gnu.org> writes:
>
>> Making these changes didn't introduce any regressions in the relevant
>> tests, so I pushed them.
>
> There is a new error when running under GNU/Linux from a shell:
>
> # make SELECTOR=\'file-notify-test03-autorevert-remote -C test filenotify-tests
> [...]
> Testing lisp/filenotify-tests.el
> Running 1 tests (2017-02-05 11:42:33+0100)
>
>
> Reverting buffer `file-notify-test141896xu'.
>
> Error: (wrong-type-argument stringp nil)
> Reverting buffer `file-notify-test141896xu'.
>
> Error: (wrong-type-argument stringp nil)
>    passed  1/1  file-notify-test03-autorevert-remote
>
> The test passes, but the error message looks suspicious. I've tried to
> find where it comes from, and it seems to be the new macro
> `ert-with-message-capture'. I didn't debug further; maybe Gemini could
> have a look?
>
> The bug is not visible when running the test interactively.

Finally, it is not a bug of `ert-with-message-capture'. It comes from
`vc-refresh-state', and it was not visible before using the new macro.

I've adapted the test case `file-notify-test03-autorevert'; it works
fine now. Bug#25158 could be closed.

>> Thanks.

Best regards, Michael.

Previous Next

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