Subject: bug#76446: 0001-tests-Move-helper-and-wrapper-functions-into-their-o.patch

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

Michael Albinus <[email-masked]> writes:

> Morgan Smith <[email-masked]> writes:
>
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -24,7 +24,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 . $(foreach file,$(TESTSOURCE), -l $(file)) -f ert-run-tests-batch-and-exit
>
> We might be better with
>
> --8<---------------cut here---------------start------------->8---
> @$(EMACS) -Q --batch -L . -L ./test -l $(TESTSOURCE) -f ert-run-tests-batch-and-exit
> --8<---------------cut here---------------end--------------->8---
>

I did the '-L ./test' part but I kept the foreach.

Without the foreach the command looks like "... -l file1.el file2.el
file3.el ..." and emacs only actually loads the first file.  The command
needs to look like "... -l file1.el -l file2.el -l file3.el ..."

>> --- /dev/null
>> +++ b/test/debbugs-test-helpers.el
>> @@ -0,0 +1,146 @@
>> +(defconst debbugs-test--bug-status-soap-return
>> +(defconst debbugs-test--bug-status
>
> Perhaps we should move these data into an ert-resource file (or two).
>

I read the info manual and source code pertaining to 'ert-resource' and
I fail to see any benefit from using it.

>> +(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)))
>
> In order to ensure that debbugs-test--teardown is called even in case of
> errors, we could use
>
> (unwind-protect
>     (progn
>       (debbugs-test--setup)
>       ,@body)
>   (debbugs-test--teardown))
>

Good catch!  Thank you!  I don't want to move things and change them at
the same time so now this is two patches.

>> --- a/test/debbugs-tests.el
>> +++ b/test/debbugs-tests.el
>> @@ -27,118 +27,7 @@
>>  ;;; Code:
>>
>>  (require 'debbugs)
>> +(require 'debbugs-test-helpers "./test/debbugs-test-helpers")
>
> (require 'debbugs-test-helpers) should be sufficient with the proposed
> change in the Makefile.

Done!

> 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