nvim-lspconfig

nvim-lspconfig copied to clipboard

Some initial discussion at #1059.

Currently the plan is to create a new repo under nvim-lua that will be a place to put per-config tests to test basic LSP integration of that language, at least as far as implemented by its lua/lspconfig/*.lua file.

@mjlbach WIP prototype at rish987/nvim-lspconfig-test. At the moment, what the workflow does is, for each language server in the tests/ directory:

1. runs the test.sh file
2. if: -- error and no existing issue, automatically creates one titled for that specific language server -- error and existing issue, does nothing -- no error and existing issue, automatically closes it -- no error and no existing issue, does nothing

Will make a few more improvements add ACTUAL tests for leanls/lean3ls soon (likely tommorrow, in fact)!

(CC: @Julian)

Alright it should be ready for a detailed review at this point! @ rish987/nvim-lspconfig-test. I've added you both as collaborators, feel free to leave comments/make any changes as you see fit. Also feel free to test out the automatic issue creation/closing by causing/fixing errors (e.g. rish987/nvim-lspconfig-test@7646a40).

I've really been wanting to get this done for https://github.com/williamboman/nvim-lsp-installer. I think we should join forces on this, where nvim-lsp-installer could fill in the gap and shoulder the responsibility of server installation/management.

I've thought of either having super rudimentary tests that only uses neovim as a scripting platform to run the downloads, but then the tests would really just check the existence of an executable. I think what would be even better is acceptance tests that would run in a neovim instance end-to-end, and make sure that servers actually start properly and are responding reliably to client requests. I imagine this could either be achieved via a headless neovim instance, or if that's not possible through some containerised instance (through a Puppeteer-like API).

The issue is tests might be extremely annoying to maintain,as if we checked for specific responses from the language server this would be heavily tied to the version of the server itself. We could pin the version of the language servers to mitigate this but that semi-defeats the point. I guess I would recommend a pinned + non-pinned tests so we know if the regression is from lspconfig or from the server.

Ah that's good to know, I'm not very familiar with those details. I was actually thinking testing further out in the stack, like user acceptance testing. The interfaces that would be involved would exclusively be core neovim APIs (like buffer contents, cursor position, etc.). I think if combined with a BDD-approach, it could be a pretty low maintenance effort (mostly a one-time cost initially).

Feature: tsserver

  Scenario: Go to a buffer-local definition
    Given a file "index.ts" with the contents
        """
const my_number = 5

console.log(my_number)
        """
    And a cursor position of "3:13"
    When I go to definition
    Then cursor position should be "1:7"

The above could be further parameterized to avoid having to individually write these for each server. Maybe the resolved client capabilities could govern which features should be tested (have to bear in mind dynamic capability registration though). Also, maybe treesitter could be used to construct ASTs that then are translated into a given source language, to be used as fixture files in tests.

I think a good start would be to just keep it simple and expand from that. I vaguely know there's the concept of "initializing a server" that is part of the spec - maybe that could be the first target to test, that each server successfully initialize?

I don't understand how that is different than the extensive testing we already do of the built in client in core.

Well yes and no. I'm not too familiar with what those tests do, but I imagine they're primarily testing the client implementation and spec compliance? What I'm after is testing and verifying that each of the server configurations provided by lspconfig actually work in an acceptable manner, and as a precursor to that that the installation in nvim-lsp-installer is correct. Sort of like system integration tests

I guess I'm confused because that doesn't avoid the issue of needing to hardcode the expected response per server version regardless of how we parameterize/standardize the request.

I'd say that depends on what you're inspecting and asserting on. If you take the perspective of an end-user, you have (rightfully) no idea what the server is responding to the neovim client in the background, or perhaps even that there even is an LSP server running. All you know is whether go to definition, rename, find references, etc. is working. Those are all user actions that have a pretty clear motive and an expected result. Building out a test scenario that performs those user actions and observes that expected behavior takes place could prove pretty useful I think, for many reasons.

Hi there, thanks for your interest! I've just recently started to get back into testing. In the experimental repo rish987/nvim-lspconfig-test I have the requirement to "formalize" the install steps of nvim-lspconfig for each server in the form of a BASH script, but your plugin seems to be much more in the right spirit of neovim. So we can start by replacing that layer with nvim-lsp-installer.

I think what would be even better is acceptance tests that would run in a neovim instance end-to-end, and make sure that servers actually start properly and are responding reliably to client requests.

I've already added these for Lean 3 and 4 servers in that repo, see for example tests/leanls/tests/lsp_spec.lua. It tests that hover and definition requests and client creation work as they should on Lean files in the different kind of project structures we're supposed to support.

The issue is tests might be extremely annoying to maintain,as if we checked for specific responses from the language server this would be heavily tied to the version of the server itself.

This problem could be minimized if we're careful about how much we're testing. For example it that test file above I just check for a small portion of the hover response that I would expect never to change between versions.

I guess I would recommend a pinned + non-pinned tests so we know if the regression is from lspconfig or from the server.

Yes I think this is something that will be necessary as well. When a PR comes in for new tests for a server we could also require that they provide an nvim-lsp-installer script for a fixed version, and test both in CI.

Maybe the resolved client capabilities could govern which features should be tested (have to bear in mind dynamic capability registration though).

We could separate tests by file (e.g. definition, hover, etc.) and have a helper that errors if the capability corresponding to that file isn't supported by the server. But to start I think we should keep it simple and assume fixed capabilities across server versions.

Also, maybe treesitter could be used to construct ASTs that then are translated into a given source language, to be used as fixture files in tests.

This sounds very interesting and ambitious -- has it been done before? I'd imagine we'd need to unify parts of treesitter grammars between languages to make this happen. And to do so we'd probably initially need to have some protocol/standard that everyone can agree on. (I don't know enough about treesitter yet to know whether this has already been done...) But I think we'll have to start out by just writing tests manually.

I'd probably rather just write a dockerfile per server than rely on nvim-lsp-installer as we can pin the system packages as well (and we need to set up system dependencies anyways). I kinda started this in https://github.com/nvim-lsp/try.nvim. We could share the rest of the testing infrastructure though.

This sounds very interesting and ambitious -- has it been done before? I'd imagine we'd need to unify parts of treesitter grammars between languages to make this happen. And to do so we'd probably initially need to have some protocol/standard that everyone can agree on. (I don't know enough about treesitter yet to know whether this has already been done...) But I think we'll have to start out by just writing tests manually.

This was just a vague idea I came up with with not much forethought to be honest. There probably could be benefits of taking such an approach, but the complexity would skyrocket. Starting small and iterating fast is the way to go for sure :+1:

I'd probably rather just write a dockerfile per server than rely on nvim-lsp-installer as we can pin the system packages as well (and we need to set up system dependencies anyways). I kinda started this in https://github.com/nvim-lsp/try.nvim. We could share the rest of the testing infrastructure though.

With the risk of me just talking about vague, unmaterialized, ideas - I've had the idea of further decoupling the different components in nvim-lsp-installer, going from having installers represented as composed Lua functions that spawn processes (or other system operations) on the host system to instead providing static descriptions of system requirements and the install steps. That'd allow for a number of different use cases, like translating it to platform-specific jobs that run on the host system, or compiling to a different format such as a Dockerfile. Having access to these static descriptors would also improve the quality and maintainability of certain plugin-specific GUI features (and probably a nicer interop with plugins like distant.nvim).

I'll see if I can contribute anything to what you've gotten started with @rish987. I might also just start by targeting some of the problems I'd like to solve for nvim-lsp-installer first, and then we can circle back at a later time and re-evaluate

See https://github.com/rish987/nvim-lspconfig-test/pull/57 for a working POC.