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

[Michael Albinus]

João Távora <joaotavora@gmail.com> writes:

Hi João,

> 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.

Both conventions are used: <package>-test-FOO and <package>-tests-FOO.
In the test/ subdirectory, (dired-do-find-regexp "deftest.*-tests-")
returns 1073 lines, and (dired-do-find-regexp "deftest.*-test-") returns
1245 lines. If you prefer the "eglot-tests" prefix, rename it (I really
don't care :-)

> 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.

Yes. That's why I have added the comment. As soon as we check for other
LSP servers but "clangd", it must be changed.

> 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))

Might work. You could also remove function eglot--call-with-tramp-test,
and move its body into the macro eglot--with-tramp-test. I don't know
whether skip-unless then still works (it is applicable inside
ert-deftest only), but you could replace its call by ert-skip as I have done.

> 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.

I've followed the discussion about shorthands a little bit, but I have
no opinion about it.

> João

Best regards, Michael.