bug#42168: 26.1; cperl-mode: Bad interpretation of $a++ / $b

[Stefan Kangas]

Hi Harald,

Please see my comments on your patch below.

Harald Jörg <haj@posteo.de> writes:

> diff --git a/test/lisp/progmodes/cperl-mode/cperl-fontification-tests.el 
> b/test/lisp/progmodes/cperl-mode/cperl-fontification-tests.el
> new file mode 100644
> index 0000000000..4148db036c
> --- /dev/null
> +++ b/test/lisp/progmodes/cperl-mode/cperl-fontification-tests.el

Great initiative to also write tests for this bug.

For now, I think you should simply place the file directly at:

lisp/progmodes/cperl-mode-tests.el

We could always break it up into several files later when it gets bigger
and there is a need to do so.

> @@ -0,0 +1,55 @@
> +;;; cperl-fontification-tests.el --- Test fontification in cperl-mode -*- 
> lexical-binding: t -*-
> +
> +;; Copyright (C) 2020-2020 ...to be decided ...

All files in GNU Emacs should be following this template:

;; Copyright (C) 2020 Free Software Foundation, Inc.

(Note that you only need a range if there's more than one year.)

> +;; Author: Harald Jörg <haj@posteo.de>
> +;; Maintainer: Harald Jörg
> +;; Keywords:       internal
> +;; Human-Keywords: internal

Nit: I think we can skip Human-Keywords here.

> +;; Homepage: https://github.com/HaraldJoerg/cperl-mode
> +
> +;;; Commentary:
> +
> +;; This is a collection of Tests for the fontification of CPerl-mode.

Typo: "tests" shouldn't have a capital letter.

> +;; The primary target is to verify that the refactoring we're doing
> +;; right now (May 2020 - ...) doesn't change any behavior, or does the
> +;; right thing in cases where new fontification rules are enabled.

I think these three lines here could be removed, maybe?  It seems to me
that they are describing the purpose of all tests, namely to stop
regressions from happening.

> +;; Run these tests interactively:
> +;; (ert-run-tests-interactively '(tag :fontification))

Nit: Missing this header here:

;;; Code:

> +
> +(defun cperl-test-face (text regexp)
> +  "Returns the face of the first character matched by REGEXP in TEXT."
> +  (interactive)
> +  (with-temp-buffer
> +    (let ((cperl-hairy nil)
> +       (cperl-font-lock nil)) ;; Does this matter?

Does it matter?  Not sure, what happens if you remove it?  :-)

> +      (insert text)
> +      (cperl-mode)
> +      (font-lock-fontify-buffer)
> +      (goto-char (point-min))
> +      (re-search-forward regexp)
> +      (message "%s" (match-string 0))
> +      (get-text-property (match-beginning 0) 'face))))

It is good practice to remove calls to 'message' in the tests, since
they mostly make the tests more noisy.  If it's really useful during
development of tests, you could comment it out or make it dependent upon
a new variable like

> +(ert-deftest jrockway-issue-45 ()

Is probably better named as: cperl-mode-test-bug-42168 to refer back to
our own bug in the name.  We already have the link to jrockway in the
doc string, which is useful if one needs to dig deeper.

> +  "Verify that '/' is a division after ++ or --, not a regexp.
> +Reported in https://github.com/jrockway/cperl-mode/issues/45.
> +If seen as regular expression, then the slash is displayed using
> +font-lock-constant-face.  If seen as a division, then it doesn't
> +have a face property."
> +  :tags '(:fontification)
> +  ;; The next two Perl expressions have divisions.  Perl "punctuation"
> +  ;; operators don't get a face.  The comment at the end of line
> +  ;; prevents cperl-mode from tripping over "End of ‘/ ... /’
> +  ;; string/RE not found" if it gets it wrong

Do we need the comment at the end of the below code fragments with your
patch as well?  If not, couldn't we just remove them?  I think that
would make the intention a little bit clearer, maybe.

> +  (let ((code "{ $a++ / $b } # /"))
> +    (should (equal (cperl-test-face code "/" ) nil)))
> +  (let ((code "{ $a-- / $b } # /"))
> +    (should (equal (cperl-test-face code "/" ) nil)))
> +  ;; The next two Perl expressions have regular expressions.  The
> +  ;; delimiter of a RE is fontified with font-lock-constant-face.
> +  (let ((code "{ $a+ / $b } # /"))
> +    (should (equal (cperl-test-face code "/" ) font-lock-constant-face)))
> +  (let ((code "{ $a- / $b } # /"))
> +    (should (equal (cperl-test-face code "/" ) font-lock-constant-face))))

The rest looks good to me.

Best regards,
Stefan Kangas