Some testing issues

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Some testing issues

Stephen Berman
I installed some new tests for todo-mode.el in master c24748a and there
were a few problems that I don't understand; maybe someone here can
enlighten me.

Several of the tests call the function todo-toggle-view-done-items,
which contains this code:

          (if (not (pos-visible-in-window-p shown))
              (recenter)

(`shown' stores the position of the first done item in the category, if
it is shown.)  When using todo-mode and also when stepping through the
function with Edebug, it works as expected.  But when running the tests,
sometimes -- but not always -- it fails with the error "`recenter'ing a
window that does not display current-buffer."  In those cases, the test
failure is prevented by evaluating the following sexp immediately before
calling todo-toggle-view-done-items:

          (set-window-buffer nil (current-buffer))

When using todo-mode this is not necessary, because the selected window
always contains the current buffer when the function is called, but
apparently when running the tests, this isn't always the case, although
according to Edebug it is, AFAICT.  And again, the failure only happens
in some cases, though in those, it is reliably reproducible.  In
addition, in at least one case, when I rerun the test immediately after
it fails, then it succeeds.  And lastly, in one case, the test succeeds
without the set-window-buffer call when run interactively, but fails
without it when run in batch mode from the shell.  Any idea what's going
on here?  (If anyone wants to take a closer look, comments in the test
file point out the problematic cases.)

A second problem concerns trying to reference a message in a test.  The
function todo-toggle-view-done-items outputs a message if the category
contains no done items, and the test captures this by calling
current-message immediately after todo-toggle-view-done-items.  This
works when running the test interactively, but in a batch run,
current-message is nil (though the message is output in the shell).
Why?  (I avoid the failure in batch mode by testing a different but
effectively equivalent condition.)

Finally, one of the tests involves a display overlay that hides the item
header, which as a consequence, when using todo-mode, prevented the
cursor from appearing where the code put point, resulting in a display
bug.  I fixed this (in master 264dd81) by moving point to the first
visible position after the overlay.  But when running the test, the
overlay evidently does not inhibit point, but since the test, following
the bugfix code in todo-mode.el, assumes it does, it fails.  It seems
that the test environment does not reflect the same display mechanisms
that using the code does.  I haven't found a solution or workaround for
this, so the test is currently marked ":expected-result :failed".  I
hope someone can come up with a better alternative.

I'd be grateful for any suggestions or advice.

Steve Berman

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Some testing issues

npostavs
On Fri, Jul 7, 2017 at 5:22 PM, Stephen Berman <[hidden email]> wrote:
> Several of the tests call the function todo-toggle-view-done-items,
> which contains this code:
>
>           (if (not (pos-visible-in-window-p shown))
>               (recenter)
>
> (`shown' stores the position of the first done item in the category, if
> it is shown.)  When using todo-mode and also when stepping through the
> function with Edebug, it works as expected.

If I comment out the set-window-buffer call it fails for me in Edebug
as well (when running the first time).

By the way, in order to run successfully I did also need the following
patch, otherwise all the tests failed (I think it only happens for me
because I'm running from an Emacs that's living under my home
directory).

modified   test/lisp/calendar/todo-mode-tests.el
@@ -46,6 +46,7 @@ (defmacro with-todo-test (&rest body)
   "Set up an isolated todo-mode test environment."
   (declare (debug (body)))
   `(let* ((todo-test-home (make-temp-file "todo-test-home-" t))
+          (abbreviated-home-dir nil)
           (process-environment (cons (format "HOME=%s" todo-test-home)
                                      process-environment))
           (todo-directory todo-test-data-dir)

>  But when running the tests,
> sometimes -- but not always -- it fails with the error "`recenter'ing a
> window that does not display current-buffer."  In those cases, the test
> failure is prevented by evaluating the following sexp immediately before
> calling todo-toggle-view-done-items:
>
>           (set-window-buffer nil (current-buffer))
>
> When using todo-mode this is not necessary, because the selected window
> always contains the current buffer when the function is called, but
> apparently when running the tests, this isn't always the case, although
> according to Edebug it is, AFAICT.

When I evaluate (selected-window) in edebug before the call, I see
that the selected window is showing the *ert* buffer.

>  And again, the failure only happens
> in some cases, though in those, it is reliably reproducible.  In
> addition, in at least one case, when I rerun the test immediately after
> it fails, then it succeeds.

I think it succeeds the second time because the *ert* buffer is in a
different state.

>  And lastly, in one case, the test succeeds
> without the set-window-buffer call when run interactively, but fails
> without it when run in batch mode from the shell.  Any idea what's going
> on here?  (If anyone wants to take a closer look, comments in the test
> file point out the problematic cases.)

todo-test-toggle-item-header04? I added a `message' call, and it seems
that in batch mode the selected window shows *scratch* whereas in
interactive mode it shows *ert*. I would say the success in
interactive mode is just a coincidence.

>
> A second problem concerns trying to reference a message in a test.  The
> function todo-toggle-view-done-items outputs a message if the category
> contains no done items, and the test captures this by calling
> current-message immediately after todo-toggle-view-done-items.  This
> works when running the test interactively, but in a batch run,
> current-message is nil (though the message is output in the shell).
> Why?

I think it's simply that there is no echo-area or any concept of a
"current" message when running in batch mode.
lread-tests--last-message might be worth looking at:

(defun lread-tests--last-message ()
  (with-current-buffer "*Messages*"
    (save-excursion
      (goto-char (point-max))
      (skip-chars-backward "\n")
      (buffer-substring (line-beginning-position) (point)))))

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Some testing issues

Stefan Monnier
In reply to this post by Stephen Berman
> Several of the tests call the function todo-toggle-view-done-items,
> which contains this code:
>
>  (if (not (pos-visible-in-window-p shown))
>      (recenter)

This code is indeed brittle: pos-visible-in-window-p presumes that
(current-buffer) is the same as (window-buffer (selected-window)), but
that's not always the case.


        Stefan


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Some testing issues

martin rudalics
In reply to this post by Stephen Berman
 > And lastly, in one case, the test succeeds
 > without the set-window-buffer call when run interactively, but fails
 > without it when run in batch mode from the shell.  Any idea what's going
 > on here?  (If anyone wants to take a closer look, comments in the test
 > file point out the problematic cases.)

The interactive call might succeed because command_loop_1 makes the
selected window's buffer current in

       set_buffer_internal (XBUFFER (XWINDOW (selected_window)->contents));

Supposedly, testing ‘recenter’ doesn't make much sense without a frame.

 > A second problem concerns trying to reference a message in a test.  The
 > function todo-toggle-view-done-items outputs a message if the category
 > contains no done items, and the test captures this by calling
 > current-message immediately after todo-toggle-view-done-items.  This
 > works when running the test interactively, but in a batch run,
 > current-message is nil (though the message is output in the shell).
 > Why?  (I avoid the failure in batch mode by testing a different but
 > effectively equivalent condition.)

‘current-message’ returns "the string currently displayed in the echo
area" and there's no echo area in batch mode.

 > Finally, one of the tests involves a display overlay that hides the item
 > header, which as a consequence, when using todo-mode, prevented the
 > cursor from appearing where the code put point, resulting in a display
 > bug.  I fixed this (in master 264dd81) by moving point to the first
 > visible position after the overlay.  But when running the test, the
 > overlay evidently does not inhibit point, but since the test, following
 > the bugfix code in todo-mode.el, assumes it does, it fails.  It seems
 > that the test environment does not reflect the same display mechanisms
 > that using the code does.  I haven't found a solution or workaround for
 > this, so the test is currently marked ":expected-result :failed".  I
 > hope someone can come up with a better alternative.

You need a frame to display something and there's none in batch mode.
Hence you can't test display features in batch mode.

martin


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Some testing issues

Stephen Berman
In reply to this post by npostavs
On Sat, 8 Jul 2017 00:20:23 -0400 Noam Postavsky <[hidden email]> wrote:

> On Fri, Jul 7, 2017 at 5:22 PM, Stephen Berman <[hidden email]> wrote:
>> Several of the tests call the function todo-toggle-view-done-items,
>> which contains this code:
>>
>>           (if (not (pos-visible-in-window-p shown))
>>               (recenter)
>>
>> (`shown' stores the position of the first done item in the category, if
>> it is shown.)  When using todo-mode and also when stepping through the
>> function with Edebug, it works as expected.
>
> If I comment out the set-window-buffer call it fails for me in Edebug
> as well (when running the first time).

You're right, I must have misrembered or been thinking of a different
case; sorry.

> By the way, in order to run successfully I did also need the following
> patch, otherwise all the tests failed (I think it only happens for me
> because I'm running from an Emacs that's living under my home
> directory).
>
> modified   test/lisp/calendar/todo-mode-tests.el
> @@ -46,6 +46,7 @@ (defmacro with-todo-test (&rest body)
>    "Set up an isolated todo-mode test environment."
>    (declare (debug (body)))
>    `(let* ((todo-test-home (make-temp-file "todo-test-home-" t))
> +          (abbreviated-home-dir nil)
>            (process-environment (cons (format "HOME=%s" todo-test-home)
>                                       process-environment))
>            (todo-directory todo-test-data-dir)

The only test file that sets abbreviated-home-dir is package-test.el, in
the macro with-package-test, which was indeed the inspiration for
with-todo-test.  I assume this would only effect cases like yours, and
hence make the test environment more robust, or are possible problems
that setting it to nil could raise?

>>  But when running the tests,
>> sometimes -- but not always -- it fails with the error "`recenter'ing a
>> window that does not display current-buffer."  In those cases, the test
>> failure is prevented by evaluating the following sexp immediately before
>> calling todo-toggle-view-done-items:
>>
>>           (set-window-buffer nil (current-buffer))
>>
>> When using todo-mode this is not necessary, because the selected window
>> always contains the current buffer when the function is called, but
>> apparently when running the tests, this isn't always the case, although
>> according to Edebug it is, AFAICT.
>
> When I evaluate (selected-window) in edebug before the call, I see
> that the selected window is showing the *ert* buffer.

Yes, I should have checked again before posting that.

>>  And again, the failure only happens
>> in some cases, though in those, it is reliably reproducible.  In
>> addition, in at least one case, when I rerun the test immediately after
>> it fails, then it succeeds.
>
> I think it succeeds the second time because the *ert* buffer is in a
> different state.

What state is that?  In Edebug it appears to be the same as on the first
test run: current-buffer is todo-test-1.todo and selected-window is the
one showing the *ert* buffer, yet now (pos-visible-in-window-p shown) is
non-nil, while on the first run it is nil.  I don't see what makes the
difference -- certainly not the value of the variable `shown', which is
226 and that position in window displaying *ert* is visible in both runs.

>>  And lastly, in one case, the test succeeds
>> without the set-window-buffer call when run interactively, but fails
>> without it when run in batch mode from the shell.  Any idea what's going
>> on here?  (If anyone wants to take a closer look, comments in the test
>> file point out the problematic cases.)
>
> todo-test-toggle-item-header04? I added a `message' call, and it seems
> that in batch mode the selected window shows *scratch* whereas in
> interactive mode it shows *ert*. I would say the success in
> interactive mode is just a coincidence.

Well, it's a reliably reproducible coincidence, which seems like a
contradiction in terms.

>> A second problem concerns trying to reference a message in a test.  The
>> function todo-toggle-view-done-items outputs a message if the category
>> contains no done items, and the test captures this by calling
>> current-message immediately after todo-toggle-view-done-items.  This
>> works when running the test interactively, but in a batch run,
>> current-message is nil (though the message is output in the shell).
>> Why?
>
> I think it's simply that there is no echo-area or any concept of a
> "current" message when running in batch mode.

Yes, as Martin also pointed out; I didn't realize (or stop to think
about) that.

> lread-tests--last-message might be worth looking at:
>
> (defun lread-tests--last-message ()
>   (with-current-buffer "*Messages*"
>     (save-excursion
>       (goto-char (point-max))
>       (skip-chars-backward "\n")
>       (buffer-substring (line-beginning-position) (point)))))

Thanks for the pointer, and for the feedback.

Steve Berman

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Some testing issues

Stephen Berman
In reply to this post by martin rudalics
On Sat, 08 Jul 2017 09:18:08 +0200 martin rudalics <[hidden email]> wrote:

>> And lastly, in one case, the test succeeds
>> without the set-window-buffer call when run interactively, but fails
>> without it when run in batch mode from the shell.  Any idea what's going
>> on here?  (If anyone wants to take a closer look, comments in the test
>> file point out the problematic cases.)
>
> The interactive call might succeed because command_loop_1 makes the
> selected window's buffer current in
>
>       set_buffer_internal (XBUFFER (XWINDOW (selected_window)->contents));
>
> Supposedly, testing ‘recenter’ doesn't make much sense without a frame.

True, though when I step through the code in the interactive test, the
condition (pos-visible-in-window-p shown) is true, so recenter is not
called; so the question to me is why this is not the case in the batch run.

>> A second problem concerns trying to reference a message in a test.  The
>> function todo-toggle-view-done-items outputs a message if the category
>> contains no done items, and the test captures this by calling
>> current-message immediately after todo-toggle-view-done-items.  This
>> works when running the test interactively, but in a batch run,
>> current-message is nil (though the message is output in the shell).
>> Why?  (I avoid the failure in batch mode by testing a different but
>> effectively equivalent condition.)
>
> ‘current-message’ returns "the string currently displayed in the echo
> area" and there's no echo area in batch mode.

Yes, as Noam also pointed out; I didn't think carefully enough about the
use of current-message.

>> Finally, one of the tests involves a display overlay that hides the item
>> header, which as a consequence, when using todo-mode, prevented the
>> cursor from appearing where the code put point, resulting in a display
>> bug.  I fixed this (in master 264dd81) by moving point to the first
>> visible position after the overlay.  But when running the test, the
>> overlay evidently does not inhibit point, but since the test, following
>> the bugfix code in todo-mode.el, assumes it does, it fails.  It seems
>> that the test environment does not reflect the same display mechanisms
>> that using the code does.  I haven't found a solution or workaround for
>> this, so the test is currently marked ":expected-result :failed".  I
>> hope someone can come up with a better alternative.
>
> You need a frame to display something and there's none in batch mode.
> Hence you can't test display features in batch mode.

Thanks for clarifying that for me.  I guess I'll have to leave such
features untested, or is there an alternative?

Thanks for the feedback.

Steve Berman

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Some testing issues

Stephen Berman
In reply to this post by Stefan Monnier
On Sat, 08 Jul 2017 00:45:10 -0400 Stefan Monnier <[hidden email]> wrote:

>> Several of the tests call the function todo-toggle-view-done-items,
>> which contains this code:
>>
>>  (if (not (pos-visible-in-window-p shown))
>>      (recenter)
>
> This code is indeed brittle: pos-visible-in-window-p presumes that
> (current-buffer) is the same as (window-buffer (selected-window)), but
> that's not always the case.

Is there a less brittle way to test the visibility of point?  At least
as far as using todo-toggle-view-done-items in todo-mode in concerned,
(current-buffer) is equivalent to (window-buffer (selected-window)), but
apparently that is not so in the test environment.

Steve Berman

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Some testing issues

npostavs
In reply to this post by Stephen Berman
On Sat, Jul 8, 2017 at 10:50 AM, Stephen Berman <[hidden email]> wrote:

>>    `(let* ((todo-test-home (make-temp-file "todo-test-home-" t))
>> +          (abbreviated-home-dir nil)
>>            (process-environment (cons (format "HOME=%s" todo-test-home)
>>                                       process-environment))
>
> The only test file that sets abbreviated-home-dir is package-test.el, in
> the macro with-package-test, which was indeed the inspiration for
> with-todo-test.  I assume this would only effect cases like yours, and
> hence make the test environment more robust, or are possible problems
> that setting it to nil could raise?

abbreviated-home-dir is essentially a cache used by
abbreviate-file-name, and when the value of HOME is changed the cached
value is wrong, hence why setting it to nil is the right thing.
Possibly we should record the value of HOME we cached and clear it
automatically when a new one is used so that this kind of thing is not
needed.

>>
>> I think it succeeds the second time because the *ert* buffer is in a
>> different state.
>
> What state is that?  In Edebug it appears to be the same as on the first
> test run: current-buffer is todo-test-1.todo and selected-window is the
> one showing the *ert* buffer, yet now (pos-visible-in-window-p shown) is
> non-nil, while on the first run it is nil.  I don't see what makes the
> difference -- certainly not the value of the variable `shown', which is
> 226 and that position in window displaying *ert* is visible in both runs.

Maybe the set-window-buffer from the other tests leaks in? For some
reason I can't reproduce this today, every time I run with the
set-window-buffer commented out it consistently fails. I'm sure
yesterday I saw it succeeding after the first time.

>> todo-test-toggle-item-header04? I added a `message' call, and it seems
>> that in batch mode the selected window shows *scratch* whereas in
>> interactive mode it shows *ert*. I would say the success in
>> interactive mode is just a coincidence.
>
> Well, it's a reliably reproducible coincidence, which seems like a
> contradiction in terms.

I mean "coincidence" in the same way that the 5th digit of pi being 5
is a "coincidence" (slightly less reliable than that though,
presumably if we made `initial-scratch-message' long enough the batch
mode behaviour would change).

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Some testing issues

martin rudalics
In reply to this post by Stephen Berman
 > True, though when I step through the code in the interactive test, the
 > condition (pos-visible-in-window-p shown) is true, so recenter is not
 > called; so the question to me is why this is not the case in the batch run.

‘pos-visible-in-window-p’ is specified as

DEFUN ("pos-visible-in-window-p", Fpos_visible_in_window_p,
   ...
   Lisp_Object in_window = Qnil;

   ...
   return in_window;

so if none of the conditions it checks succeeds, it will return nil.

 > Thanks for clarifying that for me.  I guess I'll have to leave such
 > features untested, or is there an alternative?

You can wrap them in a (skip-unless (display-graphic-p)) which means
that they are allowed to fail in interactive use only.

martin


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Some testing issues

Stefan Monnier
In reply to this post by Stephen Berman
> Is there a less brittle way to test the visibility of point?

Of course, but you first have to use a less brittle question:
which "point"?  If you just use "point" it normally refer to a location
in the current buffer regardless of any window and hence "visibility"
is ill-defined.  So to make it less brittle, you first have to
explicitly select the window that interests you (i.e. look for a window
which displays the current buffer).


        Stefan

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Some testing issues

Stephen Berman
In reply to this post by npostavs
On Sat, 8 Jul 2017 18:01:12 -0400 Noam Postavsky <[hidden email]> wrote:

> On Sat, Jul 8, 2017 at 10:50 AM, Stephen Berman <[hidden email]> wrote:
>>>    `(let* ((todo-test-home (make-temp-file "todo-test-home-" t))
>>> +          (abbreviated-home-dir nil)
>>>            (process-environment (cons (format "HOME=%s" todo-test-home)
>>>                                       process-environment))
>>
>> The only test file that sets abbreviated-home-dir is package-test.el, in
>> the macro with-package-test, which was indeed the inspiration for
>> with-todo-test.  I assume this would only effect cases like yours, and
>> hence make the test environment more robust, or are possible problems
>> that setting it to nil could raise?
>
> abbreviated-home-dir is essentially a cache used by
> abbreviate-file-name, and when the value of HOME is changed the cached
> value is wrong, hence why setting it to nil is the right thing.
> Possibly we should record the value of HOME we cached and clear it
> automatically when a new one is used so that this kind of thing is not
> needed.

In the mean time, it sounds like setting it to nil is appropriate.

>>>
>>> I think it succeeds the second time because the *ert* buffer is in a
>>> different state.
>>
>> What state is that?  In Edebug it appears to be the same as on the first
>> test run: current-buffer is todo-test-1.todo and selected-window is the
>> one showing the *ert* buffer, yet now (pos-visible-in-window-p shown) is
>> non-nil, while on the first run it is nil.  I don't see what makes the
>> difference -- certainly not the value of the variable `shown', which is
>> 226 and that position in window displaying *ert* is visible in both runs.
>
> Maybe the set-window-buffer from the other tests leaks in? For some
> reason I can't reproduce this today, every time I run with the
> set-window-buffer commented out it consistently fails. I'm sure
> yesterday I saw it succeeding after the first time.

There should be no leakage, because the state is cleared after each
individual test run (at least, that's what with-todo-test is supposed to
do).

>>> todo-test-toggle-item-header04? I added a `message' call, and it seems
>>> that in batch mode the selected window shows *scratch* whereas in
>>> interactive mode it shows *ert*. I would say the success in
>>> interactive mode is just a coincidence.
>>
>> Well, it's a reliably reproducible coincidence, which seems like a
>> contradiction in terms.
>
> I mean "coincidence" in the same way that the 5th digit of pi being 5
> is a "coincidence" (slightly less reliable than that though,
> presumably if we made `initial-scratch-message' long enough the batch
> mode behaviour would change).

Ah, ok.

Steve Berman

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Some testing issues

Stephen Berman
In reply to this post by martin rudalics
On Sun, 09 Jul 2017 09:46:10 +0200 martin rudalics <[hidden email]> wrote:

>> True, though when I step through the code in the interactive test, the
>> condition (pos-visible-in-window-p shown) is true, so recenter is not
>> called; so the question to me is why this is not the case in the batch run.
>
> ‘pos-visible-in-window-p’ is specified as
>
> DEFUN ("pos-visible-in-window-p", Fpos_visible_in_window_p,
>   ...
>   Lisp_Object in_window = Qnil;
>
>   ...
>   return in_window;
>
> so if none of the conditions it checks succeeds, it will return nil.

I see; thanks.

>> Thanks for clarifying that for me.  I guess I'll have to leave such
>> features untested, or is there an alternative?
>
> You can wrap them in a (skip-unless (display-graphic-p)) which means
> that they are allowed to fail in interactive use only.

Seems like a good idea; thanks for the pointer.

Steve Berman

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Some testing issues

Stephen Berman
In reply to this post by Stefan Monnier
On Mon, 10 Jul 2017 13:33:15 -0400 Stefan Monnier <[hidden email]> wrote:

>> Is there a less brittle way to test the visibility of point?
>
> Of course, but you first have to use a less brittle question:
> which "point"?  If you just use "point" it normally refer to a location
> in the current buffer regardless of any window and hence "visibility"
> is ill-defined.  So to make it less brittle, you first have to
> explicitly select the window that interests you (i.e. look for a window
> which displays the current buffer).

Well, as I noted, when pos-visible-in-window-p is called in
todo-toggle-view-done-items, the selected window is displaying the
current buffer and AFAICT that can't go wrong when using todo-mode as
intended (i.e., not invoking todo-toggle-view-done-items outside of
todo-mode).  So I don't see any need to explicitly select the window
here.  In the test environment, however, that seems not to be an
invariant, so calling set-window-buffer is apparently needed.

Steve Berman

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Some testing issues

Stefan Monnier
> Well, as I noted, when pos-visible-in-window-p is called in
> todo-toggle-view-done-items, the selected window is displaying the
> current buffer and AFAICT that can't go wrong when using todo-mode as
> intended (i.e., not invoking todo-toggle-view-done-items outside of
> todo-mode).

Then I suggest you add a (cl-assert (eq (current-buffer)
(window-buffer))) and then declare that it's the caller's responsibility
(e.g. the test environment) to make sure this is true.

This said, another approach is to say that `recenter` is simply not
needed in case the buffer is not displayed anywhere (which could
presumably happen if you call this code from some ad-hoc Elisp
function), so if (eq (current-buffer) (window-buffer)) is nil, just
don't bother checking visibility nor recentering.


        Stefan

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Some testing issues

Stephen Berman
On Fri, 14 Jul 2017 09:44:10 -0400 Stefan Monnier <[hidden email]> wrote:

>> Well, as I noted, when pos-visible-in-window-p is called in
>> todo-toggle-view-done-items, the selected window is displaying the
>> current buffer and AFAICT that can't go wrong when using todo-mode as
>> intended (i.e., not invoking todo-toggle-view-done-items outside of
>> todo-mode).
>
> Then I suggest you add a (cl-assert (eq (current-buffer)
> (window-buffer))) and then declare that it's the caller's responsibility
> (e.g. the test environment) to make sure this is true.

The tests have assumed this responsibility with the invocation of
set-window-buffer.

> This said, another approach is to say that `recenter` is simply not
> needed in case the buffer is not displayed anywhere (which could
> presumably happen if you call this code from some ad-hoc Elisp
> function), so if (eq (current-buffer) (window-buffer)) is nil, just
> don't bother checking visibility nor recentering.

If I'm not misunderstanding you, I think adding this check, or the above
assertion, to this defun in todo-mode.el is an unwarrented precaution,
because it assumes a use of todo-toggle-view-done-items other than the
one it was defined for.  In principle that's possible, but the same goes
for any Elisp command, and the vast majority of them surely take no such
precautions.  There are a few todo-mode commands that are intended to be
called from outside of todo-mode, and for these I have tried to make
sure they DTRT, but for the others, as with most specialized elisp
commands, don't you think it's reasonable, and sufficient, to rely on
the user's common sense?  (The case of tests is, of course, different,
but as you note, it is (or should be) their responsibility to satisfy
the assumptions of the code being tested, I think even if the
assumptions aren't made explicit in the code.)

Steve Berman

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Some testing issues

Stefan Monnier
> If I'm not misunderstanding you, I think adding this check, or the above
> assertion, to this defun in todo-mode.el is an unwarrented precaution,

I don't understand enough of the overall use of the code to
know which of the two options is best.  If I had to choose, I'd ask you,


        Stefan

Loading...