[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#25539: 26.0.50; filenotify-tests.el fails on Windows (32 and 64 bits
From: |
Eli Zaretskii |
Subject: |
bug#25539: 26.0.50; filenotify-tests.el fails on Windows (32 and 64 bits) |
Date: |
Fri, 27 Jan 2017 10:31:38 +0200 |
> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Juanma Barranquero <lekktu@gmail.com>, 25539@debbugs.gnu.org
> Date: Fri, 27 Jan 2017 08:54:42 +0100
>
> > The root cause is that file-notify--test-cleanup-p expects the
> > notification descriptor(s) to be deleted from the hash table
> > maintained internally by filenotify.el, when the above happens. But
> > that doesn't work on Windows, where deleting the parent directory
> > simply causes an error whose result is that the thread which watches
> > the filesystem changes exits abnormally, but the event is not
> > reported. So in those cases the descriptor is not removed from the
> > hash table.
> >
> > The changes below make the tests succeed, but maybe the above means we
> > need to augment the w32notify implementation to clean up better in
> > this case. Michael?
>
> Yes, catching the error somehow in w32notify (don't know how) and fire
> up a `stopped' event is preferrable. I let it to you, whether this could
> be implemented.
I'll leave this bug open, although the test suite now passes. I think
I see a way of generating 'stopped', but I'll need to test that (and
it will need more changes in filenotify.el and in the tests).
> If this is not possible, maybe we shall call
> `file-notify--rm-descriptor' in `file-notify-valid-p', when we detect
> that the descriptor is not valid anymore, but it still exists.
I don't see how this is possible, as file-notify-valid-p doesn't know
which descriptors are supposed to be removed. And it will paper over
the real problem, so I'd prefer not to do that.
> > @@ -850,6 +850,8 @@ file-notify--test-with-events
> > ;; After deleting the parent directory, the descriptor must
> > ;; not be valid anymore.
> > (should-not (file-notify-valid-p file-notify--test-desc))
> > + (if (eq system-type 'windows-nt)
> > + (file-notify--rm-descriptor file-notify--test-desc))
>
> It would be OK for me also to apply this patch. The test shall be
>
> (if (string-equal (file-notify--test-library) "w32notify")
>
> Otherwise, we would catch also the remote case when running the test on
> MS Windows. And a respective comment might serve why we apply this.
Done, thanks.