Re: master 262d0c6: Mark some tests as expensive

[Stefan Kangas <stefan@marxist.se>]

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

Hi Michael,

Michael Albinus <michael.albinus@gmx.de> writes:

>> It does seem unfortunate that no tests run for autorevert-mode in the
>> default case.  I have a patch in the works that will reduce the running
>> time for these tests, I will send it here when it's done.  (Basically
>> the patch would temporarily lower auto-revert-interval, remove sleeps
>> and manually set mtime to lower values than the current time.)
>
> Would be helpful. However, there have been discussions (and changes) of
> the timeouts in the past, so I'm not sure we can reduce them w/o causing
> damage. Please check the git history of autorevert-tests.el for such
> changes, hopefully there are bug numbers in the git log you could consult
> for the respective discussions. Otherwise, a search on the emacs-devel
> and emacs-bugs MLs for the given timeframe of such a change might be helpful.

So I have attached here a patch that employs the above mentioned
techniques somewhat aggressively to reduce the running time as follows:

Ran 7 tests, 7 results as expected, 0 unexpected (2020-10-18
19:38:29+0200, 2.323729 sec)

Everything works fine here, but I don't know if any of this would cause
any problems elsewhere.  One idea is to just push it and see what
breaks, and then adapt accordingly.

Please let me know what you think.

(Note that the patch takes care to not re-indent any of the tests in
order to be easier to review.)

---

Also, I've had some trouble finding the past discussions about this.
I've been looking for "auto-revert-tests", "autorevert-tests",
"autorevert-tests.el" and "auto-revert-interval" in both emacs-devel and
bug-gnu-emacs.  The only bugs I was able to dig up was:

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=21668
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=32645
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=35418

But I'm not sure if any of that is even relevant here.  Let me know if
you have any ideas for what I could look for.

Best regards,
Stefan Kangas

[-- Attachment #2: 0001-Make-auto-revert-mode-tests-run-faster.patch --]
[-- Type: text/x-diff, Size: 18426 bytes --]

From 877dfbfc1381e1865f36ae9c15b3a9e99b6c5699 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
Date: Sat, 12 Sep 2020 19:49:20 +0200
Subject: [PATCH] Make auto-revert-mode tests run faster

* test/lisp/autorevert-tests.el (auto-revert--timeout): Make into
defun and shorten timeout by a factor 10.
(auto-revert--wait-for-revert): Cut timeouts in half.
(with-auto-revert-test): New macro to set timeout to 0.1.
(auto-revert-tests--write-file): New defun.
(auto-revert-test00-auto-revert-mode)
(auto-revert-test01-auto-revert-several-files)
(auto-revert-test02-auto-revert-deleted-file)
(auto-revert-test03-auto-revert-tail-mode)
(auto-revert-test04-auto-revert-mode-dired)
(auto-revert-test05-global-notify)
(auto-revert-test06-write-file): Adapt test to run faster.
---
 test/lisp/autorevert-tests.el | 124 +++++++++++++++++-----------------
 1 file changed, 62 insertions(+), 62 deletions(-)

diff --git a/test/lisp/autorevert-tests.el b/test/lisp/autorevert-tests.el
index 3243a80e52..a90fa69f1b 100644
--- a/test/lisp/autorevert-tests.el
+++ b/test/lisp/autorevert-tests.el
@@ -61,8 +61,9 @@
       file-notify-debug nil
       tramp-verbose 0)
 
-(defconst auto-revert--timeout (1+ auto-revert-interval)
-  "Time to wait for a message.")
+(defun auto-revert--timeout ()
+  "Time to wait for a message."
+  (+ auto-revert-interval 0.1))
 
 (defvar auto-revert--messages nil
   "Used to collect messages issued during a section of a test.")
@@ -125,14 +126,14 @@ auto-revert--wait-for-revert
   ;; Remote files do not cooperate well with timers.  So we count ourselves.
   (let ((ct (current-time)))
     (while (and (< (float-time (time-subtract (current-time) ct))
-                   auto-revert--timeout)
+                   (auto-revert--timeout))
                 (null (string-match
                        (format-message
                         "Reverting buffer `%s'\\." (buffer-name buffer))
                        auto-revert--messages)))
       (if (with-current-buffer buffer auto-revert-use-notify)
-          (read-event nil nil 0.1)
-        (sleep-for 0.1)))))
+          (read-event nil nil 0.05)
+        (sleep-for 0.05)))))
 
 (defmacro auto-revert--deftest-remote (test docstring)
   "Define ert `TEST-remote' for remote files."
@@ -152,16 +153,29 @@ auto-revert--deftest-remote
            (funcall (ert-test-body ert-test))
          (error (message "%s" err) (signal (car err) (cdr err)))))))
 
