Re: [Emacs-diffs] master 716b468: Extend `file-notify-test02-rm-watch'

Re: [Emacs-diffs] master 716b468: Extend `file-notify-test02-rm-watch'

[Ken Brown]

On 3/22/2017 9:16 AM, Michael Albinus wrote:
> branch: master
> commit 716b46848989bc343797d98488a7a0cc33ed3179
> Author: Michael Albinus <michael.albinus@gmx.de>
> Commit: Michael Albinus <michael.albinus@gmx.de>
>
>     Extend `file-notify-test02-rm-watch'
>
>     * test/lisp/filenotify-tests.el (file-notify-test02-rm-watch):
>     Expect it failed for inotify.  Divide tests into different
>     `unwind-protect' clauses.  Check, that removing watch
>     descriptors out of order do not harm.  (Bug#26126)

Hi Michael,

Starting with this commit, file-notify-test02-rm-watch fails on Cygwin:

$ make -C test filenotify-tests SELECTOR='\"file-notify-test02-rm-watch$$\"'

[...]

Test file-notify-test02-rm-watch condition:
     (ert-test-failed
      ((should
        (equal results
               (list 2)))
       :form
       (equal nil
              (2))
       :value nil :explanation
       (different-types nil
                        (2))))


This may be another of those timing issues that we've dealt with before, 
but I haven't had a chance yet to look at it in detail.

Ken

Re: [Emacs-diffs] master 716b468: Extend `file-notify-test02-rm-watch'

[Michael Albinus]

Ken Brown <kbrown@cornell.edu> writes:

> Hi Michael,

Hi Ken,

> Starting with this commit, file-notify-test02-rm-watch fails on Cygwin:
>
> $ make -C test filenotify-tests SELECTOR='\"file-notify-test02-rm-watch$$\"'
>
> This may be another of those timing issues that we've dealt with
> before, but I haven't had a chance yet to look at it in detail.

Maybe. I'm very short in time, but if you could provide me a recent
Emacs build for cygwin, I'll try to test it.

> Ken

Best regards, Michael.

Re: [Emacs-diffs] master 716b468: Extend `file-notify-test02-rm-watch'

[Ken Brown]

On 4/5/2017 2:58 AM, Michael Albinus wrote:
> Ken Brown <kbrown@cornell.edu> writes:
>> Starting with this commit, file-notify-test02-rm-watch fails on Cygwin:
>>
>> $ make -C test filenotify-tests SELECTOR='\"file-notify-test02-rm-watch$$\"'
>>
>> This may be another of those timing issues that we've dealt with
>> before, but I haven't had a chance yet to look at it in detail.
>
> Maybe. I'm very short in time, but if you could provide me a recent
> Emacs build for cygwin, I'll try to test it.

I've just uploaded one to my private Cygwin repository at

   http://sanibeltranquility.com/cygwin/index.html

There are instructions at that site.  Let me know if anything is unclear.

Thanks.

Ken

Re: [Emacs-diffs] master 716b468: Extend `file-notify-test02-rm-watch'

[Andreas Politz]

[-- Attachment #1: Type: text/plain, Size: 341 bytes --]

Ken Brown <kbrown@cornell.edu> writes:

> This may be another of those timing issues that we've dealt with
> before [...].

I think it is as well.  If cygwin's file-notify back-end runs on a polling
timer, we need to make sure that it has acknowledged the file's
existence before we delete it.  Maybe you could try out the following
patch.


[-- Attachment #2: Type: text/plain, Size: 561 bytes --]

diff --git a/test/lisp/filenotify-tests.el b/test/lisp/filenotify-tests.el
index 54e7ebfc0e..9b5c751021 100644
--- a/test/lisp/filenotify-tests.el
+++ b/test/lisp/filenotify-tests.el
@@ -425,6 +425,7 @@ file-notify--test-make-temp-name
                   '(change) #'second-callback)))
           ;; Remove first watch.
           (file-notify-rm-watch file-notify--test-desc)
+          (file-notify--test-read-event)
           ;; Only the second callback shall run.
           (delete-file file-notify--test-tmpfile)
           (file-notify--wait-for-events

[-- Attachment #3: Type: text/plain, Size: 5 bytes --]


-ap

Re: [Emacs-diffs] master 716b468: Extend `file-notify-test02-rm-watch'

[Ken Brown]

On 4/5/2017 5:02 PM, Andreas Politz wrote:
> Ken Brown <kbrown@cornell.edu> writes:
>
>> This may be another of those timing issues that we've dealt with
>> before [...].
>
> I think it is as well.  If cygwin's file-notify back-end runs on a polling
> timer, we need to make sure that it has acknowledged the file's
> existence before we delete it.  Maybe you could try out the following
> patch.
>
>
>
> diff --git a/test/lisp/filenotify-tests.el b/test/lisp/filenotify-tests.el
> index 54e7ebfc0e..9b5c751021 100644
> --- a/test/lisp/filenotify-tests.el
> +++ b/test/lisp/filenotify-tests.el
> @@ -425,6 +425,7 @@ file-notify--test-make-temp-name
>                    '(change) #'second-callback)))
>            ;; Remove first watch.
>            (file-notify-rm-watch file-notify--test-desc)
> +          (file-notify--test-read-event)
>            ;; Only the second callback shall run.
>            (delete-file file-notify--test-tmpfile)
>            (file-notify--wait-for-events

No, it still fails.

Ken

Re: [Emacs-diffs] master 716b468: Extend `file-notify-test02-rm-watch'

[Ken Brown]

On 4/5/2017 10:59 PM, Ken Brown wrote:
> On 4/5/2017 5:02 PM, Andreas Politz wrote:
>> Ken Brown <kbrown@cornell.edu> writes:
>>
>>> This may be another of those timing issues that we've dealt with
>>> before [...].
>>
>> I think it is as well.  If cygwin's file-notify back-end runs on a
>> polling
>> timer, we need to make sure that it has acknowledged the file's
>> existence before we delete it.  Maybe you could try out the following
>> patch.
>>
>>
>>
>> diff --git a/test/lisp/filenotify-tests.el
>> b/test/lisp/filenotify-tests.el
>> index 54e7ebfc0e..9b5c751021 100644
>> --- a/test/lisp/filenotify-tests.el
>> +++ b/test/lisp/filenotify-tests.el
>> @@ -425,6 +425,7 @@ file-notify--test-make-temp-name
>>                    '(change) #'second-callback)))
>>            ;; Remove first watch.
>>            (file-notify-rm-watch file-notify--test-desc)
>> +          (file-notify--test-read-event)
>>            ;; Only the second callback shall run.
>>            (delete-file file-notify--test-tmpfile)
>>            (file-notify--wait-for-events
>
> No, it still fails.

You're on the right track, however.  The following patch does fix the 
problem:

--- a/test/lisp/filenotify-tests.el
+++ b/test/lisp/filenotify-tests.el
@@ -425,6 +425,7 @@ file-notify--test-make-temp-name
                    '(change) #'second-callback)))
            ;; Remove first watch.
            (file-notify-rm-watch file-notify--test-desc)
+          (sit-for 0.1)
            ;; Only the second callback shall run.
            (delete-file file-notify--test-tmpfile)
            (file-notify--wait-for-events

Ken

Re: [Emacs-diffs] master 716b468: Extend `file-notify-test02-rm-watch'

[Michael Albinus]

Ken Brown <kbrown@cornell.edu> writes:

Hi Ken,

> You're on the right track, however.  The following patch does fix the
> problem:

Finally, I've got the opportunity to test it under cygwin. I've
committed a patch which is similar to your proposal, and which fixes it
in my environment. Could you, pls, crosscheck?

One point still irritates me: the tests run under cygwin change the
results after a while. Several tests which pass after a reboot of a
machine, will fail when I use the machine for some hours. It isn't the
first time I've observed this, so I need to reboot my machine again and
again when working for cygwin.

Do you know this behaviour?

> Ken

Best regards, Michael.

Re: [Emacs-diffs] master 716b468: Extend `file-notify-test02-rm-watch'

[Ken Brown]

Hi Michael,

On 5/1/2017 6:21 AM, Michael Albinus wrote:
> Finally, I've got the opportunity to test it under cygwin. I've
> committed a patch which is similar to your proposal, and which fixes it
> in my environment. Could you, pls, crosscheck?

On my systems I seem to need a much larger sit-for timeout (on the order 
of 0.1 or more rather than 0.001) in order for the test to pass.  But 
maybe this is not worth worrying about, since it's clear that this is a 
timing issue rather than a file-notify problem.

> One point still irritates me: the tests run under cygwin change the
> results after a while. Several tests which pass after a reboot of a
> machine, will fail when I use the machine for some hours. It isn't the
> first time I've observed this, so I need to reboot my machine again and
> again when working for cygwin.
>
> Do you know this behaviour?

No, I don't see this.  I hardly ever reboot my computer.  But I mostly 
run 64-bit Cygwin.  Are you running 32-bit Cygwin?  And are the problems 
related to fork failures?  (You would see error messages to this effect 
on the terminal or in the logs.)  If so, I might be able to help.  But 
in that case we should probably continue the discussion off-list.

Regards,

Ken

Re: [Emacs-diffs] master 716b468: Extend `file-notify-test02-rm-watch'

[Michael Albinus]

Ken Brown <kbrown@cornell.edu> writes:

> Hi Michael,

Hi Ken,

> On my systems I seem to need a much larger sit-for timeout (on the
> order of 0.1 or more rather than 0.001) in order for the test to pass.
> But maybe this is not worth worrying about, since it's clear that this
> is a timing issue rather than a file-notify problem.

I've moved the `sit-for' call to `file-notify--test-read-event' by
intention, so we have just one place to change. You could do something
like

(if (eq system-type 'cygwin)
  (sit-for 0.1 'nodisp))

The other backends do not need them as far as I'm aware of, so do it at
your convenience. Maybe also with a comment ...

>> One point still irritates me: the tests run under cygwin change the
>> results after a while. Several tests which pass after a reboot of a
>> machine, will fail when I use the machine for some hours. It isn't the
>> first time I've observed this, so I need to reboot my machine again and
>> again when working for cygwin.
>>
>> Do you know this behaviour?
>
> No, I don't see this.  I hardly ever reboot my computer.  But I mostly
> run 64-bit Cygwin.  Are you running 32-bit Cygwin?  And are the
> problems related to fork failures?  (You would see error messages to
> this effect on the terminal or in the logs.)  If so, I might be able
> to help.  But in that case we should probably continue the discussion
> off-list.

Likely 32-bit. But I use it very rarely, so I don't want to waste your
time with debugging when you don't know it from somewhere else.

> Regards,
>
> Ken

Best regards, Michael.

Re: [Emacs-diffs] master 716b468: Extend `file-notify-test02-rm-watch'

[Ken Brown]

On 5/1/2017 2:45 PM, Michael Albinus wrote:

Hi Michael,

> I've moved the `sit-for' call to `file-notify--test-read-event' by
> intention, so we have just one place to change. You could do something
> like
>
> (if (eq system-type 'cygwin)
>   (sit-for 0.1 'nodisp))
>
> The other backends do not need them as far as I'm aware of, so do it at
> your convenience. Maybe also with a comment ...

I can find a sit-for time that works reliably when I repeatedly run 
file-notify-test02-rm-watch.  But then that test still fails when it is 
run as part of 'make check'.

My inclination at this point is to simply skip this test on Cygwin.  Is 
that OK with you?

Regards,

Ken

Re: [Emacs-diffs] master 716b468: Extend `file-notify-test02-rm-watch'

[Michael Albinus]

Ken Brown <kbrown@cornell.edu> writes:

> Hi Michael,

Hi Ken,

> I can find a sit-for time that works reliably when I repeatedly run
> file-notify-test02-rm-watch.  But then that test still fails when it
> is run as part of 'make check'.
>
> My inclination at this point is to simply skip this test on Cygwin.
> Is that OK with you?

For sure. Pls remove also the sit-for I've added for that test under cygwin.

> Regards,
>
> Ken

Best regards, Michael.