Move ert tests into separate file and run tests in CI.

[tmcgilchrist]

The improvements in https://github.com/ocaml/merlin/pull/1759 introduced a dependency on ert tests which makes merlin-mode in ELPA depend on test code. It would be better to not require test code when using merlin-mode.

This change puts the test code into its own file and introduced a CI step to run the tests.

How does the ELPA release process work? The new merlin-cap-test.el should be excluded from the package sent to ELPA.

[tmcgilchrist]

https://github.com/tmcgilchrist/merlin/pull/2 shows test failures on Emacs 28 and below.

Related to this merlin-mode now uses functions only available in 29.1. Should older there be support for versions before this?

$  opam exec -- ./check.sh
....
merlin-cap.el:1:86: warning: You should depend on (emacs "24.1") if you need lexical-binding.
merlin-cap.el:19:10: error: You should depend on (emacs "24.4") if you need `subr-x'.
merlin-cap.el:21:1: error: You should depend on (emacs "24.3") if you need `defvar-local'.
merlin-cap.el:30:1: error: You should depend on (emacs "24.3") if you need `defvar-local'.
merlin-cap.el:50:1: error: You should depend on (emacs "24.4") if you need `define-error'.
merlin-cap.el:51:38: error: You should depend on (emacs "29.1") if you need `position-symbol'.
merlin-cap.el:56:1: error: You should depend on (emacs "24.3") if you need `defvar-local'.
merlin-cap.el:67:1: error: You should depend on (emacs "24.3") if you need `defvar-local'.
merlin-cap.el:73:1: error: You should depend on (emacs "24.3") if you need `defvar-local'.
merlin-cap.el:75:1: error: You should depend on (emacs "24.3") if you need `defvar-local'.
merlin-cap.el:77:1: error: You should depend on (emacs "24.3") if you need `defvar-local'.
merlin-cap.el:79:1: error: You should depend on (emacs "24.3") if you need `defvar-local'.
merlin-cap.el:81:1: error: You should depend on (emacs "24.3") if you need `defvar-local'.
merlin-cap.el:112:20: error: You should depend on (emacs "25.1") or the compat package if you need `when-let'.
merlin-cap.el:126:23: error: You should depend on (emacs "25.1") if you need `make-pipe-process'.
merlin-cap.el:134:23: error: You should depend on (emacs "25.1") if you need `make-process'.
merlin-cap.el:170:5: error: You should depend on (emacs "24.3") or the cl-lib package if you need `cl-assert'.
merlin-cap.el:171:5: error: You should depend on (emacs "25.1") or the compat package if you need `when-let'.
merlin-cap.el:224:20: error: You should depend on (emacs "25.1") or the compat package if you need `alist-get'.
merlin-cap.el:225:19: error: You should depend on (emacs "25.1") or the compat package if you need `alist-get'.
merlin-cap.el:230:22: error: You should depend on (emacs "25.1") or the compat package if you need `alist-get'.
merlin-cap.el:231:61: error: You should depend on (emacs "25.1") or the compat package if you need `alist-get'.
merlin-cap.el:251:22: error: You should depend on (emacs "25.1") or the compat package if you need `alist-get'.
merlin-cap.el:251:56: error: You should depend on (emacs "25.1") or the compat package if you need `alist-get'.
merlin-cap.el:252:21: error: You should depend on (emacs "25.1") or the compat package if you need `alist-get'.
merlin-cap.el:253:21: error: You should depend on (emacs "25.1") or the compat package if you need `alist-get'.
merlin-cap.el:259:31: error: You should depend on (emacs "24.4") if you need `string-remove-suffix'.
merlin-cap.el:339:13: error: You should depend on (emacs "28.1") or the compat package if you need `string-search'.
merlin-cap.el:354:3: error: You should depend on (emacs "25.1") or the compat package if you need `when-let'.
merlin-cap.el:354:19: error: You should depend on (emacs "28.1") or the compat package if you need `string-search'.
merlin-cap.el:374:18: error: You should depend on (emacs "28.1") or the compat package if you need `string-search'.
merlin-cap.el:485:0: error: Aliases should start with the package's prefix "merlin-cap".

[xvw]

Hi! Thanks a lot! What do you think about moving test files in a dedicated subfolder inside emacs/ (ie: emacs/test)? And could you add in the readme a way to run tests locally?

Aside, I think that installing merlin-mode via ELPA/MELPA is not the right way to do since MELPA use main as a source. So we can lead to have a feature implemented in merlin-mode, still not available in ocamlmerlin. We can probably, at the mode level, relay on merlin version to avoid that, but FMPOV, just relaying on the files installed into opam var share is more robuts.

[voodoos]

cc @purcell who wrote melpa's receipe

[purcell]

What do you think about moving test files in a dedicated subfolder inside emacs/ (ie: emacs/test)?

MELPA already excludes *-test.el, unsure how ELPA handles things. A subfolder would work well for MELPA too — I can easily update the package recipe there.

Aside, I think that installing merlin-mode via ELPA/MELPA is not the right way to do since MELPA use main as a source. So we can lead to have a feature implemented in merlin-mode, still not available in ocamlmerlin.

Regarding MELPA using main, that's not a huge issue since there's also the MELPA Stable package version based on the latest version tag here — users can use that instead if they have issues. Shipping merlin-mode via opam just means the elisp here can't rely on any proper Emacs package dependencies, and it's not a great install/set-up experience for Emacs users, though I get your point about it helping the mutual compatibility.

[xvw]

