Re: emacs-29 9c0d7bb73b 2/2: Add automated tests for Eglot

["João Távora" <joaotavora@gmail.com>]

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

On Wed, Dec 14, 2022 at 9:57 AM Michael Albinus <michael.albinus@gmx.de>
wrote:
> > >  Is it possible to give them a unique prefix, like `eglot-test-'?
> >
> > Yes, do that please. I think this makes sense, although the test
> > symbols aren't exactly bound to functions and it is a nuisance to
> > always type this long boring prefix. If only there was some
> > namespacing mechanism in Emacs that could alleviate that.. oh wait ;-)
>
> I've renamed the tests.

Thanks.  But shouldn't it be "eglot-tests" (with trailing 's') to match the
file name eglot-tests.el?  I'm sorry I didn't notice this before.

Also another comment about your earlier commit.

+        (temporary-file-directory ert-remote-temporary-file-directory))
+    ;; We must check the remote LSP server.  So far, just "clangd" is used.
+    (let ((default-directory temporary-file-directory))
+      (unless (executable-find "clangd" 'remote)
+        (ert-skip "Remote clangd not found")))
+    (funcall fn)))

I don't think it's correct to check for "clangd" specifically in
'eglot--call-with-tramp-test', because that restricts its usefulness.

Isn't the following better and still correct?

diff --git a/test/lisp/progmodes/eglot-tests.el
b/test/lisp/progmodes/eglot-tests.el
index d8c9560f5b..509e7e7d82 100644
--- a/test/lisp/progmodes/eglot-tests.el
+++ b/test/lisp/progmodes/eglot-tests.el
@@ -1265,20 +1265,25 @@ eglot--call-with-tramp-test
          (tramp-verbose 1)
          (temporary-file-directory ert-remote-temporary-file-directory)
          (default-directory temporary-file-directory))
-    ;; We must check the remote LSP server.  So far, just "clangd" is used.
-    (unless (executable-find "clangd" 'remote)
-      (ert-skip "Remote clangd not found"))
     (funcall fn)))

+(cl-defmacro eglot--with-tramp-test (() &body body)
+  (declare (indent 1) (debug t))
+  `(eglot--call-with-tramp-test (lambda () ,@body)))
+
 (ert-deftest eglot-test-tramp-test ()
   "Ensure LSP servers can be used over TRAMP."
   :tags '(:expensive-test)
-  (eglot--call-with-tramp-test
#'eglot-tests--auto-detect-running-server-1))
+  (eglot--with-tramp-test ()
+    (skip-unless (executable-find "clangd" 'remote))
+    (eglot-tests--auto-detect-running-server-1)))

 (ert-deftest eglot-test-tramp-test-2 ()
   "Ensure LSP servers can be used over TRAMP."
   :tags '(:expensive-test)
-  (eglot--call-with-tramp-test #'eglot-tests--lsp-abiding-column-1))
+  (eglot--with-tramp-test ()
+    (skip-unless (executable-find "clangd" 'remote))
+    (eglot-tests--lsp-abiding-column-1)))

 (ert-deftest eglot-test-path-to-uri-windows ()
   (skip-unless (eq system-type 'windows-nt))

> But there is no need to type the long test
> names, use completion.

In general completion systems can't guess that I want to make
a test `eglot-test-a-new-test-im-about-to-write`.  And there's also
the clutter of overly long names which seriously impairs readability.
This is a longstanding problem and recurring argument here.  I won't
go into it except to note that if you were to apply this patch:

@@ -1317,4 +1317,5 @@ eglot-test-same-server-multi-mode

 ;; Local Variables:
 ;; checkdoc-force-docstrings-flag: nil
+;; read-symbol-shorthands: (("et-" . "eglot-tests-"))
 ;; End:

then you could just type _and_ read `et-` every time and
the symbol et-foo would be read and interned as
eglot-tests-foo.  And you could keep longhand mentions
to eglot-tests-foo in there as well, and they would also map
to the same symbol.

But this is not allowed practice yet in the Emacs repo.
Unfortunately, IMO.

João

[-- Attachment #2: Type: text/html, Size: 4301 bytes --]