bug#63550: proced-refine-with-update-test is racy

bug#63550: proced-refine-with-update-test is racy

[Mattias Engdegård]

The proced test `proced-refine-with-update-test` seems to have an update race which makes it fail randomly. (This is on macOS, but I see no reason it would be much different elsewhere.)
I'm marking it :unstable for now.

bug#63550: proced-refine-with-update-test is racy

[Basil Contovounesios via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Mattias Engdegård [2023-05-17 11:38 +0200] wrote:

> The proced test `proced-refine-with-update-test` seems to have an update race which makes it fail randomly. (This is on macOS, but I see no reason it would be much different elsewhere.)

Indeed I encounter the race from time to time on GNU/Linux as well.

> I'm marking it :unstable for now.

Thanks,

-- 
Basil

In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.16.0, Xaw3d scroll bars) of 2023-05-15 built on blc
Repository revision: ebf5e4ca1cd39d3f23c4e37d9bdfeb2bf347df6d
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101004
System Description: Ubuntu 22.04.2 LTS

Configured using:
 'configure CC=gcc-12 'CFLAGS=-Og -ggdb3' --prefix=/home/bic/.local
 --with-file-notification=yes --with-x --with-x-toolkit=lucid'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES NOTIFY
INOTIFY PDUMPER PNG RSVG SECCOMP SOUND SQLITE3 THREADS TIFF
TOOLKIT_SCROLL_BARS TREE_SITTER WEBP X11 XAW3D XDBE XIM XINPUT2 XPM
LUCID ZLIB

Important settings:
  value of $LC_MONETARY: en_IE.UTF-8
  value of $LC_NUMERIC: en_IE.UTF-8
  value of $LC_TIME: en_IE.UTF-8
  value of $LANG: en_GB.UTF-8
  value of $XMODIFIERS: @im=ibus
  locale-coding-system: utf-8-unix

bug#63550: proced-refine-with-update-test is racy

[Laurence Warne]

[-- Attachment #1.1: Type: text/plain, Size: 663 bytes --]

Hi,

> The proced test `proced-refine-with-update-test` seems to have an update
race which makes it fail randomly. (This is on macOS, but I see no reason
it would be much different elsewhere.)

Strange, I can't seem to reproduce running it continuously (I'm on linux
also).  Though whilst looking over the test suite, I don't think I like the
approach I took.  I've attached a new patch which changes the test suite to
mock the list of active processes (of course comments welcome).

Are either of you able to test whether this fixes the test in question?
The change also makes the tests a lot faster to run and hopefully less
system dependent.

Thanks, Laurence