@purcell Thanks a lot for the explanation (and for the clarification about -test files. Just out of curiosity, how to deal, via MELPA (even in stable version) to deal with specific version of merlin-mode. For example, let's imagine that I use, in my switch merlin xxx.xxx but the latest stable version of elpa/melpa is merlin yyy.yyy? The only to "fix that" seems to add a way to check the version of merlin inside the el file, since we will take some time to write a advanced extension mode for LSP via Emacs, it is something interesting to keep in mind.

[xvw]

Note that "partially answer to one of my previous question"

It seems that we can probably deal with version difference using a mechanism like "capabilities" (already present in LSP).

[purcell]

Yes, generally you'd indeed have to do some version checking of merlin within the elisp and handle any difference in capabilities there. With package.el you can't explicitly install older versions of packages, so it's all a little primitive. :)

[tmcgilchrist]

@xvw I'm running the tests as opam exec -- ./check.sh which expects a local Emacs installation and an opam switch for merlin.

@purcell What's the recommended way to deal with backwards compatible elisp? I thought about using boundp and fboundp to detect new functions like string-search and providing a fall back compatibility function. Is there a better option than that? Maybe using the Advising Functions https://www.gnu.org/software/emacs/manual/html_node/elisp/Advising-Functions.html

[purcell]

What's the recommended way to deal with backwards compatible elisp? I thought about using boundp and fboundp to detect new functions like string-search and providing a fall back compatibility function. Is there a better option than that? Maybe using the Advising Functions https://www.gnu.org/software/emacs/manual/html_node/elisp/Advising-Functions.html

Depending on the compat package solves a lot of that. Otherwise, you'd normally make your own wrapper, e.g.

(if (fboundp 'string-search)
    (defalias 'merlin--string-search 'string-search)
  (defun merlin--string-search (...)
     ...))

and use that wrapper in the code. It would be frowned upon to simply define string-search if it is found to be missing (as it could be a sentinel for other packages, or the local reimplementation could be wrong), and advising functions generally implies changing someone else's function, which is also not often a good thing for a package to do.

[voodoos]

So indeed tests are falling on emacs <= 28.2, which is very unfortunate to say the least.

I will try to test if completion does indeed work or not with emacs 28.

[voodoos]

@xvw do you think you could check that the new completion at point works on Emacs 28.2 ? I am having difficulties installing it on my machine...

[voodoos]

@purcell if it turns out that the new completion does not work on emacs <= 28.2 and that the fix is not-trivial, would it be an option to split merlin in two recipes, a legacy one which ships the old merlin-cap module and another one that ships the new completion module for emacs >= 29 ?

(thanks a lot to you and @bbatsov for all the emacs related help BTW !)

[voodoos]

There is another issue with this: merlin mode now requires the compat package and that breaks the installation with opam user-setup which does not install that package automatically.

To summarize the current issues:

• The new CAP requires the compat package for emacs < 29.1 this plays badly with the opam-based setup
• Even when compat is used tests are not passing on emacs <= 28.2

I guess we could use our own shims for the unavailable functions, but I am not familiar enough with emacs, elisp or melpa to fix all these issues in a timely manner, if anyone want to step in and have a look this will be much welcome. Meanwhile I think reverting the new completion changes is the best thing to do. I will open a draft PR with all these changes so that anyone willing to tackle the task will have a fresh base to build from.

The rewrite is an exciting improvement, but it really needs more work before we roll it :-)

[tmcgilchrist]

@voodoos Ideally we can choose the correct implementation for completion by detecting the Emacs version and loading that. I'll see if I can make that work.

What does the opam-based setup actually do? Can we change the instructions or Elisp it adds to explicitly (require 'compat)?

[bbatsov]

@voodoos Ideally we can choose the correct implementation for completion by detecting the Emacs version and loading that. I'll see if I can make that work.

I think that'd be an overkill, given the it will likely be easier to fix the updated completion on older Emacsen.

I agree with @voodoos that it would probably be best to revert the updated completion for a bit, so users are not impacted by the current breakage. In the mean time we can setup a proper test matrix, etc - if we can one the issues wouldn't have been caught so late.

One more note - probably it doesn't make sense to support anything older than Emacs 27 at this point, as Emacs 26 was released a long time ago and pretty much no one uses it at this point.

[voodoos]

What does the opam-based setup actually do? Can we change the instructions or Elisp it adds to explicitly (require 'compat)?

The opam-based setup modifies the user's .emacs file to load an elisp file that will discover opam packages that provide emacs plugins like merlin and tuareg and load the appropriate files. It will do minimal modifications in an existing .emacs file or create a new .emacs file from a template if there is none.

It expects that the plugins are "standalone" and does not configure any package manager in Emacs such as Elpa. It won't pull additional packages required by the plugins (such as compat). We might be able to vendor them as an alternative, or replace it by our own compatibility functions.

One more note - probably it doesn't make sense to support anything older than Emacs 27 at this point, as Emacs 26 was released a long time ago and pretty much no one uses it at this point.

Noted!

I would like to do a release soonish, so I will revert the changes and create a branch with the merged changes plus the content of this PR so that we can continue working on this. If it's ready before I release, we can re-merge it.

[purcell]

it doesn't make sense to support anything older than Emacs 27 at this point

Yep, agree with this. And then perhaps you can get away without needing any compatibility functions. package-lint will mostly tell you if the code is using functions/features that aren't available in the minimum Emacs version the package claims to support.

What's the issue with completion in Emacs < 28.2 though? completion-at-point has been around for a long time so it should be possible to write a backend that works with all Emacsen >= 27.

[bbatsov]

@purcell One of the new tests fails on Emacs 28, but I'm not sure why. Likely the code is using something that behaves differently on Emacs 29, but I doubt it's something hard to fix.

[voodoos]

Let's move further discussions to the new PR: https://github.com/ocaml/merlin/pull/1827