unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
* Re: [Emacs-diffs] master 716b468: Extend `file-notify-test02-rm-watch'
       [not found] ` <20170322131611.5FF7D20E17@vcs0.savannah.gnu.org>
@ 2017-04-04 22:36   ` Ken Brown
  2017-04-05  6:58     ` Michael Albinus
  2017-04-05 21:02     ` Andreas Politz
  0 siblings, 2 replies; 11+ messages in thread
From: Ken Brown @ 2017-04-04 22:36 UTC (permalink / raw)
  To: emacs-devel, Michael Albinus

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



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Emacs-diffs] master 716b468: Extend `file-notify-test02-rm-watch'
  2017-04-04 22:36   ` [Emacs-diffs] master 716b468: Extend `file-notify-test02-rm-watch' Ken Brown
@ 2017-04-05  6:58     ` Michael Albinus
  2017-04-05 19:24       ` Ken Brown
  2017-04-05 21:02     ` Andreas Politz
  1 sibling, 1 reply; 11+ messages in thread
From: Michael Albinus @ 2017-04-05  6:58 UTC (permalink / raw)
  To: Ken Brown; +Cc: emacs-devel

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.



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Emacs-diffs] master 716b468: Extend `file-notify-test02-rm-watch'
  2017-04-05  6:58     ` Michael Albinus
@ 2017-04-05 19:24       ` Ken Brown
  0 siblings, 0 replies; 11+ messages in thread
From: Ken Brown @ 2017-04-05 19:24 UTC (permalink / raw)
  To: Michael Albinus; +Cc: emacs-devel

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




^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Emacs-diffs] master 716b468: Extend `file-notify-test02-rm-watch'
  2017-04-04 22:36   ` [Emacs-diffs] master 716b468: Extend `file-notify-test02-rm-watch' Ken Brown
  2017-04-05  6:58     ` Michael Albinus
@ 2017-04-05 21:02     ` Andreas Politz
  2017-04-06  2:59       ` Ken Brown
  1 sibling, 1 reply; 11+ messages in thread
From: Andreas Politz @ 2017-04-05 21:02 UTC (permalink / raw)
  To: Ken Brown; +Cc: Michael Albinus, emacs-devel

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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [Emacs-diffs] master 716b468: Extend `file-notify-test02-rm-watch'
  2017-04-05 21:02     ` Andreas Politz
@ 2017-04-06  2:59       ` Ken Brown
  2017-04-07 14:56         ` Ken Brown
  0 siblings, 1 reply; 11+ messages in thread
From: Ken Brown @ 2017-04-06  2:59 UTC (permalink / raw)
  To: Andreas Politz; +Cc: Michael Albinus, emacs-devel

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




^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Emacs-diffs] master 716b468: Extend `file-notify-test02-rm-watch'
  2017-04-06  2:59       ` Ken Brown
@ 2017-04-07 14:56         ` Ken Brown
  2017-05-01 10:21           ` Michael Albinus
  0 siblings, 1 reply; 11+ messages in thread
From: Ken Brown @ 2017-04-07 14:56 UTC (permalink / raw)
  To: Andreas Politz; +Cc: Michael Albinus, emacs-devel

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



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Emacs-diffs] master 716b468: Extend `file-notify-test02-rm-watch'
  2017-04-07 14:56         ` Ken Brown
@ 2017-05-01 10:21           ` Michael Albinus
  2017-05-01 18:28             ` Ken Brown
  0 siblings, 1 reply; 11+ messages in thread
From: Michael Albinus @ 2017-05-01 10:21 UTC (permalink / raw)
  To: Ken Brown; +Cc: Andreas Politz, emacs-devel

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.



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Emacs-diffs] master 716b468: Extend `file-notify-test02-rm-watch'
  2017-05-01 10:21           ` Michael Albinus
@ 2017-05-01 18:28             ` Ken Brown
  2017-05-01 18:45               ` Michael Albinus
  0 siblings, 1 reply; 11+ messages in thread
From: Ken Brown @ 2017-05-01 18:28 UTC (permalink / raw)
  To: Michael Albinus; +Cc: Andreas Politz, emacs-devel

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




^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Emacs-diffs] master 716b468: Extend `file-notify-test02-rm-watch'
  2017-05-01 18:28             ` Ken Brown
@ 2017-05-01 18:45               ` Michael Albinus
  2017-05-08 13:28                 ` Ken Brown
  0 siblings, 1 reply; 11+ messages in thread
From: Michael Albinus @ 2017-05-01 18:45 UTC (permalink / raw)
  To: Ken Brown; +Cc: Andreas Politz, emacs-devel

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.



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Emacs-diffs] master 716b468: Extend `file-notify-test02-rm-watch'
  2017-05-01 18:45               ` Michael Albinus
@ 2017-05-08 13:28                 ` Ken Brown
  2017-05-08 15:30                   ` Michael Albinus
  0 siblings, 1 reply; 11+ messages in thread
From: Ken Brown @ 2017-05-08 13:28 UTC (permalink / raw)
  To: Michael Albinus; +Cc: Andreas Politz, emacs-devel

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



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Emacs-diffs] master 716b468: Extend `file-notify-test02-rm-watch'
  2017-05-08 13:28                 ` Ken Brown
@ 2017-05-08 15:30                   ` Michael Albinus
  0 siblings, 0 replies; 11+ messages in thread
From: Michael Albinus @ 2017-05-08 15:30 UTC (permalink / raw)
  To: Ken Brown; +Cc: Andreas Politz, emacs-devel

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.



^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2017-05-08 15:30 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20170322131610.22658.57119@vcs0.savannah.gnu.org>
     [not found] ` <20170322131611.5FF7D20E17@vcs0.savannah.gnu.org>
2017-04-04 22:36   ` [Emacs-diffs] master 716b468: Extend `file-notify-test02-rm-watch' Ken Brown
2017-04-05  6:58     ` Michael Albinus
2017-04-05 19:24       ` Ken Brown
2017-04-05 21:02     ` Andreas Politz
2017-04-06  2:59       ` Ken Brown
2017-04-07 14:56         ` Ken Brown
2017-05-01 10:21           ` Michael Albinus
2017-05-01 18:28             ` Ken Brown
2017-05-01 18:45               ` Michael Albinus
2017-05-08 13:28                 ` Ken Brown
2017-05-08 15:30                   ` Michael Albinus

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).