[-- Attachment #1.2: Type: text/html, Size: 838 bytes --]

[-- Attachment #2: 0001-Mock-processes-in-proced-test.patch --]
[-- Type: text/x-patch, Size: 6138 bytes --]

From 08ff832e84470822ff25b733af07f7d750d0cde3 Mon Sep 17 00:00:00 2001
From: Laurence Warne <laurencewarne@gmail.com>
Date: Sat, 20 May 2023 20:24:45 +0100
Subject: [PATCH] Mock processes in proced-test

Mock 'list-system-processes' and 'process-attributes' for Proced tests
in order to mock a list of existing processes.

* test/lisp/proced-tests.el (proced--within-buffer): Allow a list of
processes to specified as a parameter to be used as mocks.
(proced-format-test, proced-update-test)
(proced-update-preserves-pid-at-point-test, proced-revert-test)
(proced-color-test, proced-refine-test, proced-refine-with-update-test)
(proced-update-preserves-pid-at-point-test): Adapt to use mocks.
---
 test/lisp/proced-tests.el | 101 ++++++++++++++++++++++++++++----------
 1 file changed, 76 insertions(+), 25 deletions(-)

diff --git a/test/lisp/proced-tests.el b/test/lisp/proced-tests.el
index d69414cf43a..06c427d056f 100644
--- a/test/lisp/proced-tests.el
+++ b/test/lisp/proced-tests.el
@@ -18,26 +18,69 @@
 ;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
 
 ;;; Code:
+(require 'cl-lib)
 (require 'ert)
 (require 'proced)
 (require 'thingatpt)
 
-(cl-defmacro proced--within-buffer (format filter &body body)
-  "Execute BODY within a proced buffer using format FORMAT and filter FILTER."
+(defconst proced--mock-process-attributes
+  '((args . "/sbin/init")
+    (pmem . 0.03338765170917735)
+    (pcpu . 0.008040037401062874)
+    (etime 0 20149 160000 0)
+    (rss . 10964)
+    (vsize . 164524)
+    (start 25704 42324 637481 508000)
+    (thcount . 1)
+    (nice . 0)
+    (pri . 20)
+    (ctime 0 873 550000 0)
+    (cstime 0 16 30000 0)
+    (cutime 0 857 520000 0)
+    (time 0 1 620000 0)
+    (stime 0 1 360000 0)
+    (utime 0 0 260000 0)
+    (cmajflt . 3931)
+    (cminflt . 425816)
+    (majflt . 162)
+    (minflt . 25825)
+    (tpgid . -1)
+    (ttname . "")
+    (sess . 1)
+    (pgrp . 1)
+    (ppid . 0)
+    (state . "S")
+    (comm . "systemd")
+    (group . "root")
+    (egid . 0)
+    (user . "root")
+    (euid . 0))
+  "A mocked list of process attributes.")
+
+(defconst proced--mock-pid 34123)
+
+(cl-defmacro proced--within-buffer (format filter processes &body body)
+  "Execute BODY within a proced buffer using format FORMAT and filter FILTER.
+
+Also use PROCESSES as a mocked list of processes."
   `(let ((proced-format ,format)
          (proced-filter ,filter)
          (proced-auto-update-flag nil)
          (inhibit-message t))
-     (proced)
-     (unwind-protect
-         (with-current-buffer "*Proced*"
-           ,@body)
-       (kill-buffer "*Proced*"))))
-
-(defun proced--assert-emacs-pid-in-buffer ()
-  "Fail unless the process ID of the current Emacs process exists in buffer."
+     (cl-letf (((symbol-function 'list-system-processes)
+                (lambda (&rest args) (mapcar #'car ,processes)))
+               ((symbol-function 'process-attributes)
+                (lambda (process) (alist-get process ,processes))))
+       (proced)
+       (unwind-protect
+           (with-current-buffer "*Proced*"
+             ,@body)
+         (kill-buffer "*Proced*")))))
+
+(defun proced--assert-pid-in-buffer (pid)
+  "Fail unless PID exists in the current buffer."
   (should (string-match-p
-           (number-to-string (emacs-pid))
+           (number-to-string pid)
            (buffer-substring-no-properties (point-min) (point-max)))))
 
 (defun proced--move-to-column (attribute)
@@ -48,35 +91,40 @@ proced-format-test
   (dolist (format '(short medium long verbose))
     (proced--within-buffer
      format
-     'user
-     (proced--assert-emacs-pid-in-buffer))))
+     'all
+     `((,proced--mock-pid . ,proced--mock-process-attributes))
+     (proced--assert-pid-in-buffer proced--mock-pid))))
 
 (ert-deftest proced-update-test ()
   (proced--within-buffer
    'short
-   'user
+   'all
+   `((,proced--mock-pid . ,proced--mock-process-attributes))
    (proced-update)
-   (proced--assert-emacs-pid-in-buffer)))
+   (proced--assert-pid-in-buffer proced--mock-pid)))
 
 (ert-deftest proced-revert-test ()
   (proced--within-buffer
    'short
-   'user
+   'all
+   `((,proced--mock-pid . ,proced--mock-process-attributes))
    (proced-revert)
-   (proced--assert-emacs-pid-in-buffer)))
+   (proced--assert-pid-in-buffer proced--mock-pid)))
 
 (ert-deftest proced-color-test ()
   (let ((proced-enable-color-flag t))
     (proced--within-buffer
      'short
-     'user
-     (proced--assert-emacs-pid-in-buffer))))
+     'all
+     `((,proced--mock-pid . ,proced--mock-process-attributes))
+     (proced--assert-pid-in-buffer proced--mock-pid))))
 
 (ert-deftest proced-refine-test ()
-  ;;(skip-unless (memq system-type '(gnu/linux gnu/kfreebsd darwin)))
   (proced--within-buffer
    'medium
-   'user
+   'all
+   `((,proced--mock-pid . ,proced--mock-process-attributes)
+     (,(1+ proced--mock-pid) . ,proced--mock-process-attributes))
    ;; When refining on PID for process A, a process is kept if and only
    ;; if its PID are the same as process A, which more or less guarentees
    ;; the refinement will remove some processes.
@@ -89,10 +137,11 @@ proced-refine-test
        (forward-line)))))
 
 (ert-deftest proced-refine-with-update-test ()
-  :tags '(:unstable)   ; There seems to be an update race here.
   (proced--within-buffer
    'medium
-   'user
+   'all
+   `((,proced--mock-pid . ,proced--mock-process-attributes)
+     (,(1+ proced--mock-pid) . ,proced--mock-process-attributes))
    (proced--move-to-column "PID")
    (let ((pid (word-at-point)))
      (proced-refine)
@@ -105,9 +154,11 @@ proced-refine-with-update-test
 (ert-deftest proced-update-preserves-pid-at-point-test ()
   (proced--within-buffer
    'medium
-   'user
+   'all
+   `((,proced--mock-pid . ,proced--mock-process-attributes)
+     (,(1+ proced--mock-pid) . ,proced--mock-process-attributes))
    (goto-char (point-min))
-   (search-forward (number-to-string (emacs-pid)))
+   (search-forward (number-to-string proced--mock-pid))
    (proced--move-to-column "PID")
    (save-window-excursion
      (let ((pid (proced-pid-at-point))
-- 
2.30.2

bug#63550: proced-refine-with-update-test is racy

[Eli Zaretskii]

> Cc: Mattias Engdegård <mattias.engdegard@gmail.com>,
>  63550@debbugs.gnu.org
> From: Laurence Warne <laurencewarne@gmail.com>
> Date: Sun, 21 May 2023 09:37:10 +0100
> 
> Strange, I can't seem to reproduce running it continuously (I'm on linux also).  Though whilst looking
> over the test suite, I don't think I like the approach I took.  I've attached a new patch which changes the
> test suite to mock the list of active processes (of course comments welcome).
> 
> Are either of you able to test whether this fixes the test in question?  The change also makes the tests
> a lot faster to run and hopefully less system dependent.

Is mocking out the real Proced display a good idea in this case?
These tests test the ability to manipulate real-life process-attribute
displays, so showing they work in synthetic environment verifies only
part of the functionality, no?

bug#63550: proced-refine-with-update-test is racy

[Laurence Warne]

[-- Attachment #1.1: Type: text/plain, Size: 1369 bytes --]

One thing I've noticed about the failing test is that we should probably
use `(proced-update)` instead of `(proced-update t)` so as not to refresh
`proced-process-alist` (I've attached a patch).  When I first saw this, I
thought this would fix the failure as I thought what might have been
happening was that the process used for the refinement might have exited
between proced being called, and then `(proced-update t)` being called, but
I think the test should still pass in this case (though I've optimistically
used 'fix' in the patch commit (: ).

Mattias (or Basil), are you able to provide a backtrace?

> Is mocking out the real Proced display a good idea in this case?
> These tests test the ability to manipulate real-life process-attribute
> displays, so showing they work in synthetic environment verifies only
> part of the functionality, no?

Since systems will likely have a variety of processes running at a time, I
think you are right, mocking will result in less coverage, but then again
the processes and their attributes people have running on their machines
are not consistent.  Perhaps we could use mocking, but only for features
which are difficult to test otherwise, like refinements?

Maybe I was too fast to jump to using mocking and it's tangential to this
issue, though I'd still be interested to see if the (mocking patch) fixes
the issue.

[-- Attachment #1.2: Type: text/html, Size: 1581 bytes --]

[-- Attachment #2: 0001-Fix-unstable-proced-test.patch --]
[-- Type: text/x-patch, Size: 1144 bytes --]

From 59782731876e62898ee3ee2a912c935bf6b21254 Mon Sep 17 00:00:00 2001
From: Laurence Warne <laurencewarne@gmail.com>
Date: Sun, 21 May 2023 18:59:43 +0100
Subject: [PATCH] Fix unstable proced test

Fix unstable proced test by omitting the revert parameter in
'proced-update'.

* test/lisp/proced-tests.el (proced-refine-with-update-test): Do not
use revert parameter when calling 'proced-update'.
---
 test/lisp/proced-tests.el | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/test/lisp/proced-tests.el b/test/lisp/proced-tests.el
index d69414cf43a..fa65772076f 100644
--- a/test/lisp/proced-tests.el
+++ b/test/lisp/proced-tests.el
@@ -89,14 +89,13 @@ proced-refine-test
        (forward-line)))))
 
 (ert-deftest proced-refine-with-update-test ()
-  :tags '(:unstable)   ; There seems to be an update race here.
   (proced--within-buffer
    'medium
    'user
    (proced--move-to-column "PID")
    (let ((pid (word-at-point)))
      (proced-refine)
-     (proced-update t)
+     (proced-update)
      (while (not (eobp))
        (proced--move-to-column "PID")
        (should (string= pid (word-at-point)))
-- 
2.30.2

bug#63550: proced-refine-with-update-test is racy

[Mattias Engdegård]

21 maj 2023 kl. 20.45 skrev Laurence Warne <laurencewarne@gmail.com>:

> One thing I've noticed about the failing test is that we should probably use `(proced-update)` instead of `(proced-update t)` so as not to refresh `proced-process-alist` (I've attached a patch).  When I first saw this, I thought this would fix the failure as I thought what might have been happening was that the process used for the refinement might have exited between proced being called, and then `(proced-update t)` being called, but I think the test should still pass in this case (though I've optimistically used 'fix' in the patch commit (: ).

These steps seem to provoke a failure of the original test quite reliably on macOS:

1. add the call (sleep-for 1) both before and after (proced-refine), to widen the windows
2. in a different terminal, keep the command

  while true; do /bin/sleep 2 & sleep 0.1; done

running during the test, to create some churn in the process tables. (I'm using zsh if it matters.)

Changing (proced-update t) to (proced-update) does appear to remove the failure. Maybe you can decide whether it is better or not.

bug#63550: proced-refine-with-update-test is racy

[Laurence Warne]

[-- Attachment #1.1: Type: text/plain, Size: 871 bytes --]

> These steps seem to provoke a failure of the original test quite reliably
on macOS:

> 1. add the call (sleep-for 1) both before and after (proced-refine), to
widen the windows
> 2. in a different terminal, keep the command

>  while true; do /bin/sleep 2 & sleep 0.1; done

Thanks for this, with the help of these instructions I can reproduce.

> but I think the test should still pass in this case

Sorry I was wrong here, if the process used for the refinement exited
between proced being called, and then `(proced-update t)` being called,
then the whole process list will be shown as proced would skip applying the
refinement.  So I think using (proced-update) instead of (proced-update t)
is the simplest fix since it means `proced-process-alist` will never be
refreshed.  I've re-attached the patch from before, but with a bit more
explanation.

Thanks, Laurence

[-- Attachment #1.2: Type: text/html, Size: 1163 bytes --]

[-- Attachment #2: 0001-Fix-unstable-proced-test.patch --]
[-- Type: text/x-patch, Size: 1696 bytes --]

From dcf854e3e2b78c8978f1a59d81436cdd4f531d48 Mon Sep 17 00:00:00 2001
From: Laurence Warne <laurencewarne@gmail.com>
Date: Sun, 21 May 2023 18:59:43 +0100
Subject: [PATCH] Fix unstable proced test

Fix unstable proced test by omitting the revert parameter in
'proced-update'.  This was caused by the process being refined on
exiting between the initial 'proced' call and the successive
'proced-update' call.  This resulted in proced skipping the refinement
in 'proced-update', causing all processes to be shown again and the
test to fail.

* test/lisp/proced-tests.el (proced-refine-with-update-test): Do not
use revert parameter when calling 'proced-update'.
---
 test/lisp/proced-tests.el | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/test/lisp/proced-tests.el b/test/lisp/proced-tests.el
index d69414cf43a..cc3da69a3a0 100644
--- a/test/lisp/proced-tests.el
+++ b/test/lisp/proced-tests.el
@@ -89,14 +89,17 @@ proced-refine-test
        (forward-line)))))
 
 (ert-deftest proced-refine-with-update-test ()
-  :tags '(:unstable)   ; There seems to be an update race here.
   (proced--within-buffer
    'medium
    'user
    (proced--move-to-column "PID")
    (let ((pid (word-at-point)))
      (proced-refine)
-     (proced-update t)
+     ;; Don't use (proced-update t) since this will reset `proced-process-alist'
+     ;; and it's possible the process refined on would have exited by that point.
+     ;; In this case proced will skip the refinement and show all processes again,
+     ;; causing the test to fail.
+     (proced-update)
      (while (not (eobp))
        (proced--move-to-column "PID")
        (should (string= pid (word-at-point)))
-- 
2.30.2

bug#63550: proced-refine-with-update-test is racy

[Mattias Engdegård]

27 maj 2023 kl. 21.14 skrev Laurence Warne <laurencewarne@gmail.com>:

> Sorry I was wrong here, if the process used for the refinement exited between proced being called, and then `(proced-update t)` being called, then the whole process list will be shown as proced would skip applying the refinement.  So I think using (proced-update) instead of (proced-update t) is the simplest fix since it means `proced-process-alist` will never be refreshed.  I've re-attached the patch from before, but with a bit more explanation.

Looks fine, applied and pushed and closing the bug. Thank you!
(I took the liberty to reflow the comments to fit within 80 columns.)