Subject: Re: bug#76446: [Patch Debbugs] Infrastructure Improvements

From: Morgan Smith <[email-masked]>
Date: Fri, 21 Feb 2025 13:20:41 -0500

Michael Albinus <[email-masked]> writes:

> Morgan Smith <[email-masked]> writes:
>
>> Hello!
>
> Hi Morgan,
>
>> Nothing too exciting in this series.  Just adjusting things so we can
>> split the tests into multiple files.  Also adding a makefile target to
>> run the "checkdoc" stuff with the spell checker enabled.
>
> Thanks for this! I'm happy that you's started to provide also a
> debbugs-gnu-tests.el file.
>
> I didn't test the patches yet, starting with my comments on the code
> reading first. See next messages.
>

Thanks for reviewing my patches!  I appreciate the insightful comments
you've provided.

I've attached the relevant patches to the other email threads but I'll
dump them all in this email as well in case that's easier.  You'll
notice I've also sneakily included an extra 5th patch here :).

>> Thanks,
>> Morgan
> Best regards, Michael.

>From 310038c998bfcaddebd4144d6d37da6a4d4f6aeb Mon Sep 17 00:00:00 2001
From: Morgan Smith <[email-masked]>
Date: Fri, 21 Feb 2025 12:52:03 -0500
Subject: [PATCH v2 1/5] tests: Move helper and wrapper functions into their
 own file

* test/debbugs-test-helpers.el: New file.
* test/debbugs-tests.el: Move test helper and wrapper functions into the new
file and require it.
* Makefile(check): Load all files in the test directory.
---
 Makefile                     |   5 +-
 test/debbugs-test-helpers.el | 146 +++++++++++++++++++++++++++++++++++
 test/debbugs-tests.el        | 113 +--------------------------
 3 files changed, 151 insertions(+), 113 deletions(-)
 create mode 100644 test/debbugs-test-helpers.el

diff --git a/Makefile b/Makefile
index a7b6b0c7f3..eec1c65a14 100644
--- a/Makefile
+++ b/Makefile
@@ -14,6 +14,9 @@ INFOMANUALS=debbugs.info debbugs-ug.info
 %.elc: %.el
 	@$(EMACS) -Q -batch -L . -f batch-byte-compile $<
 
+test/%.elc: test/%.el
+	@$(EMACS) -Q -batch -L . -L ./test -f batch-byte-compile $<
+
 %.info: %.texi
 	$(MAKEINFO) --error-limit=0 --no-split $< -o $@
 
@@ -24,7 +27,7 @@ doc: $(INFOMANUALS)
 build: $(TARGET)
 
 check: build $(TESTTARGET)
-	@$(EMACS) -Q --batch -L . -l $(TESTSOURCE) -f ert-run-tests-batch-and-exit
+	@$(EMACS) -Q --batch -L . -L ./test $(foreach file,$(TESTSOURCE), -l $(file)) -f ert-run-tests-batch-and-exit
 
 clean:
 	-rm -f $(TARGET) $(TESTTARGET) $(INFOMANUALS)
