Subject: Re: bug#76372: 31.0.50; False () in ert-font-lock macro signatures

From: "Basil L. Contovounesios" <[email-masked]>
Date: Thu, 20 Feb 2025 10:53:28 +0100

found 76372 30.0.93
quit

Vladimir Kazanov [2025-02-20 08:23 +0000] wrote:

> Feel free to push,

I was about to, but then it occurred to me that ert-font-lock is new in
Emacs 30.

Perhaps the docstring fixes at least should go to emacs-30 instead of
master?

If Eli/Stefan/Andrea agree, I propose the first patch for emacs-30, and
the second for master (they just separate the documentation/test changes
from those to code).

To recap, the main issue in this report is that the arglist of
ert-font-lock-deftest and ert-font-lock-deftest-file is misrepresented
in their documentation and declare specs, which can lead to minor
confusion, misindentation, and misfontification.

> Thanks for looking into this!

Thank *you* for the neat feature and review!
-- 
Basil

>From ea2b2e3612e026129d9cf043d7e2444981084605 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <[email-masked]>
Date: Thu, 20 Feb 2025 10:29:54 +0100
Subject: [PATCH 1/2] Fix ert-font-lock macro signatures

* doc/misc/ert.texi (Syntax Highlighting Tests):
* test/lisp/emacs-lisp/ert-font-lock-tests.el
(test-line-comment-p--emacs-lisp, test-line-comment-p--shell-script)
(test-line-comment-p--javascript, test-line-comment-p--python)
(test-line-comment-p--c, test-macro-test--correct-highlighting)
(test-macro-test--docstring, test-macro-test--failing)
(test-macro-test--file, test-macro-test--file-no-asserts)
(test-macro-test--file-failing): Reindent macro calls.
(with-temp-buffer-str-mode): Evaluate macro arguments left-to-right.
(ert-font-lock--wrap-begin-end): Use rx for more robust composition.
(test-line-comment-p--php): Require that php-mode is callable, not
already loaded.

