[PATCH] Test 'command-completion-using-modes-p' (bug#50228)

[PATCH] Test 'command-completion-using-modes-p' (bug#50228)

[Johannes Maier]

[-- Attachment #1: Type: text/plain, Size: 965 bytes --]

After discovering bug#50228 and asking about tests Lars Ingebrigtsen
replied that "there really should be tests in this area".  I tried
writing some, but these are my first steps with ERT, and Emacs' codebase
in general, so I'm still having a couple of questions:

* There are two assertions in each of the two tests, one positive and
  one negative.  Do you prefer one `ert-deftest' per scenario/branch (or
  even a single one for all cases)?

* The pattern "call with-temp-buffer, maybe enable a mode, then check
  whether a command is applicable" repeats itself.  Is creating small
  helper functions/macros generally advised for such small tests?

There are some more branches/cases that could be tested (maybe the
conditions for multiple modes warrant a test for
`command-completion-with-modes-p' instead), but I feel like I need to
get a better grasp of Elisp and the codebase beforehand, and answers to
these questions would be a first step.

TIA,
Johannes


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: test-command-completion-using-modes-p.patch --]
[-- Type: text/x-patch, Size: 2159 bytes --]

From 0f0acc143f4f3c80baf73b01290c6b64b9cccedf Mon Sep 17 00:00:00 2001
From: Johannes Maier <johannes.maier@mailbox.org>
Date: Sat, 28 Aug 2021 22:29:31 +0200
Subject: [PATCH] Add tests for 'command-completion-using-modes-p' (bug#50228)

* test/lisp/simple-tests.el: Add tests for
'command-completion-using-modes-p'.
---
 test/lisp/simple-tests.el | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/test/lisp/simple-tests.el b/test/lisp/simple-tests.el
index 3ece61290b..7dd3e3dd83 100644
--- a/test/lisp/simple-tests.el
+++ b/test/lisp/simple-tests.el
@@ -971,5 +971,34 @@ test-undo-region
     ;;(should (= (length (delq nil (undo-make-selective-list 5 9))) 0))
     (should (= (length (delq nil (undo-make-selective-list 6 9))) 0))))
 
+(ert-deftest command-completion-using-modes-p-single-major-mode ()
+  "A command that is applicable for one major mode only should be
+filtered out by `command-completion-using-modes-p' if that
+mode (or a derivation thereof) is not active, and accepted
+otherwise."
+  (cl-flet ((cmd (lambda ()
+                   (interactive nil emacs-lisp-mode))))
+    (with-temp-buffer
+      (let ((included (command-completion-using-modes-p #'cmd (current-buffer))))
+        (should-not included)))
+    (with-temp-buffer
+      (emacs-lisp-mode)
+      (let ((included (command-completion-using-modes-p #'cmd (current-buffer))))
+        (should included)))))
+
+(ert-deftest command-completion-using-modes-p-multiple-modes ()
+  "A command that is applicable for more than one mode should be
+filtered out by `command-completion-using-modes-p' if none of the
+modes are active, and accepted otherwise.  See bug#50228."
+  (cl-flet ((cmd (lambda ()
+                   (interactive nil emacs-lisp-mode line-number-mode))))
+    (with-temp-buffer
+      (let ((included (command-completion-using-modes-p #'cmd (current-buffer))))
+        (should-not included)))
+    (with-temp-buffer
+      (line-number-mode)
+      (let ((included (command-completion-using-modes-p #'cmd (current-buffer))))
+        (should included)))))
+
 (provide 'simple-test)
 ;;; simple-test.el ends here
-- 
2.32.0

Re: [PATCH] Test 'command-completion-using-modes-p' (bug#50228)

[Lars Ingebrigtsen]

Johannes Maier <johannes.maier@mailbox.org> writes:

> * There are two assertions in each of the two tests, one positive and
>   one negative.  Do you prefer one `ert-deftest' per scenario/branch (or
>   even a single one for all cases)?

We have a tendency to drop a lot of test cases in the save ert-deftest,
and I think that's fine (as long as they don't get too long -- then it's
a chore to find out what actually failed).

> * The pattern "call with-temp-buffer, maybe enable a mode, then check
>   whether a command is applicable" repeats itself.  Is creating small
>   helper functions/macros generally advised for such small tests?

It goes against all "good programming" practices to repeat ourselves
this much, so the natural inclination everybody has when programming,
even tests, is to write abstractions ten macros deep to avoid that.

Which means that the test code looks really pretty, but it's virtually
impossible to see what's actually being tested, and when something
fails, you have to poke at the harness for half an hour before you see
what's going wrong.

When it comes to writing test code, my rule is: Repeat yourself as much
as possible.  If you can't eval a test case with `C-x C-e' after a form
in Emacs, the code is too pretty.

(I know others disagree.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no