diff --git a/test/debbugs-test-helpers.el b/test/debbugs-test-helpers.el
new file mode 100644
index 0000000000..f98bab40ed
--- /dev/null
+++ b/test/debbugs-test-helpers.el
@@ -0,0 +1,146 @@
+;;; debbugs-test-helpers.el --- Helpers for testing debbugs -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2024-2025 Free Software Foundation, Inc.
+
+;; Author: Morgan Smith <[email-masked]>
+;; Package: debbugs
+
+;; This file is not part of GNU Emacs.
+
+;; This program is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; This program is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; Please ensure tests don't actually make network calls.
+
+;;; Code:
+
+(require 'debbugs)
+
+;;; Helper Data:
+
+;; Generated using this:
+;; (soap-invoke debbugs-wsdl debbugs-port "get_status" [64064])
+(defconst debbugs-test--bug-status-soap-return
+  '(((item
+      (key . 64064)
+      (value
+       (package . "emacs") (found_date) (last_modified . 1689593050)
+       (affects) (date . 1686745022) (fixed_versions)
+       (originator . "Morgan Smith <[email-masked]>")
+       (blocks) (archived . 1) (found) (unarchived) (tags . "patch")
+       (severity . "normal") (location . "archive") (owner) (fixed)
+       (blockedby) (pending . "done") (keywords . "patch") (id . 64064)
+       (found_versions) (mergedwith) (summary) (forwarded)
+       (log_modified . 1689593050)
+       (done . "Michael Albinus <[email-masked]>")
+       (source . "unknown")
+       (msgid
+        . "<[email-masked]>")
+       (bug_num . 64064) (subject . "[PATCH 0/4] debbugs improvements")
+       (fixed_date)))))
+  "Mock result from `soap-invoke' for bug 64064.")
+
+;; Generated using this:
+;; (debbugs-get-status 64064)
+(defconst debbugs-test--bug-status
+  '(((cache_time . 5000) (source . "unknown") (unarchived)
+     (keywords "patch") (blocks) (pending . "done") (severity . "normal")
+     (done . "Michael Albinus <[email-masked]>") (location . "archive")
+     (log_modified . 1689593050) (subject . "[PATCH 0/4] debbugs improvements")
+     (last_modified . 1689593050) (found) (tags "patch") (package "emacs")
+     (originator . "Morgan Smith <[email-masked]>") (archived . t)
+     (blockedby) (affects) (mergedwith) (summary) (date . 1686745022)
+     (fixed_versions) (id . 64064) (fixed) (found_date) (forwarded)
+     (msgid
+      . "<[email-masked]>")
+     (owner) (found_versions) (fixed_date) (bug_num . 64064)))
+  "Mock result from `debbugs-get-status' for bug 64064.")
+
+;;; Helper Functions:
+
+(defvar debbugs-test--soap-operation-name nil)
+(defvar debbugs-test--soap-parameters nil)
+(defun debbugs-test--soap-invoke-internal
+    (callback _cbargs _wsdl _service operation-name &rest parameters)
+  "Over-ride for testing."
+  (setq debbugs-test--soap-operation-name operation-name)
+  (setq debbugs-test--soap-parameters parameters)
+  (let ((return
+         (cond ((string-equal operation-name "get_status")
+                debbugs-test--bug-status-soap-return)
+               ((string-equal operation-name "get_usertag")
+                '(((hi))))
+               (t '((0))))))
+    (if callback
+        (progn
+          (funcall callback return)
+          nil)
+      return)))
+
+(defun debbugs-test--override-float-time (func &rest rest)
+  "Override `float-time' for FUNC with args REST."
+  (cl-letf (((symbol-function #'float-time)
+             (lambda (&optional _specified-time) 5000)))
+    (apply func rest)))
+
+(defun debbugs-test--setup ()
+  "Mock network and time functions.
+These mock functions are needed to make the tests reproducible."
+  (setq debbugs-test--soap-operation-name nil)
+  (setq debbugs-test--soap-parameters nil)
+
+  (add-function
+   :override (symbol-function #'soap-invoke-internal)
+   #'debbugs-test--soap-invoke-internal)
+
+  (add-function
+   :around (symbol-function #'debbugs-get-cache)
+   #'debbugs-test--override-float-time)
+
+  (add-function
+   :around (symbol-function #'debbugs-put-cache)
+   #'debbugs-test--override-float-time))
+
+(defun debbugs-test--teardown ()
+  "Restore functions to as they where before."
+  (setq debbugs-test--soap-operation-name nil)
+  (setq debbugs-test--soap-parameters nil)
+
+  (remove-function
+   (symbol-function #'soap-invoke-internal)
+   #'debbugs-test--soap-invoke-internal)
+
+  (remove-function
+   (symbol-function #'debbugs-get-cache)
+   #'debbugs-test--override-float-time)
+
+  (remove-function
+   (symbol-function #'debbugs-put-cache)
+   #'debbugs-test--override-float-time))
+
+(defmacro ert-deftest--debbugs (name args docstring &rest body)
+  "The same as `ert-deftest' but runs setup and teardown functions."
+  (declare
+   (doc-string 3)
+   (indent 2))
+  `(ert-deftest ,name ,args ,docstring
+                (debbugs-test--setup)
+                ,@body
+                (debbugs-test--teardown)))
+
+
+(provide 'debbugs-test-helpers)
+
+;;; debbugs-test-helpers.el ends here
diff --git a/test/debbugs-tests.el b/test/debbugs-tests.el
index f3abea2837..da1b9dffe6 100644
--- a/test/debbugs-tests.el
+++ b/test/debbugs-tests.el
@@ -27,118 +27,7 @@
 ;;; Code:
 
 (require 'debbugs)
-
-;;; Helper Data:
-
-;; Generated using this:
-;; (soap-invoke debbugs-wsdl debbugs-port "get_status" [64064])
-(defconst debbugs-test--bug-status-soap-return
-  '(((item
-      (key . 64064)
-      (value
-       (package . "emacs") (found_date) (last_modified . 1689593050)
-       (affects) (date . 1686745022) (fixed_versions)
-       (originator . "Morgan Smith <[email-masked]>")
-       (blocks) (archived . 1) (found) (unarchived) (tags . "patch")
-       (severity . "normal") (location . "archive") (owner) (fixed)
-       (blockedby) (pending . "done") (keywords . "patch") (id . 64064)
-       (found_versions) (mergedwith) (summary) (forwarded)
-       (log_modified . 1689593050)
-       (done . "Michael Albinus <[email-masked]>")
-       (source . "unknown")
-       (msgid
-        . "<[email-masked]>")
-       (bug_num . 64064) (subject . "[PATCH 0/4] debbugs improvements")
-       (fixed_date)))))
-  "Mock result from `soap-invoke' for bug 64064.")
-
-;; Generated using this:
-;; (debbugs-get-status 64064)
-(defconst debbugs-test--bug-status
-  '(((cache_time . 5000) (source . "unknown") (unarchived)
-     (keywords "patch") (blocks) (pending . "done") (severity . "normal")
-     (done . "Michael Albinus <[email-masked]>") (location . "archive")
-     (log_modified . 1689593050) (subject . "[PATCH 0/4] debbugs improvements")
-     (last_modified . 1689593050) (found) (tags "patch") (package "emacs")
-     (originator . "Morgan Smith <[email-masked]>") (archived . t)
-     (blockedby) (affects) (mergedwith) (summary) (date . 1686745022)
-     (fixed_versions) (id . 64064) (fixed) (found_date) (forwarded)
-     (msgid
-      . "<[email-masked]>")
-     (owner) (found_versions) (fixed_date) (bug_num . 64064)))
-  "Mock result from `debbugs-get-status' for bug 64064.")
-
-;;; Helper Functions:
-
-(defvar debbugs-test--soap-operation-name nil)
-(defvar debbugs-test--soap-parameters nil)
-(defun debbugs-test--soap-invoke-internal
-    (callback _cbargs _wsdl _service operation-name &rest parameters)
-  "Over-ride for testing."
-  (setq debbugs-test--soap-operation-name operation-name)
-  (setq debbugs-test--soap-parameters parameters)
-  (let ((return
-         (cond ((string-equal operation-name "get_status")
-                debbugs-test--bug-status-soap-return)
-               ((string-equal operation-name "get_usertag")
-                '(((hi))))
-               (t '((0))))))
-    (if callback
-        (progn
-          (funcall callback return)
-          nil)
-      return)))
-
-(defun debbugs-test--override-float-time (func &rest rest)
-  "Override `float-time' for FUNC with args REST."
-  (cl-letf (((symbol-function #'float-time)
-             (lambda (&optional _specified-time) 5000)))
-    (apply func rest)))
-
-(defun debbugs-test--setup ()
-  "Mock network and time functions.
-These mock functions are needed to make the tests reproducible."
-  (setq debbugs-test--soap-operation-name nil)
-  (setq debbugs-test--soap-parameters nil)
-
-  (add-function
-   :override (symbol-function #'soap-invoke-internal)
-   #'debbugs-test--soap-invoke-internal)
-
-  (add-function
-   :around (symbol-function #'debbugs-get-cache)
-   #'debbugs-test--override-float-time)
-
-  (add-function
-   :around (symbol-function #'debbugs-put-cache)
-   #'debbugs-test--override-float-time))
-
-(defun debbugs-test--teardown ()
-  "Restore functions to as they where before."
-  (setq debbugs-test--soap-operation-name nil)
-  (setq debbugs-test--soap-parameters nil)
-
-  (remove-function
-   (symbol-function #'soap-invoke-internal)
-   #'debbugs-test--soap-invoke-internal)
-
-  (remove-function
-   (symbol-function #'debbugs-get-cache)
-   #'debbugs-test--override-float-time)
-
-  (remove-function
-   (symbol-function #'debbugs-put-cache)
-   #'debbugs-test--override-float-time))
-
-(defmacro ert-deftest--debbugs (name args docstring &rest body)
-  "The same as `ert-deftest' but runs setup and teardown functions."
-  (declare
-   (doc-string 3)
-   (indent 2))
-  `(ert-deftest ,name ,args ,docstring
-                (debbugs-test--setup)
-                ,@body
-                (debbugs-test--teardown)))
+(require 'debbugs-test-helpers)
 
 ;;; Tests:
 
-- 
2.48.1

>From 1f59c38fb149ec8b5716e89135e9d2fb46c687d9 Mon Sep 17 00:00:00 2001
From: Morgan Smith <[email-masked]>
Date: Fri, 21 Feb 2025 12:47:28 -0500
Subject: [PATCH v2 2/5] tests: Use 'unwind-protect' to ensure test cleanup
 always occurs

* test/debbugs-test-helpers.el(ert-deftest--debbugs): Use 'unwind-protect'.
---
 test/debbugs-test-helpers.el | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/test/debbugs-test-helpers.el b/test/debbugs-test-helpers.el
index f98bab40ed..7ea6f00dce 100644
--- a/test/debbugs-test-helpers.el
+++ b/test/debbugs-test-helpers.el
@@ -136,9 +136,11 @@ These mock functions are needed to make the tests reproducible."
    (doc-string 3)
    (indent 2))
   `(ert-deftest ,name ,args ,docstring
-                (debbugs-test--setup)
-                ,@body
-                (debbugs-test--teardown)))
+                (unwind-protect
+                    (progn
+                      (debbugs-test--setup)
+                      ,@body)
+                  (debbugs-test--teardown))))

 (provide 'debbugs-test-helpers)
-- 
2.48.1

>From 3bd34242bffce78a908ca66ff1073e304d185fda Mon Sep 17 00:00:00 2001
From: Morgan Smith <[email-masked]>
Date: Fri, 21 Feb 2025 12:52:24 -0500
Subject: [PATCH v2 3/5] tests: Add tests for debbugs-gnu

* test/debbugs-gnu-tests.el: New file.
---
 test/debbugs-gnu-tests.el | 47 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)
 create mode 100644 test/debbugs-gnu-tests.el

diff --git a/test/debbugs-gnu-tests.el b/test/debbugs-gnu-tests.el
new file mode 100644
index 0000000000..04c445e43b
--- /dev/null
+++ b/test/debbugs-gnu-tests.el
@@ -0,0 +1,47 @@
+;;; debbugs-gnu-tests.el --- tests for debbugs-gnu.el -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2025 Free Software Foundation, Inc.
+
+;; Author: Morgan Smith <[email-masked]>
+;; Package: debbugs
+
+;; This file is not part of GNU Emacs.
+
+;; This program is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; This program is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; Please ensure tests don't actually make network calls.
+
+;;; Code:
+
+(require 'debbugs)
+(require 'debbugs-gnu)
+(require 'debbugs-test-helpers)
+
+;;; Tests:
+
+(ert-deftest--debbugs debbugs-test-debbugs-gnu-search ()
+  "Test `debbugs-gnu-search'."
+  (cl-letf (((symbol-function #'debbugs-gnu)
+             #'list))
+    (should
+     (equal '(nil ("guix" "guix-patches") nil)
+            (debbugs-gnu-search "frogs" '((pending . "pending")) nil '("guix" "guix-patches") nil)))
+    (should (equal debbugs-gnu-current-query '((phrase . "frogs"))))
+    (should (equal debbugs-gnu-current-filter '((pending . "pending"))))))
+
+(provide 'debbugs-gnu-tests)
+
+;;; debbugs-gnu-tests.el ends here
-- 
2.48.1

>From 9d6fc20715ae9a13a82811bf7b3b3469e060bd45 Mon Sep 17 00:00:00 2001
From: Morgan Smith <[email-masked]>
Date: Fri, 21 Feb 2025 12:52:35 -0500
Subject: [PATCH v2 4/5] Makefile: Add target to run checkdoc on all files

* Makefile: Add target to run checkdoc on all files.
* test/debbugs-checkdoc-config.el: New file.
---
 Makefile                        |  5 +++-
 test/debbugs-checkdoc-config.el | 50 +++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 1 deletion(-)
 create mode 100644 test/debbugs-checkdoc-config.el

diff --git a/Makefile b/Makefile
index eec1c65a14..8ed6a917fe 100644
--- a/Makefile
+++ b/Makefile
@@ -8,7 +8,7 @@ TESTTARGET=$(patsubst %.el,%.elc,$(TESTSOURCE))
 
 INFOMANUALS=debbugs.info debbugs-ug.info
 
-.PHONY: all build check clean
+.PHONY: all build check clean checkdoc
 .PRECIOUS: %.elc
 
 %.elc: %.el
@@ -26,6 +26,9 @@ doc: $(INFOMANUALS)
 
 build: $(TARGET)
 
+checkdoc: $(SOURCE) $(TESTSOURCE)
+	@$(EMACS) -Q --batch -l test/debbugs-checkdoc-config.el $(foreach file,$^,"--eval=(checkdoc-file \"$(file)\")")
+
 check: build $(TESTTARGET)
 	@$(EMACS) -Q --batch -L . -L ./test $(foreach file,$(TESTSOURCE), -l $(file)) -f ert-run-tests-batch-and-exit
 
diff --git a/test/debbugs-checkdoc-config.el b/test/debbugs-checkdoc-config.el
new file mode 100644
index 0000000000..d09b1adfe5
--- /dev/null
+++ b/test/debbugs-checkdoc-config.el
@@ -0,0 +1,50 @@
+;;; debbugs-checkdoc-config.el --- Configuration for running checkdoc on debbugs -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2025 Free Software Foundation, Inc.
+
+;; Author: Morgan Smith <[email-masked]>
+;; Package: debbugs
+
+;; This file is not part of GNU Emacs.
+
+;; This program is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; This program is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;; Code:
+
+(defvar checkdoc-package-keywords-flag)
+(defvar checkdoc-spellcheck-documentation-flag)
+(defvar checkdoc-ispell-lisp-words)
+
+(with-eval-after-load "checkdoc"
+  (setq checkdoc-package-keywords-flag t)
+  (setq checkdoc-spellcheck-documentation-flag t)
+  (setq checkdoc-ispell-lisp-words
+        '("ChangeLog" "ChangeLogs" "UTF" "alist" "args"
+          "armstrong" "backend" "bcc" "bugreport" "cdate" "cedet"
+          "coreutils" "cygwin" "debbugs" "debian" "el" "emacs"
+          "etags" "freemail" "fsf" "guix" "gw" "henoch" "hu"
+          "hyperestraier" "keymap" "magit" "magnus" "maint"
+          "maintainer" "maintainer's" "mbox" "mboxes" "minibuffer"
+          "moreinfo" "multibyte" "notabug" "paren" "persistency"
+          "regexp" "rescan" "rgm" "rmail" "severities" "sexp"
+          "solaris" "src" "sublist" "submitter" "submitter's"
+          "subproduct" "subqueries" "subquery" "teardown"
+          "unarchived" "unibyte" "unreproducible" "url" "util"
+          "wishlist" "wontfix" "wsdl" "www" "xsd" "zltuz")))
+
+(provide 'debbugs-checkdoc-config)
+
+;;; debbugs-checkdoc-config.el ends here
-- 
2.48.1

>From db6e63b75a576bdebbb6800c5afd477886e4bcf9 Mon Sep 17 00:00:00 2001
From: Morgan Smith <[email-masked]>
Date: Fri, 21 Feb 2025 12:49:04 -0500
Subject: [PATCH v2 5/5] tests: Support older Emacs sorting conventions

* test/debbugs-tests.el(debbugs-test-get-status): Call sort with two arguments
to avoid errors on older versions of emacs (such as version 29.4).
---
 test/debbugs-tests.el | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/test/debbugs-tests.el b/test/debbugs-tests.el
index da1b9dffe6..e0479cd811 100644
--- a/test/debbugs-tests.el
+++ b/test/debbugs-tests.el
@@ -64,15 +64,20 @@
 
 (ert-deftest--debbugs debbugs-test-get-status ()
   "Test \"get_status\"."
-  (should (equal (sort (car (debbugs-get-status 64064)))
-                 (sort (car debbugs-test--bug-status))))
+  ;; eval-and-compile gets rid of ridiculous warning:
+  ;; the function `alist-sort' is not known to be defined.
+  (eval-and-compile
+    (defun alist-sort (x y)
+      (string< (car x) (car y))))
+  (should (equal (sort (car (debbugs-get-status 64064)) #'alist-sort)
+                 (sort (car debbugs-test--bug-status) #'alist-sort)))
   (should (string-equal debbugs-test--soap-operation-name "get_status"))
   (should (equal debbugs-test--soap-parameters '([64064])))
   (setq debbugs-test--soap-operation-name nil)
   (setq debbugs-test--soap-parameters nil)
   ;; cached
-  (should (equal (sort (car (debbugs-get-status 64064)))
-                 (sort (car debbugs-test--bug-status))))
+  (should (equal (sort (car (debbugs-get-status 64064)) #'alist-sort)
+                 (sort (car debbugs-test--bug-status) #'alist-sort)))
   (should (equal debbugs-test--soap-operation-name nil))
   (should (equal debbugs-test--soap-parameters nil)))
 
-- 
2.48.1