* lisp/emacs-lisp/ert-font-lock.el (ert-font-lock-deftest)
(ert-font-lock-deftest-file): NAME is not followed by an empty list
like in ert-deftest, so the optional DOCSTRING is actually the
second argument.  Adapt calling convention in docstring, and debug,
doc-string, and indent properties accordingly (bug#76372).  Fix
docstring grammar, document MAJOR-MODE, and avoid referring to a
file name as a path.
---
 doc/misc/ert.texi                           | 10 +--
 lisp/emacs-lisp/ert-font-lock.el            | 39 +++++----
 test/lisp/emacs-lisp/ert-font-lock-tests.el | 94 ++++++++++-----------
 3 files changed, 73 insertions(+), 70 deletions(-)

diff --git a/doc/misc/ert.texi b/doc/misc/ert.texi
index 566fa03bf3f..8f9e4fff415 100644
--- a/doc/misc/ert.texi
+++ b/doc/misc/ert.texi
@@ -962,7 +962,7 @@ Syntax Highlighting Tests
 comments directly and can be read either from inline strings or files.
 The parser expects the input string to contain at least one assertion.
 
-Test assertion parser extracts tests from comment-only lines.  Every
+The test assertion parser extracts tests from comment-only lines.  Every
 comment assertion line starts either with a caret (@samp{^}) or an arrow
 (@samp{<-}).  A single caret/arrow or carets should be followed
 immediately by the name of a face or a list of faces to be checked
@@ -984,7 +984,7 @@ Syntax Highlighting Tests
 @end example
 
 Both symbol-only @code{:face} property values and assertion face values
-are normalized to single element lists so assertions below are
+are normalized to single element lists so the assertions below are
 equivalent:
 
 @example
@@ -1061,7 +1061,7 @@ Syntax Highlighting Tests
 
 @lisp
 (ert-font-lock-deftest test-macro-test--inline
-    emacs-lisp-mode
+  emacs-lisp-mode
   "
 (defun fun ())
 ;; ^ font-lock-keyword-face
@@ -1075,13 +1075,13 @@ Syntax Highlighting Tests
 
 @lisp
 (ert-font-lock-deftest-file test-macro-test--file
-    "Test reading correct assertions from a file"
+  "Test reading correct assertions from a file."
   javascript-mode
   "correct.js")
 @end lisp
 
 The @code{ert-font-lock-deftest} and @code{ert-font-lock-deftest-file}
-macros accept the same keyword parameters as @code{ert-deftest} i.e.,
+macros accept the same keyword arguments as @code{ert-deftest}, i.e.,
 @code{:tag} and @code{:expected-result}.

diff --git a/lisp/emacs-lisp/ert-font-lock.el b/lisp/emacs-lisp/ert-font-lock.el
index e584eb679fd..80f4a6d5467 100644
--- a/lisp/emacs-lisp/ert-font-lock.el
+++ b/lisp/emacs-lisp/ert-font-lock.el
@@ -27,8 +27,8 @@
 ;; highlighting provided by font-lock.
 ;;
 ;; ert-font-lock entry points are functions
-;; `ert-font-lock-test-string' and `ert-font-lock-test-file' and
-;; convenience macros: `ert-font-lock-deftest' and
+;; `ert-font-lock-test-string' and `ert-font-lock-test-file', and
+;; convenience macros `ert-font-lock-deftest' and
 ;; `ert-font-lock-deftest-file'.
 ;;
 ;; See unit tests in ert-font-lock-tests.el for usage examples.
@@ -124,19 +124,21 @@ ert-font-lock--parse-macro-args
 (defmacro ert-font-lock-deftest (name &rest docstring-keys-mode-and-str)
   "Define test NAME (a symbol) using assertions from TEST-STR.
 
-Other than MAJOR-MODE and TEST-STR parameters, this macro accepts
-the same parameters and keywords as `ert-deftest' and is intended
-to be used through `ert'.
+The MAJOR-MODE symbol determines the syntax and font lock of TEST-STR.
 
-\(fn NAME () [DOCSTRING] [:expected-result RESULT-TYPE] \
+Except for the MAJOR-MODE and TEST-STR parameters, this macro accepts
+the same arguments and keywords as `ert-deftest' and is intended to be
+used through `ert'.
+
+\(fn NAME [DOCSTRING] [:expected-result RESULT-TYPE] \
 [:tags \\='(TAG...)] MAJOR-MODE TEST-STR)"
   (declare (debug (&define [&name "test@" symbolp]
-                           sexp [&optional stringp]
+                           [&optional stringp]
                            [&rest keywordp sexp]
                            symbolp
                            stringp))
-           (doc-string 3)
-           (indent 2))
+           (doc-string 2)
+           (indent 1))
   (pcase-let ((`(,documentation
                  ,documentation-supplied-p
                  ,keys ,mode ,arg)
@@ -159,22 +161,23 @@ ert-font-lock-deftest
 (defmacro ert-font-lock-deftest-file (name &rest docstring-keys-mode-and-file)
   "Define test NAME (a symbol) using assertions from FILE.
 
-FILE - path to a file with assertions in ERT resource director as
-return by `ert-resource-directory'.
+FILE names a file with assertions in the ERT resource directory, as
+returned by `ert-resource-directory'.  The MAJOR-MODE symbol determines
+the syntax and font lock of FILE's contents.
 
-Other than MAJOR-MODE and FILE parameters, this macro accepts the
-same parameters and keywords as `ert-deftest' and is intended to
-be used through `ert'.
+Except for the MAJOR-MODE and FILE parameters, this macro accepts the
+same arguments and keywords as `ert-deftest' and is intended to be used
+through `ert'.
 
-\(fn NAME () [DOCSTRING] [:expected-result RESULT-TYPE] \
+\(fn NAME [DOCSTRING] [:expected-result RESULT-TYPE] \
 [:tags \\='(TAG...)] MAJOR-MODE FILE)"
   (declare (debug (&define [&name "test@" symbolp]
-                           sexp [&optional stringp]
+                           [&optional stringp]
                            [&rest keywordp sexp]
                            symbolp
                            stringp))
-           (doc-string 3)
-           (indent 2))
+           (doc-string 2)
+           (indent 1))
 
   (pcase-let ((`(,documentation
                  ,documentation-supplied-p
diff --git a/test/lisp/emacs-lisp/ert-font-lock-tests.el b/test/lisp/emacs-lisp/ert-font-lock-tests.el
index e410cf5fa13..0c3e4405562 100644
--- a/test/lisp/emacs-lisp/ert-font-lock-tests.el
+++ b/test/lisp/emacs-lisp/ert-font-lock-tests.el
@@ -39,13 +39,13 @@ with-temp-buffer-str-mode
   "Create a buffer with STR contents and MODE. "
   (declare (indent 1) (debug t))
   `(with-temp-buffer
-     (insert ,str)
      (,mode)
+     (insert ,str)
      (goto-char (point-min))
      ,@body))
 
 (defun ert-font-lock--wrap-begin-end (re)
-  (concat "^" re "$"))
+  (rx bol (regexp re) eol))
 
 ;;; Regexp tests
 ;;;
@@ -97,89 +97,89 @@ test-line-comment-p--fundamental
 
 (ert-deftest test-line-comment-p--emacs-lisp ()
   (with-temp-buffer-str-mode emacs-lisp-mode
-                             "not comment
+    "not comment
 ;; comment
 "
-                             (should-not (ert-font-lock--line-comment-p))
-                             (forward-line)
-                             (should (ert-font-lock--line-comment-p))
-                             (forward-line)
-                             (should-not (ert-font-lock--line-comment-p))))
+    (should-not (ert-font-lock--line-comment-p))
+    (forward-line)
+    (should (ert-font-lock--line-comment-p))
+    (forward-line)
+    (should-not (ert-font-lock--line-comment-p))))
 
 (ert-deftest test-line-comment-p--shell-script ()
   (with-temp-buffer-str-mode shell-script-mode
-                             "echo Not a comment
+    "echo Not a comment
 # comment
 "
-                             (should-not (ert-font-lock--line-comment-p))
-                             (forward-line)
-                             (should (ert-font-lock--line-comment-p))))
+    (should-not (ert-font-lock--line-comment-p))
+    (forward-line)
+    (should (ert-font-lock--line-comment-p))))
 
 (declare-function php-mode "php-mode")
 (ert-deftest test-line-comment-p--php ()
-  (skip-unless (featurep 'php-mode))
+  (skip-unless (fboundp 'php-mode))
 
   (with-temp-buffer-str-mode php-mode
-                             "echo 'Not a comment'
+    "echo 'Not a comment'
 // comment
 /* comment */
 "
-                             (should-not (ert-font-lock--line-comment-p))
-                             (forward-line)
-                             (should (ert-font-lock--line-comment-p))
-                             (forward-line)
-                             (should (ert-font-lock--line-comment-p))))
+    (should-not (ert-font-lock--line-comment-p))
+    (forward-line)
+    (should (ert-font-lock--line-comment-p))
+    (forward-line)
+    (should (ert-font-lock--line-comment-p))))

 (ert-deftest test-line-comment-p--javascript ()
   (with-temp-buffer-str-mode javascript-mode
-                             "// comment
+    "// comment
 
    // comment, after a blank line
 
 var abc = function(d) {};
 "
-                             (should (ert-font-lock--line-comment-p))
+    (should (ert-font-lock--line-comment-p))
 
-                             (forward-line)
-                             (should-not (ert-font-lock--line-comment-p))
+    (forward-line)
+    (should-not (ert-font-lock--line-comment-p))
 
-                             (forward-line)
-                             (should (ert-font-lock--line-comment-p))
+    (forward-line)
+    (should (ert-font-lock--line-comment-p))
 
-                             (forward-line)
-                             (should-not (ert-font-lock--line-comment-p))
+    (forward-line)
+    (should-not (ert-font-lock--line-comment-p))
 
-                             (forward-line)
-                             (should-not (ert-font-lock--line-comment-p))))
+    (forward-line)
+    (should-not (ert-font-lock--line-comment-p))))
 
 (ert-deftest test-line-comment-p--python ()
 
   (with-temp-buffer-str-mode python-mode
-                             "# comment
+    "# comment
 
    # comment
 print(\"Hello, world!\")"
-                             (should (ert-font-lock--line-comment-p))
+    (should (ert-font-lock--line-comment-p))
 
-                             (forward-line)
-                             (should-not (ert-font-lock--line-comment-p))
+    (forward-line)
+    (should-not (ert-font-lock--line-comment-p))
 
-                             (forward-line)
-                             (should (ert-font-lock--line-comment-p))
+    (forward-line)
+    (should (ert-font-lock--line-comment-p))
 
-                             (forward-line)
-                             (should-not (ert-font-lock--line-comment-p))))
+    (forward-line)
+    (should-not (ert-font-lock--line-comment-p))))
 
 (ert-deftest test-line-comment-p--c ()
 
   (with-temp-buffer-str-mode c-mode
-                             "// comment
+    "// comment
 /* also comment */"
-                             (should (ert-font-lock--line-comment-p))
+    (should (ert-font-lock--line-comment-p))
 
-                             (forward-line)
-                             (should (ert-font-lock--line-comment-p))))
+    (forward-line)
+    (should (ert-font-lock--line-comment-p))))
 
 (ert-deftest test-parse-comments--no-assertion-error ()
   (let* ((str "
@@ -568,14 +568,14 @@ test-font-lock-test-file--wrong
 ;;
 
 (ert-font-lock-deftest test-macro-test--correct-highlighting
-    emacs-lisp-mode
+  emacs-lisp-mode
   "
 (defun fun ())
 ;; ^ font-lock-keyword-face
 ;;      ^ font-lock-function-name-face")
 
 (ert-font-lock-deftest test-macro-test--docstring
-    "A test with a docstring."
+  "A test with a docstring."
   emacs-lisp-mode
   "
 (defun fun ())
@@ -583,7 +583,7 @@ fun
   )
 
 (ert-font-lock-deftest test-macro-test--failing
-    "A failing test."
+  "A failing test."
   :expected-result :failed
   emacs-lisp-mode
   "
@@ -591,18 +591,18 @@ fun
 ;; ^ wrong-face")
 
 (ert-font-lock-deftest-file test-macro-test--file
-    "Test reading correct assertions from a file"
+  "Test reading correct assertions from a file."
   javascript-mode
   "correct.js")
 
 (ert-font-lock-deftest-file test-macro-test--file-no-asserts
-    "Check failing on files without assertions"
+  "Check failing on files without assertions."
   :expected-result :failed
   javascript-mode
   "no-asserts.js")
 
 (ert-font-lock-deftest-file test-macro-test--file-failing
-    "Test reading wrong assertions from a file"
+  "Test reading wrong assertions from a file."
   :expected-result :failed
   javascript-mode
   "broken.js")
-- 
2.47.2

>From e53bcc902db2223045f518facf8d95ac102bfb56 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <[email-masked]>
Date: Thu, 20 Feb 2025 10:34:00 +0100
Subject: [PATCH 2/2] Clean up ert-font-lock error messages

* lisp/emacs-lisp/ert-font-lock.el: Remove redundant requires.
(ert-font-lock--parse-macro-args): First return value 'doc' being
nil already indicates omission of a docstring, so remove redundant
second return value doc-p.  All users updated.  Fix error
messages (bug#76372).
(ert-font-lock--check-faces): Simplify with ensure-list.
---
 lisp/emacs-lisp/ert-font-lock.el | 38 ++++++++++++--------------------
 1 file changed, 14 insertions(+), 24 deletions(-)

diff --git a/lisp/emacs-lisp/ert-font-lock.el b/lisp/emacs-lisp/ert-font-lock.el
index 80f4a6d5467..0f8e90d747c 100644
--- a/lisp/emacs-lisp/ert-font-lock.el
+++ b/lisp/emacs-lisp/ert-font-lock.el
@@ -37,8 +37,6 @@
 
 (require 'ert)
 (require 'ert-x)
-(require 'newcomment)
-(require 'pcase)
 
 (defconst ert-font-lock--face-symbol-re
   (rx (+ (or alphanumeric "-" "_" "." "/")))
@@ -100,25 +98,24 @@ ert-font-lock--test-body-file
 
 (defun ert-font-lock--parse-macro-args (doc-keys-mode-arg)
   "Parse DOC-KEYS-MODE-ARG macro argument list."
-  (let (doc doc-p mode arg)
+  (let (doc mode arg)
 
     (when (stringp (car doc-keys-mode-arg))
-      (setq doc (pop doc-keys-mode-arg)
-            doc-p t))
+      (setq doc (pop doc-keys-mode-arg)))
 
     (pcase-let
         ((`(,keys ,mode-arg)
           (ert--parse-keys-and-body doc-keys-mode-arg)))
 
       (unless (symbolp (car mode-arg))
-        (error "A major mode symbol expected: %S" (car mode-arg)))
+        (error "Expected a major mode symbol: %S" (car mode-arg)))
       (setq mode (pop mode-arg))
 
       (unless (stringp (car mode-arg))
-        (error "A string or file with assertions expected: %S" (car mode-arg)))
+        (error "Expected a string or file with assertions: %S" (car mode-arg)))
       (setq arg (pop mode-arg))
 
-      (list doc doc-p keys mode arg))))
+      (list doc keys mode arg))))
 
 ;;;###autoload
 (defmacro ert-font-lock-deftest (name &rest docstring-keys-mode-and-str)
@@ -139,22 +136,20 @@ ert-font-lock-deftest
                            stringp))
            (doc-string 2)
            (indent 1))
