See attached 3 patches. Tests pass on emacs 29, 30, and 31.0.5. Most of the tests pass on emacs 28 except the ones that hit the recent addition of `string-equal-ignore-case' in "lisp/org-element.el".
Changes since last review: - New patch (Make testing macros hygenic) - `ob-tangle/tangle-to-self', `ob-tangle/bibtex', use `buffer-file-name' instead of macro bound `file' Ihor Radchenko <[email protected]> writes: > Morgan Smith <[email protected]> writes: > >>> This won't work. FILE variable is used to derive the tangled file name. >>> Same for some clauses you changed in >>> test-org-capture/org-capture-expand-olp. >> >> The macro `org-test-with-temp-text-in-file' exposes the variable FILE. >> ... > > Hmm. That's actually a bad code practice. (which is why I missed it) > Normally, macros should not bind variables (especially not without > documenting it in the docstring). In the main source code, Org even has > a special `org-with-gensyms' that makes sure that let-binding inside > macro do not have a chance to interfere with the code using the macro. > > So, what org-test-with-temp-text-in-file does is not right and can cause > hard-to-debug errors of the type > (let ((buffer (current-buffer))) ; parent buffer > (org-with-temp-text-in-file "..." > (should (equal (buffer-string) (with-current-buffer buffer ...))))) > > At least, we should (1) make the let-bound file name inside > `org-test-with-temp-text-in-file' more unique. Like > org-test-temp-file-name; (2) Document this let-bind in the > docstring. Then, your patch will be ok. I have gone through all the macro's in "testing/org-test.el" and made them hygenic. Then I fixed all tests that relied on those local variables (there was only 4). All the test files compile without any new warnings with my local patches applied that address all the existing warnings (I'm working on upstreaming those). So I'm fairly confident I caught all uses of the macro injected variables.
>From 3fb0f0a51a1de80a8d020424755505f54128c2f1 Mon Sep 17 00:00:00 2001 From: Morgan Smith <[email protected]> Date: Sun, 19 Oct 2025 19:48:17 -0400 Subject: [PATCH 1/3] testing: Make testing macros hygenic Ensure macros don't expose any local variables. * testing/org-test.el (org-test-at-id, org-test-in-example-file) (org-test-with-temp-text, org-test-with-temp-text-in-file) (org-test-with-exported-text): Use `org-with-gensyms' to avoid defining local variables. * testing/lisp/test-ob-tangle.el (ob-tangle/comment-links-relative-file) (ob-tangle/comment-noweb-relative, ob-tangle/comment-noweb-absolute) (ob-tangle/comment-noweb-absolute): Use `buffer-file-name' instead of variable bound by `org-test-with-temp-text-in-file'. --- testing/lisp/test-ob-tangle.el | 12 ++- testing/org-test.el | 165 +++++++++++++++++---------------- 2 files changed, 93 insertions(+), 84 deletions(-) diff --git a/testing/lisp/test-ob-tangle.el b/testing/lisp/test-ob-tangle.el index cd6876370..83dddb116 100644 --- a/testing/lisp/test-ob-tangle.el +++ b/testing/lisp/test-ob-tangle.el @@ -151,7 +151,8 @@ ob-tangle/comment-links-relative-file 1 #+end_src" (unwind-protect - (let ((org-babel-tangle-use-relative-file-links t)) + (let ((org-babel-tangle-use-relative-file-links t) + (file buffer-file-name)) (org-babel-tangle) (with-temp-buffer (insert-file-contents "test-ob-tangle.el") @@ -169,7 +170,8 @@ ob-tangle/comment-links-relative-file 1 #+end_src" (unwind-protect - (let ((org-babel-tangle-use-relative-file-links nil)) + (let ((org-babel-tangle-use-relative-file-links nil) + (file buffer-file-name)) (org-babel-tangle) (with-temp-buffer (insert-file-contents "test-ob-tangle.el") @@ -195,7 +197,8 @@ ob-tangle/comment-noweb-relative <<inner>>) #+end_src" (unwind-protect - (let ((org-babel-tangle-use-relative-file-links t)) + (let ((org-babel-tangle-use-relative-file-links t) + (file buffer-file-name)) (org-babel-tangle) (with-temp-buffer (insert-file-contents "test-ob-tangle.el") @@ -223,7 +226,8 @@ ob-tangle/comment-noweb-absolute <<inner>> #+end_src" (unwind-protect - (let ((org-babel-tangle-use-relative-file-links nil)) + (let ((org-babel-tangle-use-relative-file-links nil) + (file buffer-file-name)) (org-babel-tangle) (with-temp-buffer (insert-file-contents "test-ob-tangle.el") diff --git a/testing/org-test.el b/testing/org-test.el index 46ec56608..bc63b62b5 100644 --- a/testing/org-test.el +++ b/testing/org-test.el @@ -119,48 +119,50 @@ org-test-compare-with-file (defmacro org-test-at-id (id &rest body) "Run body after placing the point in the headline identified by ID." (declare (indent 1) (debug t)) - `(let* ((id-location (org-id-find ,id)) - (id-file (car id-location)) - (visited-p (get-file-buffer id-file)) - to-be-removed) - (unwind-protect - (save-window-excursion - (save-match-data - (org-id-goto ,id) - (setq to-be-removed (current-buffer)) - (condition-case nil - (progn - (org-fold-show-subtree) - (org-fold-show-all '(blocks))) - (error nil)) - (save-restriction ,@body))) - (unless (or visited-p (not to-be-removed)) - (kill-buffer to-be-removed))))) + (org-with-gensyms (id-location id-file visited-p to-be-removed) + `(let* ((,id-location (org-id-find ,id)) + (,id-file (car ,id-location)) + (,visited-p (get-file-buffer ,id-file)) + ,to-be-removed) + (unwind-protect + (save-window-excursion + (save-match-data + (org-id-goto ,id) + (setq ,to-be-removed (current-buffer)) + (condition-case nil + (progn + (org-fold-show-subtree) + (org-fold-show-all '(blocks))) + (error nil)) + (save-restriction ,@body))) + (unless (or ,visited-p (not ,to-be-removed)) + (kill-buffer ,to-be-removed)))))) (defmacro org-test-in-example-file (file &rest body) "Execute body in the Org example file." (declare (indent 1) (debug t)) - `(let* ((my-file (or ,file org-test-file)) - (visited-p (get-file-buffer my-file)) - to-be-removed - results) - (save-window-excursion - (save-match-data - (find-file my-file) - (unless (eq major-mode 'org-mode) - (org-mode)) - (setq to-be-removed (current-buffer)) - (goto-char (point-min)) - (condition-case nil - (progn - (outline-next-visible-heading 1) - (org-fold-show-subtree) - (org-fold-show-all '(blocks))) - (error nil)) - (setq results (save-restriction ,@body)))) - (unless visited-p - (kill-buffer to-be-removed)) - results)) + (org-with-gensyms (my-file visited-p to-be-removed results) + `(let* ((,my-file (or ,file org-test-file)) + (,visited-p (get-file-buffer ,my-file)) + ,to-be-removed + ,results) + (save-window-excursion + (save-match-data + (find-file ,my-file) + (unless (eq major-mode 'org-mode) + (org-mode)) + (setq ,to-be-removed (current-buffer)) + (goto-char (point-min)) + (condition-case nil + (progn + (outline-next-visible-heading 1) + (org-fold-show-subtree) + (org-fold-show-all '(blocks))) + (error nil)) + (setq ,results (save-restriction ,@body)))) + (unless ,visited-p + (kill-buffer ,to-be-removed)) + ,results))) (defmacro org-test-at-marker (file marker &rest body) "Run body after placing the point at MARKER in FILE. @@ -179,19 +181,20 @@ org-test-with-temp-text then remove it and place the point there before running BODY, otherwise place the point at the beginning of the inserted text." (declare (indent 1) (debug t)) - `(let ((inside-text (if (stringp ,text) ,text (eval ,text))) - (org-mode-hook nil)) - (with-temp-buffer - (org-mode) - (let ((point (string-match "<point>" inside-text))) - (if point - (progn - (insert (replace-match "" nil nil inside-text)) - (goto-char (1+ (match-beginning 0)))) - (insert inside-text) - (goto-char (point-min)))) - (font-lock-ensure (point-min) (point-max)) - ,@body))) + (org-with-gensyms (inside-text) + `(let ((,inside-text (if (stringp ,text) ,text (eval ,text))) + (org-mode-hook nil)) + (with-temp-buffer + (org-mode) + (let ((point (string-match "<point>" ,inside-text))) + (if point + (progn + (insert (replace-match "" nil nil ,inside-text)) + (goto-char (1+ (match-beginning 0)))) + (insert ,inside-text) + (goto-char (point-min)))) + (font-lock-ensure (point-min) (point-max)) + ,@body)))) (defmacro org-test-with-temp-text-in-file (text &rest body) "Run body in a temporary file buffer with Org mode as the active mode. @@ -199,27 +202,28 @@ org-test-with-temp-text-in-file place the point there before running BODY, otherwise place the point at the beginning of the buffer." (declare (indent 1) (debug t)) - `(let ((file (make-temp-file "org-test")) - (inside-text (if (stringp ,text) ,text (eval ,text))) - buffer) - (with-temp-file file (insert inside-text)) - (unwind-protect - (progn - ;; FIXME: For the rare cases where we do need to mess with windows, - ;; we should let `body' take care of displaying this buffer! - (setq buffer (find-file file)) - (when (re-search-forward "<point>" nil t) - (replace-match "")) - (org-mode) - (progn ,@body)) - (let ((kill-buffer-query-functions nil)) - (when buffer - (set-buffer buffer) - ;; Ignore changes, we're deleting the file in the next step - ;; anyways. - (set-buffer-modified-p nil) - (kill-buffer)) - (delete-file file))))) + (org-with-gensyms (file inside-text buffer) + `(let ((,file (make-temp-file "org-test")) + (,inside-text (if (stringp ,text) ,text (eval ,text))) + ,buffer) + (with-temp-file ,file (insert ,inside-text)) + (unwind-protect + (progn + ;; FIXME: For the rare cases where we do need to mess with windows, + ;; we should let `body' take care of displaying this buffer! + (setq ,buffer (find-file ,file)) + (when (re-search-forward "<point>" nil t) + (replace-match "")) + (org-mode) + (progn ,@body)) + (let ((kill-buffer-query-functions nil)) + (when ,buffer + (set-buffer ,buffer) + ;; Ignore changes, we're deleting the file in the next step + ;; anyways. + (set-buffer-modified-p nil) + (kill-buffer)) + (delete-file ,file)))))) (defun org-test-table-target-expect (target &optional expect laps &rest tblfm) "For all TBLFM: Apply the formula to TARGET, compare EXPECT with result. @@ -601,14 +605,15 @@ org-test-get-day-name (defmacro org-test-with-exported-text (backend source &rest body) "Run BODY in export buffer for SOURCE string via BACKEND." (declare (indent 2)) - `(org-test-with-temp-text ,source - (let ((export-buffer (generate-new-buffer "Org temporary export"))) - (unwind-protect - (progn - (org-export-to-buffer ,backend export-buffer) - (with-current-buffer export-buffer - ,@body)) - (kill-buffer export-buffer))))) + (org-with-gensyms (export-buffer) + `(org-test-with-temp-text ,source + (let ((,export-buffer (generate-new-buffer "Org temporary export"))) + (unwind-protect + (progn + (org-export-to-buffer ,backend ,export-buffer) + (with-current-buffer ,export-buffer + ,@body)) + (kill-buffer ,export-buffer)))))) (provide 'org-test) -- 2.51.0
>From eac9ec4237b78ebc0bfa51534b0fb8a89a9f85ad Mon Sep 17 00:00:00 2001 From: Morgan Smith <[email protected]> Date: Mon, 11 Aug 2025 13:24:36 -0400 Subject: [PATCH 2/3] * testing/lisp/test-org-agenda.el: Isolate test errors If a test failed, it would not perform the cleanup which would cause all subsequent tests to fail. * testing/lisp/test-org-agenda.el (org-test-agenda-with-agenda): Use unwind-protect to ensure cleanup happens. --- testing/lisp/test-org-agenda.el | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/testing/lisp/test-org-agenda.el b/testing/lisp/test-org-agenda.el index fb6dd3342..36da85fc0 100644 --- a/testing/lisp/test-org-agenda.el +++ b/testing/lisp/test-org-agenda.el @@ -46,8 +46,10 @@ org-test-agenda-with-agenda (declare (indent 1)) `(org-test-with-temp-text-in-file ,text (let ((org-agenda-files `(,buffer-file-name))) - ,@body - (org-test-agenda--kill-all-agendas)))) + (unwind-protect + (progn + ,@body) + (org-test-agenda--kill-all-agendas))))) ;; Test the Agenda -- 2.51.0
>From 6ffd1786bc8ad8544dfbd9d63ccb3d52700a911c Mon Sep 17 00:00:00 2001 From: Morgan Smith <[email protected]> Date: Thu, 16 Oct 2025 13:40:58 -0400 Subject: [PATCH 3/3] testing: Suppress interactive prompts during testing and on quit Running the tests interactively would give the user multiple prompts asking if they wanted to kill a modified buffer. Then when quitting Emacs, the user would receive prompts asking if they wanted to save some of the temporary test files. * testing/org-test.el (org-test-kill-buffer): New function. (org-test-at-id, org-test-in-example-file, org-test-with-temp-text-in-file, org-test-kill-all-examples): Use new function to kill buffers. * testing/lisp/test-ob-tangle.el (ob-tangle/tangle-to-self, ob-tangle/bibtex): Use `org-test-with-temp-text-in-file' to ensure buffers are killed. * testing/lisp/test-org-agenda.el (test-org-agenda/set-priority): Use `unwind-protect' for test cleanup and kill a buffer to "agenda-file.org". * testing/lisp/test-org-capture.el (test-org-capture/org-capture-expand-olp): Use `org-test-with-temp-text-in-file' to ensure buffers are killed. * testing/lisp/test-org-protocol.el (test-org-protocol/org-protocol-capture-file): Add cleanup for buffers and files. * testing/lisp/test-ox.el (test-org-export/org-export-copy-buffer): Kill buffer. --- testing/lisp/test-ob-tangle.el | 45 +++++++++++++------------------ testing/lisp/test-org-agenda.el | 12 +++++---- testing/lisp/test-org-capture.el | 20 +++++--------- testing/lisp/test-org-protocol.el | 16 ++++++----- testing/lisp/test-ox.el | 3 ++- testing/org-test.el | 22 ++++++++------- 6 files changed, 58 insertions(+), 60 deletions(-) diff --git a/testing/lisp/test-ob-tangle.el b/testing/lisp/test-ob-tangle.el index 83dddb116..2ab3950eb 100644 --- a/testing/lisp/test-ob-tangle.el +++ b/testing/lisp/test-ob-tangle.el @@ -559,17 +559,14 @@ ob-tangle/strip-tangle (ert-deftest ob-tangle/tangle-to-self () "Do not allow tangling into self." - (let ((file (make-temp-file "org-tangle-" nil ".org"))) - (unwind-protect - (with-current-buffer (find-file-noselect file) - (insert - (format " -#+begin_src elisp :tangle %s + (org-test-with-temp-text-in-file + " +#+begin_src elisp :tangle <point> 2 #+end_src -" file)) - (should-error (org-babel-tangle))) - (delete-file file)))) +" + (insert buffer-file-name) + (should-error (org-babel-tangle)))) (ert-deftest ob-tangle/detangle-false-positive () "Test handling of false positive link during detangle." @@ -721,29 +718,25 @@ ob-tangle/collect-blocks (ert-deftest ob-tangle/bibtex () "Tangle BibTeX into a `.bib' file." - (let ((file (make-temp-file "org-tangle-" nil ".org")) - (bib "@Misc{example, + (let ((bib "@Misc{example, author = {Richard Stallman and {contributors}}, title = {{GNU} {Emacs}}, publisher = {Free Software Foundation}, url = {https://www.emacs.org/}, }")) - (unwind-protect - (with-current-buffer (find-file-noselect file) - (insert (format "#+begin_src bibtex :tangle yes + (org-test-with-temp-text-in-file + (format "#+begin_src bibtex :tangle yes %s -#+end_src" - bib)) - (org-babel-tangle) - (let ((bib-file - (if (fboundp 'file-name-with-extension) - (file-name-with-extension file "bib") - ;; Emacs <28 - (concat (file-name-sans-extension file) "." "bib")))) - (should (file-exists-p bib-file)) - (should (string= (string-trim (org-file-contents bib-file)) - bib)))) - (delete-file file)))) +#+end_src" bib) + (org-babel-tangle) + (let ((bib-file + (if (fboundp 'file-name-with-extension) + (file-name-with-extension buffer-file-name "bib") + ;; Emacs <28 + (concat (file-name-sans-extension buffer-file-name) "." "bib")))) + (should (file-exists-p bib-file)) + (should (string= (string-trim (org-file-contents bib-file)) + bib)))))) ;; See https://list.orgmode.org/87msfxd81c.fsf@localhost/T/#t (ert-deftest ob-tangle/tangle-from-capture-buffer () diff --git a/testing/lisp/test-org-agenda.el b/testing/lisp/test-org-agenda.el index 36da85fc0..46ae94229 100644 --- a/testing/lisp/test-org-agenda.el +++ b/testing/lisp/test-org-agenda.el @@ -385,11 +385,13 @@ test-org-agenda/set-priority ;; `org-today' or not. (org-agenda-list nil "<2017-07-19 Wed>") (set-buffer org-agenda-buffer-name) - (should - (progn (goto-line 3) - (org-agenda-priority ?B) - (looking-at-p " *agenda-file:Scheduled: *\\[#B\\] test agenda")))) - (org-test-agenda--kill-all-agendas)) + (unwind-protect + (should + (progn (goto-line 3) + (org-agenda-priority ?B) + (looking-at-p " *agenda-file:Scheduled: *\\[#B\\] test agenda"))) + (org-test-agenda--kill-all-agendas) + (org-test-kill-buffer "agenda-file.org")))) (ert-deftest test-org-agenda/sticky-agenda-name () "Agenda buffer name after having created one sticky agenda buffer." diff --git a/testing/lisp/test-org-capture.el b/testing/lisp/test-org-capture.el index 494fee4cf..6b49a2df7 100644 --- a/testing/lisp/test-org-capture.el +++ b/testing/lisp/test-org-capture.el @@ -1089,28 +1089,22 @@ test-org-capture/org-capture-expand-olp (should (equal '("A" "B" "C") - (let ((file (make-temp-file "org-test"))) - (unwind-protect - (org-capture-expand-olp file "A" "B" "C") - (delete-file file))))) + (org-test-with-temp-text-in-file "" + (org-capture-expand-olp buffer-file-name "A" "B" "C")))) ;; The current buffer during the funcall of the lambda is the temporary ;; test file. (should - (let ((file (make-temp-file "org-test"))) + (org-test-with-temp-text-in-file "" (equal - file - (unwind-protect - (org-capture-expand-olp file (lambda () (buffer-file-name))) - (delete-file file))))) + buffer-file-name + (org-capture-expand-olp buffer-file-name (lambda () (buffer-file-name)))))) ;; `org-capture-expand-olp' rejects outline path that is not ;; inlined. (should-error (equal '("A" "B" "C") - (let ((file (make-temp-file "org-test"))) - (unwind-protect - (org-capture-expand-olp file '("A" "B" "C")) - (delete-file file)))))) + (org-test-with-temp-text-in-file "" + (org-capture-expand-olp buffer-file-name '("A" "B" "C")))))) (provide 'test-org-capture) ;;; test-org-capture.el ends here diff --git a/testing/lisp/test-org-protocol.el b/testing/lisp/test-org-protocol.el index 6429432a3..d31940d3e 100644 --- a/testing/lisp/test-org-protocol.el +++ b/testing/lisp/test-org-protocol.el @@ -164,12 +164,16 @@ test-org-protocol/org-protocol-capture-file (temp-file-name (make-temp-file "org-protocol-test")) (org-capture-templates `(("t" "Test" plain (file ,temp-file-name) "%a\n%i\n" :kill-buffer t)))) - (let ((uri "/org-protocol:/capture:/t/file%3A%2F%2F%2Fetc%2Fmailcap/Triple%20Slash/Body")) - (should (null (org-protocol-check-filename-for-protocol uri (list uri) nil))) - (should (string= (buffer-string) "[[file:///etc/mailcap][Triple Slash]]\nBody"))) - (let ((uri "/org-protocol:/capture?template=t&url=file%3A%2F%2F%2Fetc%2Fmailcap&title=Triple%20Slash&body=Body")) - (should (null (org-protocol-check-filename-for-protocol uri (list uri) nil))) - (should (string= (buffer-string) "[[file:///etc/mailcap][Triple Slash]]\nBody"))))) + (unwind-protect + (progn + (let ((uri "/org-protocol:/capture:/t/file%3A%2F%2F%2Fetc%2Fmailcap/Triple%20Slash/Body")) + (should (null (org-protocol-check-filename-for-protocol uri (list uri) nil))) + (should (string= (buffer-string) "[[file:///etc/mailcap][Triple Slash]]\nBody"))) + (let ((uri "/org-protocol:/capture?template=t&url=file%3A%2F%2F%2Fetc%2Fmailcap&title=Triple%20Slash&body=Body")) + (should (null (org-protocol-check-filename-for-protocol uri (list uri) nil))) + (should (string= (buffer-string) "[[file:///etc/mailcap][Triple Slash]]\nBody")))) + (org-test-kill-buffer (get-file-buffer temp-file-name)) + (delete-file temp-file-name)))) (ert-deftest test-org-protocol/org-protocol-open-source () "Test org-protocol://open-source links." diff --git a/testing/lisp/test-ox.el b/testing/lisp/test-ox.el index 279642d94..ddf37a5f6 100644 --- a/testing/lisp/test-ox.el +++ b/testing/lisp/test-ox.el @@ -75,7 +75,8 @@ test-org-export/org-export-copy-buffer "* Heading" (with-temp-buffer (insert-file-contents file) - (buffer-string))))))) + (buffer-string)))) + (org-test-kill-buffer (current-buffer))))) ;; The copy must not show when re-opening the original file. (org-test-with-temp-text-in-file "* Heading" diff --git a/testing/org-test.el b/testing/org-test.el index bc63b62b5..28e095440 100644 --- a/testing/org-test.el +++ b/testing/org-test.el @@ -116,6 +116,15 @@ org-test-compare-with-file If file is not given, search for a file named after the test currently executed.") +(defun org-test-kill-buffer (buffer) + "Kill BUFFER like `kill-buffer' but without user interaction." + (setq buffer (get-buffer buffer)) + (when (and buffer (buffer-live-p buffer)) + (with-current-buffer buffer + ;; Prevent "Buffer *temp* modified; kill anyway?". + (set-buffer-modified-p nil) + (kill-buffer)))) + (defmacro org-test-at-id (id &rest body) "Run body after placing the point in the headline identified by ID." (declare (indent 1) (debug t)) @@ -136,7 +145,7 @@ org-test-at-id (error nil)) (save-restriction ,@body))) (unless (or ,visited-p (not ,to-be-removed)) - (kill-buffer ,to-be-removed)))))) + (org-test-kill-buffer ,to-be-removed)))))) (defmacro org-test-in-example-file (file &rest body) "Execute body in the Org example file." @@ -161,7 +170,7 @@ org-test-in-example-file (error nil)) (setq ,results (save-restriction ,@body)))) (unless ,visited-p - (kill-buffer ,to-be-removed)) + (org-test-kill-buffer ,to-be-removed)) ,results))) (defmacro org-test-at-marker (file marker &rest body) @@ -217,12 +226,7 @@ org-test-with-temp-text-in-file (org-mode) (progn ,@body)) (let ((kill-buffer-query-functions nil)) - (when ,buffer - (set-buffer ,buffer) - ;; Ignore changes, we're deleting the file in the next step - ;; anyways. - (set-buffer-modified-p nil) - (kill-buffer)) + (org-test-kill-buffer ,buffer) (delete-file ,file)))))) (defun org-test-table-target-expect (target &optional expect laps &rest tblfm) @@ -455,7 +459,7 @@ org-test-touch-all-examples (defun org-test-kill-all-examples () (while org-test-buffers (let ((b (pop org-test-buffers))) - (when (buffer-live-p b) (kill-buffer b))))) + (org-test-kill-buffer b)))) (defun org-test-update-id-locations () (setq org-id-locations-file -- 2.51.0