+(defmacro with-auto-revert-test (&rest body)
+  `(let ((auto-revert-interval-orig auto-revert-interval))
+     (unwind-protect
+         (progn
+           (customize-set-variable 'auto-revert-interval 0.1)
+           ,@body)
+       (customize-set-variable 'auto-revert-interval auto-revert-interval-orig))))
+
+(defun auto-revert-tests--write-file (text file time-delta &optional append)
+  (write-region text nil file append 'no-message)
+  (set-file-times file (time-subtract (current-time) time-delta)))
+
 (ert-deftest auto-revert-test00-auto-revert-mode ()
   "Check autorevert for a file."
   ;; `auto-revert-buffers' runs every 5".  And we must wait, until the
   ;; file has been reverted.
-  :tags '(:expensive-test)
+  (with-auto-revert-test
   (let ((tmpfile (make-temp-file "auto-revert-test"))
+        (times '(60 30 15))
         buf)
     (unwind-protect
-	(progn
-          (write-region "any text" nil tmpfile nil 'no-message)
+        (progn
+          (auto-revert-tests--write-file "any text" tmpfile (pop times))
 	  (setq buf (find-file-noselect tmpfile))
           (with-current-buffer buf
             (ert-with-message-capture auto-revert--messages
@@ -169,14 +183,12 @@ auto-revert-test00-auto-revert-mode
               ;; `buffer-stale--default-function' checks for
               ;; `verify-visited-file-modtime'.  We must ensure that it
               ;; returns nil.
-              (sleep-for 1)
               (auto-revert-mode 1)
               (should auto-revert-mode)
 
-              ;; Modify file.  We wait for a second, in order to have
+              ;;  order to have
               ;; another timestamp.
-              (sleep-for 1)
-              (write-region "another text" nil tmpfile nil 'no-message)
+              (auto-revert-tests--write-file "another text" tmpfile (pop times))
 
               ;; Check, that the buffer has been reverted.
               (auto-revert--wait-for-revert buf))
@@ -185,8 +197,7 @@ auto-revert-test00-auto-revert-mode
             ;; When the buffer is modified, it shall not be reverted.
             (ert-with-message-capture auto-revert--messages
               (set-buffer-modified-p t)
-              (sleep-for 1)
-              (write-region "any text" nil tmpfile nil 'no-message)
+              (auto-revert-tests--write-file "any text" tmpfile (pop times))
 
               ;; Check, that the buffer hasn't been reverted.
               (auto-revert--wait-for-revert buf))
@@ -196,7 +207,7 @@ auto-revert-test00-auto-revert-mode
       (ignore-errors
         (with-current-buffer buf (set-buffer-modified-p nil))
         (kill-buffer buf))
-      (ignore-errors (delete-file tmpfile)))))
+      (ignore-errors (delete-file tmpfile))))))
 
 (auto-revert--deftest-remote auto-revert-test00-auto-revert-mode
   "Check autorevert for a remote file.")
@@ -204,9 +215,9 @@ auto-revert-test00-auto-revert-mode
 ;; This is inspired by Bug#21841.
 (ert-deftest auto-revert-test01-auto-revert-several-files ()
   "Check autorevert for several files at once."
-  :tags '(:expensive-test)
   (skip-unless (executable-find "cp" (file-remote-p temporary-file-directory)))
 
+  (with-auto-revert-test
   (let* ((cp (executable-find "cp" (file-remote-p temporary-file-directory)))
          (tmpdir1 (make-temp-file "auto-revert-test" 'dir))
          (tmpdir2 (make-temp-file "auto-revert-test" 'dir))
@@ -214,12 +225,13 @@ auto-revert-test01-auto-revert-several-files
           (make-temp-file (expand-file-name "auto-revert-test" tmpdir1)))
          (tmpfile2
           (make-temp-file (expand-file-name "auto-revert-test" tmpdir1)))
+         (times '(120 60 30 15))
          buf1 buf2)
     (unwind-protect
         (ert-with-message-capture auto-revert--messages
-          (write-region "any text" nil tmpfile1 nil 'no-message)
+          (auto-revert-tests--write-file "any text" tmpfile1 (pop times))
           (setq buf1 (find-file-noselect tmpfile1))
-          (write-region "any text" nil tmpfile2 nil 'no-message)
+          (auto-revert-tests--write-file "any text" tmpfile2 (pop times))
           (setq buf2 (find-file-noselect tmpfile2))
 
           (dolist (buf (list buf1 buf2))
@@ -228,21 +240,19 @@ auto-revert-test01-auto-revert-several-files
               ;; `buffer-stale--default-function' checks for
               ;; `verify-visited-file-modtime'.  We must ensure that
               ;; it returns nil.
-              (sleep-for 1)
               (auto-revert-mode 1)
               (should auto-revert-mode)))
 
           ;; Modify files.  We wait for a second, in order to have
           ;; another timestamp.
-          (sleep-for 1)
-          (write-region
-           "another text" nil
+          (auto-revert-tests--write-file
+           "another text"
            (expand-file-name (file-name-nondirectory tmpfile1) tmpdir2)
-           nil 'no-message)
-          (write-region
-           "another text" nil
+           (pop times))
+          (auto-revert-tests--write-file
+           "another text"
            (expand-file-name (file-name-nondirectory tmpfile2) tmpdir2)
-           nil 'no-message)
+           (pop times))
           ;;(copy-directory tmpdir2 tmpdir1 nil 'copy-contents)
           ;; Strange, that `copy-directory' does not work as expected.
           ;; The following shell command is not portable on all
@@ -263,7 +273,7 @@ auto-revert-test01-auto-revert-several-files
           (with-current-buffer buf (set-buffer-modified-p nil))
           (kill-buffer buf)))
       (ignore-errors (delete-directory tmpdir1 'recursive))
-      (ignore-errors (delete-directory tmpdir2 'recursive)))))
+      (ignore-errors (delete-directory tmpdir2 'recursive))))))
 
 (auto-revert--deftest-remote auto-revert-test01-auto-revert-several-files
   "Check autorevert for several remote files at once.")
@@ -271,19 +281,20 @@ auto-revert-test01-auto-revert-several-files
 ;; This is inspired by Bug#23276.
 (ert-deftest auto-revert-test02-auto-revert-deleted-file ()
   "Check autorevert for a deleted file."
-  :tags '(:expensive-test)
   ;; Repeated unpredictable failures, bug#32645.
   ;; Unlikely to be hydra-specific?
 ;  (skip-unless (not (getenv "EMACS_HYDRA_CI")))
 
+  (with-auto-revert-test
   (let ((tmpfile (make-temp-file "auto-revert-test"))
         ;; Try to catch bug#32645.
         (auto-revert-debug (getenv "EMACS_HYDRA_CI"))
         (file-notify-debug (getenv "EMACS_HYDRA_CI"))
+        (times '(120 60 30 15))
         buf desc)
     (unwind-protect
 	(progn
-          (write-region "any text" nil tmpfile nil 'no-message)
+          (auto-revert-tests--write-file "any text" tmpfile (pop times))
 	  (setq buf (find-file-noselect tmpfile))
 	  (with-current-buffer buf
             (should-not
@@ -292,7 +303,6 @@ auto-revert-test02-auto-revert-deleted-file
             ;; `buffer-stale--default-function' checks for
             ;; `verify-visited-file-modtime'.  We must ensure that
             ;; it returns nil.
-            (sleep-for 1)
             (auto-revert-mode 1)
             (should auto-revert-mode)
             (setq desc auto-revert-notify-watch-descriptor)
@@ -308,8 +318,7 @@ auto-revert-test02-auto-revert-deleted-file
              nil t)
 
             (ert-with-message-capture auto-revert--messages
-              (sleep-for 1)
-              (write-region "another text" nil tmpfile nil 'no-message)
+              (auto-revert-tests--write-file "another text" tmpfile (pop times))
               (auto-revert--wait-for-revert buf))
             ;; Check, that the buffer hasn't been reverted.  File
             ;; notification should be disabled, falling back to
@@ -325,8 +334,7 @@ auto-revert-test02-auto-revert-deleted-file
             ;; reverted.
             (kill-local-variable 'before-revert-hook)
             (ert-with-message-capture auto-revert--messages
-              (sleep-for 1)
-              (write-region "another text" nil tmpfile nil 'no-message)
+              (auto-revert-tests--write-file "another text" tmpfile (pop times))
               (auto-revert--wait-for-revert buf))
             ;; Check, that the buffer has been reverted.
             (should (string-match "another text" (buffer-string)))
@@ -338,8 +346,7 @@ auto-revert-test02-auto-revert-deleted-file
 
             ;; An empty file shall still be reverted.
             (ert-with-message-capture auto-revert--messages
-              (sleep-for 1)
-              (write-region "" nil tmpfile nil 'no-message)
+              (auto-revert-tests--write-file "" tmpfile (pop times))
               (auto-revert--wait-for-revert buf))
             ;; Check, that the buffer has been reverted.
             (should (string-equal "" (buffer-string)))))
@@ -348,7 +355,7 @@ auto-revert-test02-auto-revert-deleted-file
       (ignore-errors
         (with-current-buffer buf (set-buffer-modified-p nil))
         (kill-buffer buf))
-      (ignore-errors (delete-file tmpfile)))))
+      (ignore-errors (delete-file tmpfile))))))
 
 (auto-revert--deftest-remote auto-revert-test02-auto-revert-deleted-file
   "Check autorevert for a deleted remote file.")
@@ -357,28 +364,25 @@ auto-revert-test03-auto-revert-tail-mode
   "Check autorevert tail mode."
   ;; `auto-revert-buffers' runs every 5".  And we must wait, until the
   ;; file has been reverted.
-  :tags '(:expensive-test)
   (let ((tmpfile (make-temp-file "auto-revert-test"))
+        (times '(30 15))
         buf)
     (unwind-protect
         (ert-with-message-capture auto-revert--messages
-          (write-region "any text" nil tmpfile nil 'no-message)
+          (auto-revert-tests--write-file "any text" tmpfile (pop times))
 	  (setq buf (find-file-noselect tmpfile))
 	  (with-current-buffer buf
             ;; `buffer-stale--default-function' checks for
             ;; `verify-visited-file-modtime'.  We must ensure that it
             ;; returns nil.
-            (sleep-for 1)
-	    (auto-revert-tail-mode 1)
+            (auto-revert-tail-mode 1)
 	    (should auto-revert-tail-mode)
             (erase-buffer)
             (insert "modified text\n")
             (set-buffer-modified-p nil)
 
-	    ;; Modify file.  We wait for a second, in order to have
-	    ;; another timestamp.
-	    (sleep-for 1)
-            (write-region "another text" nil tmpfile 'append 'no-message)
+            ;; Modify file.
+            (auto-revert-tests--write-file "another text" tmpfile (pop times) 'append)
 
 	    ;; Check, that the buffer has been reverted.
             (auto-revert--wait-for-revert buf)
@@ -396,9 +400,10 @@ auto-revert-test04-auto-revert-mode-dired
   "Check autorevert for dired."
   ;; `auto-revert-buffers' runs every 5".  And we must wait, until the
   ;; file has been reverted.
-  :tags '(:expensive-test)
+  (with-auto-revert-test
   (let* ((tmpfile (make-temp-file "auto-revert-test"))
          (name (file-name-nondirectory tmpfile))
+         (times '(30))
          buf)
     (unwind-protect
 	(progn
@@ -407,16 +412,13 @@ auto-revert-test04-auto-revert-mode-dired
             ;; `buffer-stale--default-function' checks for
             ;; `verify-visited-file-modtime'.  We must ensure that it
             ;; returns nil.
-            (sleep-for 1)
             (auto-revert-mode 1)
             (should auto-revert-mode)
 	    (should
              (string-match name (substring-no-properties (buffer-string))))
 
             (ert-with-message-capture auto-revert--messages
-              ;; Delete file.  We wait for a second, in order to have
-              ;; another timestamp.
-              (sleep-for 1)
+              ;; Delete file.
               (delete-file tmpfile)
               (auto-revert--wait-for-revert buf))
             ;; Check, that the buffer has been reverted.
@@ -427,8 +429,7 @@ auto-revert-test04-auto-revert-mode-dired
               ;; Make dired buffer modified.  Check, that the buffer has
               ;; been still reverted.
               (set-buffer-modified-p t)
-              (sleep-for 1)
-              (write-region "any text" nil tmpfile nil 'no-message)
+              (auto-revert-tests--write-file "any text" tmpfile (pop times))
 
               (auto-revert--wait-for-revert buf))
             ;; Check, that the buffer has been reverted.
@@ -439,7 +440,7 @@ auto-revert-test04-auto-revert-mode-dired
       (ignore-errors
         (with-current-buffer buf (set-buffer-modified-p nil))
         (kill-buffer buf))
-      (ignore-errors (delete-file tmpfile)))))
+      (ignore-errors (delete-file tmpfile))))))
 
 (auto-revert--deftest-remote auto-revert-test04-auto-revert-mode-dired
   "Check remote autorevert for dired.")
@@ -468,9 +469,9 @@ auto-revert-test--wait-for-buffer-text
 
 (ert-deftest auto-revert-test05-global-notify ()
   "Test `global-auto-revert-mode' without polling."
-  :tags '(:expensive-test)
   (skip-unless (or file-notify--library
                    (file-remote-p temporary-file-directory)))
+  (with-auto-revert-test
   (let* ((auto-revert-use-notify t)
          (auto-revert-avoid-polling t)
          (was-in-global-auto-revert-mode global-auto-revert-mode)
@@ -510,7 +511,7 @@ auto-revert-test05-global-notify
           (auto-revert-test--wait-for
            (lambda () (buffer-local-value
                        'auto-revert-notify-watch-descriptor buf-3))
-           auto-revert--timeout)
+           (auto-revert--timeout))
           (should (buffer-local-value
                    'auto-revert-notify-watch-descriptor buf-3))
           (auto-revert-test--write-file "3-a" file-3)
@@ -519,11 +520,10 @@ auto-revert-test05-global-notify
 
           ;; Delete a visited file, and re-create it with new contents.
           (delete-file file-1)
-          (sleep-for 0.5)
           (should (equal (auto-revert-test--buffer-string buf-1) "1-a"))
           (auto-revert-test--write-file "1-b" file-1)
           (auto-revert-test--wait-for-buffer-text
-           buf-1 "1-b" auto-revert--timeout)
+           buf-1 "1-b" (auto-revert--timeout))
           (should (buffer-local-value
                    'auto-revert-notify-watch-descriptor buf-1))
 
@@ -533,7 +533,7 @@ auto-revert-test05-global-notify
           (should (equal (auto-revert-test--buffer-string buf-2) "2-a"))
           (auto-revert-test--write-file "2-b" file-2b)
           (auto-revert-test--wait-for-buffer-text
-           buf-2 "2-b" auto-revert--timeout)
+           buf-2 "2-b" (auto-revert--timeout))
           (should (buffer-local-value
                    'auto-revert-notify-watch-descriptor buf-2)))
 
@@ -544,16 +544,16 @@ auto-revert-test05-global-notify
         (ignore-errors (kill-buffer buf)))
       (dolist (file (list file-1 file-2 file-2b file-3))
         (ignore-errors (delete-file file)))
-      )))
+      ))))
 
 (auto-revert--deftest-remote auto-revert-test05-global-notify
   "Test `global-auto-revert-mode' without polling for remote buffers.")
 
 (ert-deftest auto-revert-test06-write-file ()
   "Verify that notification follows `write-file' correctly."
-  :tags '(:expensive-test)
   (skip-unless (or file-notify--library
                    (file-remote-p temporary-file-directory)))
+  (with-auto-revert-test
   (let* ((auto-revert-use-notify t)
          (file-1 (make-temp-file "auto-revert-test"))
          (file-2 (concat file-1 "-2"))
@@ -572,13 +572,13 @@ auto-revert-test06-write-file
 
             (auto-revert-test--write-file "C" file-2)
             (auto-revert-test--wait-for-buffer-text
-             buf "C" auto-revert--timeout)
+             buf "C" (auto-revert--timeout))
             (should (equal (buffer-string) "C"))))
 
       ;; Clean up.
       (ignore-errors (kill-buffer buf))
       (ignore-errors (delete-file file-1))
-      (ignore-errors (delete-file file-2)))))
+      (ignore-errors (delete-file file-2))))))
 
 (auto-revert--deftest-remote auto-revert-test06-write-file
   "Test `write-file' in `auto-revert-mode' for remote buffers.")
-- 
2.28.0