-  (pcase-let ((`(,documentation
-                 ,documentation-supplied-p
-                 ,keys ,mode ,arg)
+  (pcase-let ((`(,documentation ,keys ,mode ,arg)
                (ert-font-lock--parse-macro-args docstring-keys-mode-and-str)))
 
     `(ert-set-test ',name
                    (make-ert-test
                     :name ',name
-                    ,@(when documentation-supplied-p
+                    ,@(when documentation
                         `(:documentation ,documentation))
                     ,@(when (map-contains-key keys :expected-result)
                         `(:expected-result-type ,(map-elt keys :expected-result)))
                     ,@(when (map-contains-key keys :tags)
                         `(:tags ,(map-elt keys :tags)))
-                    :body (lambda () (ert-font-lock--test-body-str ',mode ,arg ',name))
-
+                    :body (lambda ()
+                            (ert-font-lock--test-body-str ',mode ,arg ',name))
                     :file-name ,(or (macroexp-file-name) buffer-file-name)))))
 
 ;;;###autoload
@@ -178,23 +173,20 @@ ert-font-lock-deftest-file
                            stringp))
            (doc-string 2)
            (indent 1))
-
-  (pcase-let ((`(,documentation
-                 ,documentation-supplied-p
-                 ,keys ,mode ,arg)
+  (pcase-let ((`(,documentation ,keys ,mode ,arg)
                (ert-font-lock--parse-macro-args docstring-keys-mode-and-file)))
 
     `(ert-set-test ',name
                    (make-ert-test
                     :name ',name
-                    ,@(when documentation-supplied-p
+                    ,@(when documentation
                         `(:documentation ,documentation))
                     ,@(when (map-contains-key keys :expected-result)
                         `(:expected-result-type ,(map-elt keys :expected-result)))
                     ,@(when (map-contains-key keys :tags)
                         `(:tags ,(map-elt keys :tags)))
                     :body (lambda () (ert-font-lock--test-body-file
-                                 ',mode (ert-resource-file ,arg) ',name))
+                                      ',mode (ert-resource-file ,arg) ',name))
                     :file-name ,(or (macroexp-file-name) buffer-file-name)))))
 
 (defun ert-font-lock--in-comment-p ()
@@ -357,10 +349,8 @@ ert-font-lock--check-faces
 
       ;; normalize both expected and resulting face - these can be
       ;; either symbols, nils or lists of symbols
-      (when (not (listp actual-face))
-        (setq actual-face (list actual-face)))
-      (when (not (listp expected-face))
-        (setq expected-face (list expected-face)))
+      (setq actual-face (ensure-list actual-face))
+      (setq expected-face (ensure-list expected-face))
 
       ;; fail when lists are not 'equal and the assertion is *not negated*
       (when (and (not negation) (not (equal actual-face expected-face)))
-- 
2.47.2