bug#25539: 26.0.50; filenotify-tests.el fails on Windows (32 and 64 bits)

bug#25539: 26.0.50; filenotify-tests.el fails on Windows (32 and 64 bits)

[Juanma Barranquero]

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

trunk@C:\...\test> mmake -s lisp/filenotify-tests
Testing lisp/filenotify-tests.el
Running 20 tests (2017-01-26 00:36:36+0100)
Library: `w32notify'
   passed   1/20  file-notify-test00-availability
  skipped   2/20  file-notify-test00-availability-remote
   passed   3/20  file-notify-test01-add-watch
  skipped   4/20  file-notify-test01-add-watch-remote
   passed   5/20  file-notify-test02-events
  skipped   6/20  file-notify-test02-events-remote
Reverting buffer `file-notify-test5408q2D'.
Reverting buffer `file-notify-test5408q2D'.
   passed   7/20  file-notify-test03-autorevert
  skipped   8/20  file-notify-test03-autorevert-remote
Test file-notify-test04-file-validity backtrace:
  (if (unwind-protect (setq value-4 (apply fn-2 args-3)) (setq form-de
  (let (form-description-6) (if (unwind-protect (setq value-4 (apply f
  (let ((value-4 (quote ert-form-evaluation-aborted-5))) (let (form-de
  (let ((fn-2 (function file-notify--test-no-descriptors)) (args-3 (li
  file-notify--test-cleanup-p()
  (let ((temporary-file-directory (make-temp-file "file-notify-test-pa
  (unwind-protect (let ((temporary-file-directory (make-temp-file "fil
  (closure (t) nil (let ((fn-194 (function file-notify--test-local-ena
  ert--run-test-internal([cl-struct-ert--test-execution-info [cl-struc
  ert-run-test([cl-struct-ert-test file-notify-test04-file-validity "C
  ert-run-or-rerun-test([cl-struct-ert--stats t [[cl-struct-ert-test f
  ert-run-tests(t #[385 "\306 \307\"\203G \211\211G\310U\203 \211@\20
  ert-run-tests-batch(nil)
  ert-run-tests-batch-and-exit(nil)
  eval((ert-run-tests-batch-and-exit nil))
  command-line-1(("-L" ";." "-l" "ert" "-l" "lisp/filenotify-tests.el"
  command-line()
  normal-top-level()
Test file-notify-test04-file-validity condition:
    (ert-test-failed
     ((should
       (file-notify--test-no-descriptors))
      :form
      (file-notify--test-no-descriptors)
      :value nil :explanation
      ("Watch descriptor(s) existent:"
       (38542300
"c:/Users/Juanma/AppData/Local/Temp/file-notify-test-parent5408RVW"
                 (nil . file-notify--test-event-handler)))))
   FAILED   9/20  file-notify-test04-file-validity
  skipped  10/20  file-notify-test04-file-validity-remote
Test file-notify-test05-dir-validity backtrace:
  (if (unwind-protect (setq value-4 (apply fn-2 args-3)) (setq form-de
  (let (form-description-6) (if (unwind-protect (setq value-4 (apply f
  (let ((value-4 (quote ert-form-evaluation-aborted-5))) (let (form-de
  (let ((fn-2 (function file-notify--test-no-descriptors)) (args-3 (li
  file-notify--test-cleanup-p()
  (progn (let ((value-269 (cl-gensym "ert-form-evaluation-aborted-")))
  (unwind-protect (progn (let ((value-269 (cl-gensym "ert-form-evaluat
  (closure (t) nil (let ((fn-250 (function file-notify--test-local-ena
  ert--run-test-internal([cl-struct-ert--test-execution-info [cl-struc
  ert-run-test([cl-struct-ert-test file-notify-test05-dir-validity "Ch
  ert-run-or-rerun-test([cl-struct-ert--stats t [[cl-struct-ert-test f
  ert-run-tests(t #[385 "\306 \307\"\203G \211\211G\310U\203 \211@\20
  ert-run-tests-batch(nil)
  ert-run-tests-batch-and-exit(nil)
  eval((ert-run-tests-batch-and-exit nil))
  command-line-1(("-L" ";." "-l" "ert" "-l" "lisp/filenotify-tests.el"
  command-line()
  normal-top-level()
Test file-notify-test05-dir-validity condition:
    (ert-test-failed
     ((should
       (file-notify--test-no-descriptors))
      :form
      (file-notify--test-no-descriptors)
      :value nil :explanation
      ("Watch descriptor(s) existent:"
       (38542240
"c:/Users/Juanma/AppData/Local/Temp/file-notify-test-parent54084zo"
                 (nil . ignore)))))
   FAILED  11/20  file-notify-test05-dir-validity
  skipped  12/20  file-notify-test05-dir-validity-remote
Test file-notify-test06-many-events backtrace:
  (if (unwind-protect (setq value-4 (apply fn-2 args-3)) (setq form-de
  (let (form-description-6) (if (unwind-protect (setq value-4 (apply f
  (let ((value-4 (quote ert-form-evaluation-aborted-5))) (let (form-de
  (let ((fn-2 (function file-notify--test-no-descriptors)) (args-3 (li
  file-notify--test-cleanup-p()
  (let ((n 1000) source-file-list target-file-list (default-directory
  (unwind-protect (let ((n 1000) source-file-list target-file-list (de
  (closure (t) nil (let ((fn-288 (function file-notify--test-local-ena
  ert--run-test-internal([cl-struct-ert--test-execution-info [cl-struc
  ert-run-test([cl-struct-ert-test file-notify-test06-many-events "Che
  ert-run-or-rerun-test([cl-struct-ert--stats t [[cl-struct-ert-test f
  ert-run-tests(t #[385 "\306 \307\"\203G \211\211G\310U\203 \211@\20
  ert-run-tests-batch(nil)
  ert-run-tests-batch-and-exit(nil)
  eval((ert-run-tests-batch-and-exit nil))
  command-line-1(("-L" ";." "-l" "ert" "-l" "lisp/filenotify-tests.el"
  command-line()
  normal-top-level()
Test file-notify-test06-many-events condition:
    (ert-test-failed
     ((should
       (file-notify--test-no-descriptors))
      :form
      (file-notify--test-no-descriptors)
      :value nil :explanation
      ("Watch descriptor(s) existent:"
       (38542336
"c:/Users/Juanma/AppData/Local/Temp/file-notify-test-parent5408F-u"
                 (nil . file-notify--test-event-handler)))))
   FAILED  13/20  file-notify-test06-many-events
  skipped  14/20  file-notify-test06-many-events-remote
   passed  15/20  file-notify-test07-backup
  skipped  16/20  file-notify-test07-backup-remote
Test file-notify-test08-watched-file-in-watched-dir backtrace:
  (if (unwind-protect (setq value-4 (apply fn-2 args-3)) (setq form-de
  (let (form-description-6) (if (unwind-protect (setq value-4 (apply f
  (let ((value-4 (quote ert-form-evaluation-aborted-5))) (let (form-de
  (let ((fn-2 (function file-notify--test-no-descriptors)) (args-3 (li
  file-notify--test-cleanup-p()
  (progn (let ((value-370 (cl-gensym "ert-form-evaluation-aborted-")))
  (let* ((--cl-dir-callback-- (function (lambda (event) (let ((file-no
  (unwind-protect (let* ((--cl-dir-callback-- (function (lambda (event
  (closure (t) nil (let ((fn-361 (function file-notify--test-local-ena
  ert--run-test-internal([cl-struct-ert--test-execution-info [cl-struc
  ert-run-test([cl-struct-ert-test file-notify-test08-watched-file-in-
  ert-run-or-rerun-test([cl-struct-ert--stats t [[cl-struct-ert-test f
  ert-run-tests(t #[385 "\306 \307\"\203G \211\211G\310U\203 \211@\20
  ert-run-tests-batch(nil)
  ert-run-tests-batch-and-exit(nil)
  eval((ert-run-tests-batch-and-exit nil))
  command-line-1(("-L" ";." "-l" "ert" "-l" "lisp/filenotify-tests.el"
  command-line()
  normal-top-level()
Test file-notify-test08-watched-file-in-watched-dir condition:
    (ert-test-failed
     ((should
       (file-notify--test-no-descriptors))
      :form
      (file-notify--test-no-descriptors)
      :value nil :explanation
      ("Watch descriptor(s) existent:"
       (38542360
"c:/Users/Juanma/AppData/Local/Temp/file-notify-test-parent5408RcK"
                 (nil closure ... ... ...)))))
   FAILED  17/20  file-notify-test08-watched-file-in-watched-dir
  skipped  18/20  file-notify-test08-watched-file-in-watched-dir-remote
  skipped  19/20  file-notify-test09-sufficient-resources
  skipped  20/20  file-notify-test09-sufficient-resources-remote

Ran 20 tests, 5 results as expected, 4 unexpected, 11 skipped (2017-01-26
00:38:10+0100)

4 unexpected results:
   FAILED  file-notify-test04-file-validity
   FAILED  file-notify-test05-dir-validity
   FAILED  file-notify-test06-many-events
   FAILED  file-notify-test08-watched-file-in-watched-dir

11 skipped results:
  SKIPPED  file-notify-test00-availability-remote
  SKIPPED  file-notify-test01-add-watch-remote
  SKIPPED  file-notify-test02-events-remote
  SKIPPED  file-notify-test03-autorevert-remote
  SKIPPED  file-notify-test04-file-validity-remote
  SKIPPED  file-notify-test05-dir-validity-remote
  SKIPPED  file-notify-test06-many-events-remote
  SKIPPED  file-notify-test07-backup-remote
  SKIPPED  file-notify-test08-watched-file-in-watched-dir-remote
  SKIPPED  file-notify-test09-sufficient-resources
  SKIPPED  file-notify-test09-sufficient-resources-remote

Makefile:115: recipe for target `lisp/filenotify-tests.log' failed
make[1]: *** [lisp/filenotify-tests.log] Error 1
Makefile:152: recipe for target `lisp/filenotify-tests' failed
make: *** [lisp/filenotify-tests] Error 2

[-- Attachment #2: Type: text/html, Size: 10874 bytes --]

bug#25539: 26.0.50; filenotify-tests.el fails on Windows (32 and 64 bits)

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Thu, 26 Jan 2017 01:03:09 +0100
> 
> 4 unexpected results:
> FAILED file-notify-test04-file-validity
> FAILED file-notify-test05-dir-validity
> FAILED file-notify-test06-many-events
> FAILED file-notify-test08-watched-file-in-watched-dir

The problem is in file-notify--test-cleanup-p, and it happens only
when the parent directory of the file(s) being watched is deleted.

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?

diff --git a/test/lisp/filenotify-tests.el b/test/lisp/filenotify-tests.el
index d237d0c..5d31251 100644
--- a/test/lisp/filenotify-tests.el
+++ b/test/lisp/filenotify-tests.el
@@ -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))
 
         ;; The environment shall be cleaned up.
         (file-notify--test-cleanup-p))
@@ -906,6 +908,8 @@ file-notify--test-with-events
 	 (file-notify--test-timeout)
 	 (not (file-notify-valid-p file-notify--test-desc)))
         (should-not (file-notify-valid-p file-notify--test-desc))
+        (if (eq system-type 'windows-nt)
+            (file-notify--rm-descriptor file-notify--test-desc))
 
         ;; The environment shall be cleaned up.
         (file-notify--test-cleanup-p))
@@ -975,6 +979,8 @@ file-notify--test-with-events
             (file-notify--test-read-event)
             (delete-file file)))
         (delete-directory file-notify--test-tmpfile)
+        (if (eq system-type 'windows-nt)
+            (file-notify--rm-descriptor file-notify--test-desc))
 
         ;; The environment shall be cleaned up.
         (file-notify--test-cleanup-p))
@@ -1184,6 +1190,9 @@ file-notify--test-with-events
           (delete-directory file-notify--test-tmpfile 'recursive))
         (should-not (file-notify-valid-p file-notify--test-desc1))
         (should-not (file-notify-valid-p file-notify--test-desc2))
+        (when (eq system-type 'windows-nt)
+          (file-notify--rm-descriptor file-notify--test-desc1)
+          (file-notify--rm-descriptor file-notify--test-desc2))
 
         ;; The environment shall be cleaned up.
         (file-notify--test-cleanup-p))

bug#25539: 26.0.50; filenotify-tests.el fails on Windows (32 and 64 bits)

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

>> 4 unexpected results:
>> FAILED file-notify-test04-file-validity
>> FAILED file-notify-test05-dir-validity
>> FAILED file-notify-test06-many-events
>> FAILED file-notify-test08-watched-file-in-watched-dir
>
> The problem is in file-notify--test-cleanup-p, and it happens only
> when the parent directory of the file(s) being watched is deleted.
>
> 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.

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.

> @@ -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.

Best regards, Michael.

bug#25539: 26.0.50; filenotify-tests.el fails on Windows (32 and 64 bits)

[Eli Zaretskii]

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

bug#25539: 26.0.50; filenotify-tests.el fails on Windows (32 and 64 bits)

[Juanma Barranquero]

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

On Fri, Jan 27, 2017 at 9:32 AM Eli Zaretskii <eliz@gnu.org> wrote:

> Done, thanks.

This was marked as Done in 26.1 and it doesn't happen on 27.0.50, so I'm
closing it.

[-- Attachment #2: Type: text/html, Size: 304 bytes --]