Re: master 941a2e7: todo-mode: don't assume an ordering of tests

Re: master 941a2e7: todo-mode: don't assume an ordering of tests

[Stephen Berman]

On Mon, 29 May 2017 20:50:56 -0400 (EDT) rgm@gnu.org (Glenn Morris) wrote:

> branch: master
> commit 941a2e7347e3a0d393e67872e6151be8cc66d9a2
> Author: Glenn Morris <rgm@gnu.org>
> Commit: Glenn Morris <rgm@gnu.org>
>
>     todo-mode: don't assume an ordering of tests
>     
>     * test/lisp/calendar/todo-mode-tests.el (todo-test-todo-quit02)
>     (todo-test-item-highlighting): Avoid prompting for input file.
[...]
> @@ -93,6 +95,7 @@ current again."
>  If the buffer made current by invoking todo-quit in a todo-mode
>  buffer is buried by quit-window, the todo-mode buffer should not
>  become current."
> +  (todo-test-get-archive 2)
>    (todo-show)
>    (should (todo-test-is-current-buffer todo-test-file-1))
>    (let ((dir (dired default-directory)))
> @@ -105,6 +108,7 @@ become current."
>  (ert-deftest todo-test-item-highlighting () ; bug#27133
>    "Test whether `todo-toggle-item-highlighting' highlights whole item.
>  In particular, all lines of a multiline item should be highlighted."
> +  (todo-test-get-archive 2)
>    (todo-show)
>    (todo-jump-to-category nil "testcat1") ; For test rerun.
>    (todo-toggle-item-highlighting)

When I start emacs with -Q, load the test file, and then either run all
tests with `M-x ert RET t RET', or each individually with `M-x ert RET
"todo-test-todo-quit02" RET' etc. in any order, also with reruns, all
tests pass and I get no prompt for a file.  Same when invoking
`emacs -batch -l ert -l /path/to/test/lisp/calendar/todo-mode-tests.el -f
ert-run-tests-batch-and-exit' from the shell.

The tests todo-test-todo-quit02 and todo-test-item-highlighting
shouldn't have any need to display an archive buffer.  How did you run
the tests such that you got prompted for a file and what was the prompt?

Steve Berman

Re: master 941a2e7: todo-mode: don't assume an ordering of tests

[Glenn Morris]

Stephen Berman wrote:

> How did you run the tests such that you got prompted for a file

make check

> and what was the prompt?

"Enter name of new todo file (TAB or SPC to see current names):"

Ref: http://hydra.nixos.org/build/53544385
   
   Testing lisp/calendar/todo-mode-tests.el
   Running 3 tests (2017-05-29 22:44:07+0000)
   Enter name of new todo file (TAB or SPC to see current names): Test
   todo-test-item-highlighting backtrace:
     completing-read("Enter name of new todo file (TAB or SPC to see curr
     todo-read-file-name("Enter name of new todo file (TAB or SPC to see 
     todo-add-file()
     todo-show()

Re: master 941a2e7: todo-mode: don't assume an ordering of tests

[Stephen Berman]

On Tue, 30 May 2017 11:35:33 -0400 Glenn Morris <rgm@gnu.org> wrote:

> Stephen Berman wrote:
>
>> How did you run the tests such that you got prompted for a file
>
> make check

Hmm.  I cannot reproduce the failure.  I commented out your changes to
todo-mode-tests.el, deleted the .elc file, ran make check, and all the
tests in todo-mode-tests.el passed.  Do you have any idea why our
results differ?

>> and what was the prompt?
>
> "Enter name of new todo file (TAB or SPC to see current names):"
>
> Ref: http://hydra.nixos.org/build/53544385
>    
>    Testing lisp/calendar/todo-mode-tests.el
>    Running 3 tests (2017-05-29 22:44:07+0000)
>    Enter name of new todo file (TAB or SPC to see current names): Test
>    todo-test-item-highlighting backtrace:
>      completing-read("Enter name of new todo file (TAB or SPC to see curr
>      todo-read-file-name("Enter name of new todo file (TAB or SPC to see 
>      todo-add-file()
>      todo-show()

This backtrace means that the variables todo-current-todo-file,
todo-global-current-todo-file, and todo-default-todo-file all evaluated
to nil.  I don't see how that could happen with the test file, and when
I start Emacs with -Q, eval the test file, instrument todo-show, run ert
on the test todo-test-item-highlighting, and step through todo-show and
todo-check-file, I see that todo-default-todo-file gets assigned
todo-test-1.todo from the todo-resources directory as its value.  So I
have no idea how this error could arise.  And I also don't see how
adding the sexp `(todo-test-get-archive 2)' prevents it, since that just
displays the file todo-test-1.toda in todo-archive-mode (and that file
becomes the buffer-local value of todo-current-todo-file).

Can you reproduce the error, and if so, could you try debugging it?  Or
if you don't have time for that, could you tell me why you used that
specific change to avoid the error?  Maybe I'm overlooking something.

Steve Berman

Re: master 941a2e7: todo-mode: don't assume an ordering of tests

[Glenn Morris]

Stephen Berman wrote:

> Do you have any idea why our results differ?

I have no idea how todo-default-todo-file can get set to the right value
for you. (Maybe it's because you have files in ~/.emacs.d/todo, and I
don't?)

> This backtrace means that the variables todo-current-todo-file,
> todo-global-current-todo-file, and todo-default-todo-file all evaluated
> to nil.  I don't see how that could happen with the test file, and when
> I start Emacs with -Q, eval the test file, instrument todo-show, run ert
> on the test todo-test-item-highlighting, and step through todo-show and
> todo-check-file, I see that todo-default-todo-file gets assigned
> todo-test-1.todo from the todo-resources directory as its value.

 todo-default-todo-file is a variable defined in 'todo-mode.el'
 Its value is nil
 Original value was "todo-test-1"

It is nil because the test file requires todo-mode before it sets
todo-directory. It mismatches its default because the default changes
when todo-directory does. (This seems to me like a not great way for a
defcustom to behave.)

So another solution that works for me is to not require todo-mode till
after you set todo-directory. As the FIXME comment in the test file
indicates, this setting of global variables isn't a good way to behave.

BTW, the test will hang with a "Do you want to convert.."  prompt if an
old todo file exists in ~/.emacs.d. Maybe make a macro that sets HOME to
a temporary directory with a copy of the input files, and use it in each
test.

>   So I have no idea how this error could arise. And I also don't see
> how adding the sexp `(todo-test-get-archive 2)' prevents it, since
> that just displays the file todo-test-1.toda in todo-archive-mode (and
> that file becomes the buffer-local value of todo-current-todo-file).
>
> Can you reproduce the error,

Yes.

> and if so, could you try debugging it?

See above. :)

> if you don't have time for that, could you tell me why you used that
> specific change to avoid the error?

I looked at it very briefly, and thought: "nothing is telling
todo-test-item-highlighting which source file to use;
todo-test-get-archive is the only place in the file that seems to set a
source file; I'll stick that in; now it works.".

Re: master 941a2e7: todo-mode: don't assume an ordering of tests

[Stephen Berman]

On Tue, 30 May 2017 20:48:56 -0400 Glenn Morris <rgm@gnu.org> wrote:

> Stephen Berman wrote:
>
>> Do you have any idea why our results differ?
>
> I have no idea how todo-default-todo-file can get set to the right value
> for you. (Maybe it's because you have files in ~/.emacs.d/todo, and I
> don't?)

Yes, you're right.  That's what I overlooked :(.

>> This backtrace means that the variables todo-current-todo-file,
>> todo-global-current-todo-file, and todo-default-todo-file all evaluated
>> to nil.  I don't see how that could happen with the test file, and when
>> I start Emacs with -Q, eval the test file, instrument todo-show, run ert
>> on the test todo-test-item-highlighting, and step through todo-show and
>> todo-check-file, I see that todo-default-todo-file gets assigned
>> todo-test-1.todo from the todo-resources directory as its value.
>
>  todo-default-todo-file is a variable defined in 'todo-mode.el'
>  Its value is nil
>  Original value was "todo-test-1"
>
> It is nil because the test file requires todo-mode before it sets
> todo-directory. It mismatches its default because the default changes
> when todo-directory does. (This seems to me like a not great way for a
> defcustom to behave.)

Yes, this is bad, though outside of a test environment, it seems
unlikely that todo-directory will be changed, doesn't it?  Or do you see
a general way (i.e. not just for testing) around this problem?

> So another solution that works for me is to not require todo-mode till
> after you set todo-directory. As the FIXME comment in the test file
> indicates, this setting of global variables isn't a good way to behave.

Yes, that's an ugly workaround and putting it before the require seems
even uglier.

> BTW, the test will hang with a "Do you want to convert.."  prompt if an
> old todo file exists in ~/.emacs.d. Maybe make a macro that sets HOME to
> a temporary directory with a copy of the input files, and use it in each
> test.

Thanks, that's a good idea.

[...]
>> could you tell me why you used that
>> specific change to avoid the error?
>
> I looked at it very briefly, and thought: "nothing is telling
> todo-test-item-highlighting which source file to use;
> todo-test-get-archive is the only place in the file that seems to set a
> source file; I'll stick that in; now it works.".

It works, but it's rather rather opaque, since that test has nothing to
do with todo archives.  In lieu of a solution t theo todo-directory
problem, which would make the first todo-show in the test work, I've
replaced todo-test-get-archive with a function that displays a category
in either todo-mode or todo-archive-mode.  This, plus setting HOME,
eliminates the prompts and the tests pass make check.  Unless I can
quickly come up with a better fix, I'll commit this as a reasonable
stop-gap.

Thanks for giving first-aid to the tests, and for your feedback.

Steve Berman