bug#69714: 30.0.50; ert-font-lock doesn't handle list of faces

[Troy Brown <brownts@troybrown.dev>]

The new patch looks good to me.

One other thing I thought I'd mention.  I have places in my test where
I want to verify that there is no font lock face.  I've been able to
specify "nil" for the face and that works perfectly to check this.  I
thought I'd mention this because I wasn't sure if it was intentional
behavior, but I do find that useful.


Thanks,

Troy.

On Wed, Mar 13, 2024 at 1:04 PM Vladimir Kazanov <vekazanov@gmail.com> wrote:
>
> Thanks a lot!
>
> The suggestions really do make sense.
>
> Here's the final integrated patch, complete with updated tests and
> docs. If you're fine with it then I'll ask somebody to install it on
> master.
>
> PS I've got to write an additional announcement in the main mailing
> list inviting people to check the updated version out.
>
> On Wed, 13 Mar 2024 at 16:14, Troy Brown <brownts@troybrown.dev> wrote:
> >
> > Hi Vlad, sorry for the delayed response.
> >
> > I haven't pushed my change which uses this package yet, as I was
> > struggling to get it working and didn't want to push failing tests.  I
> > just discovered the package and was working on a regression test for a
> > bug fix involving font locking.  This seemed like the perfect reason
> > to use your package.  At the moment I only have this one
> > work-in-progress test, but I expect to use it more going forward.
> >
> > I did check out your patch and for my immediate needs, it worked
> > perfectly.  Thanks!  Additionally, I did experiment a little with the
> > multi-caret functionality, which is nice as I have a use for that.  I
> > also experimented with the negation functionality (although I don't
> > have an immediate need for that), and did notice a couple things.  The
> > first was that the assertion would be ignored if there was a space
> > between the negation symbol and the face.  Also, if the actual and
> > expected faces didn't match and the negation flag was being used, it
> > acted like the negation was not indicated at all and failed the test.
> > I've included a diff below containing the changes I made which seemed
> > to address those concerns.
> >
> > diff --git a/lisp/emacs-lisp/ert-font-lock.el b/lisp/emacs-lisp/ert-font-lock.el
> > index 06c90add9d3..1a5fe96fb09 100644
> > --- a/lisp/emacs-lisp/ert-font-lock.el
> > +++ b/lisp/emacs-lisp/ert-font-lock.el
> > @@ -61,6 +61,7 @@ ert-font-lock--assertion-line-re
> >     (group (zero-or-more (seq "^" (zero-or-more whitespace))))
> >     ;; optional negation of the face specification
> >     (group (optional "!"))
> > +   (zero-or-more whitespace)
> >     ;; face symbol name or a list of symbols
> >     (group (or (regexp ert-font-lock--face-symbol-re)
> >                (regexp ert-font-lock--face-symbol-list-re))))
> > @@ -354,7 +355,7 @@ ert-font-lock--check-faces
> >        (when (symbolp expected-face)
> >          (setq expected-face (list expected-face)))
> >
> > -      (when (not (equal actual-face expected-face))
> > +      (when (and (not negation) (not (equal actual-face expected-face)))
> >          (ert-fail
> >           (list (format "Expected face %S, got %S on line %d column %d"
> >                         expected-face actual-face line-checked column-checked)
> >
> >
> > Thanks,
> >
> > Troy.
> >
> > On Tue, Mar 12, 2024 at 4:47 PM Vladimir Kazanov <vekazanov@gmail.com> wrote:
> > >
> > > Hi,
> > >
> > > I've attached a patch that handles face lists, fails on files without
> > > assertions and expands the parser a bit to support multiple carets per
> > > line.
> > >
> > > For faces it does the following:
> > >
> > > 1. Turn symbols into single element lists.
> > > 2. Parses face lists from the assertions.
> > > 3. Compare face lists using equas.
> > >
> > > Could you please check if this works for you?
> > >
> > > Thanks
> > >
> > > On Mon, 11 Mar 2024 at 08:36, Vladimir Kazanov <vekazanov@gmail.com> wrote:
> > > >
> > > > Hi,
> > > >
> > > > Thanks for reporting this! I have a bunch of ert-font-lock
> > > > improvements in my local repo getting ready for submission, and can
> > > > look into your suggestions as well.
> > > >
> > > > Do you have your unit test code somewhere in a public repo? It'd be
> > > > great to think of further improvements to support your use case.
> > > >
> > > > Thanks,
> > > > Vlad
> > > >
> > > > On Sun, 10 Mar 2024 at 20:33, Troy Brown <brownts@troybrown.dev> wrote:
> > > > >
> > > > > I'm trying to use this package to test out my tree-sitter mode, but am
> > > > > running into an issue with lists of faces.  It's possible that the
> > > > > face for a location in the buffer will contain a list of 1 or more
> > > > > faces.  For example, when I use the ":override 'prepend" keyword in
> > > > > the call to treesit-font-lock-rules, even if only a single face is
> > > > > specified for the rule that matches that section of the buffer, this
> > > > > will result in a list of one entry (i.e., "(face-name)").
> > > > >
> > > > > When this happens, ert-font-lock fails to recognize that this matches
> > > > > the face "face-name" (e.g., "^ face-name" will fail to match in this
> > > > > case).  I feel the tool should recognize a list containing a single
> > > > > face as matching the face.  Even worse however, it appears
> > > > > ert-font-lock doesn't support a list of faces in the comment.  I tried
> > > > > to work around the original issue by using "^ (face-name)", but the
> > > > > tool silently ignores this, as it doesn't match the internal regular
> > > > > expression (which ended up allowing my test to pass without actually
> > > > > checking anything).
> > > > >
> > > > > I can't figure out a way to use this tool in its current state due to
> > > > > its lack of support for a list of faces.  Also, I find that since it
> > > > > silently ignores incorrect comment syntax (e.g., "^face-name", "^
> > > > > (face-name)"), it gives a false illusion that it's actually performing
> > > > > those checks (and the checks are passing), when it's really just
> > > > > ignoring them.  Maybe any comment line starting with a "^" or "<-"
> > > > > should be considered an assertion check and to fail if the rest of the
> > > > > syntax is not as expected.  Maybe it should also fail the test if no
> > > > > assertion checks are found in a source file or string.
> > > > >
> > > > > Even if the tool would allow a list of a single face to match the
> > > > > supplied face in the comment, I think it should also allow for
> > > > > multiple faces to be listed in the comment.  I have other places where
> > > > > multiple faces are used (e.g., "(font-lock-constant-face
> > > > > font-lock-variable-name-face)" to highlight a constant variable),
> > > > > which would not be testable with the current state of the package.
> > > > >
> > > > >
> > > > >
> > > >
> > > >
> > > > --
> > > > Regards,
> > > >
> > > > Vladimir Kazanov
> > >
> > >
> > >
> > > --
> > > Regards,
> > >
> > > Vladimir Kazanov
>
>
>
> --
> Regards,
>
> Vladimir Kazanov