First draft of tests for Github Actions

[tombh]

I'd like to contribute the fix to #42. But first I wonder whether it'd be useful to have, and indeed whether you want, automatic tests? This is just a rough first draft, just to see what you think.

The current passing runs are on my fork: https://github.com/tombh/lsp-format.nvim/runs/6374422003?check_suite_focus=true

I was inspired by https://github.com/rafcamlet/nvim-luapad and https://github.com/williamboman/nvim-lsp-installer

Luapad has an interesting approach where not only is the spec runner run in a headless Neovim, but each individual spec is itself run inside a remote nvim instance. I assume this is because of the interactive nature of the plugin, sending keypresses etc. So I followed that approach, it may turn out to be unnecessary.

I chose Deno and its language server to test against just because I know that it's relatively easy to install and setup. For instance there's already a oneliner Github action to set it up.

A judicious use of sleeping is made, which is always a bad sign for tests. But this is inevitable with the asynchronous nature of both LSP attachment and LSP formatting. Sleep too little and we get false failures, sleep too much and the test suite becomes annoying. There may be better ways to detect ready states.

In any case, this is merely a quick first draft, just to see if this is a direction worth exploring

[lukas-reineke]

Thank you for the effort

I am all for tests. But this is easier to do when you go with the current plenary test harness https://github.com/nvim-lua/plenary.nvim#plenarytest_harness No need for the helper file and complex setup. Just pass a simple init.lua and define the tests.

I also don't think it is a good idea to test with a real LSP. That is way too much overhead, the RPC calls are super simple. The whole client should just be mocked.

[tombh]

I don't have any intuitive sense of how how mocking the whole client is less overhead than using a real LSP. Can you give an example? Especially in the case of interacting with asynchronous formatting over the boundary of Normal to Insert mode.

Edit: Oh, by client, I think you mean the LSP server? That makes more sense, I see what you mean then. It would indeed be easier then to do things like create a fake formatting delay for testing the async Normal to Insert mode behaviour.

[tombh]

Yes, that's much simpler, thanks. I pushed that change as a new commit.

So about mocking the LSP server calls. I think that would mean stubbing clean.request_sync and client.sync in M._format()? I'm looking at the Neovim LSP code for vim.lsp, that's where we find client.request for example. It's documented as private and seems to be a runtime function? As in it's returned by another function at runtime, so it's harder to be mocked. Similarly for vim.lsp.rpc.client. They may need to be mocked using another method, like intercepting package.loaded, there's some ideas in this Stack Overflow answer. I've never mocked with Lua before, so I could be missing something obvious. I'd imagine that just refactoring lsp-format's code to wrap those client calls into their own single-purpose mockable functions would be easier.

Interestingly I see that Neovim has its own fake language server.

Just a couple of other things I'm wondering about mocking:

• Does a client still need to be attached to the buffer in order to trigger formatting? Would that state need to be faked too?
• client.request() also takes a params argument. Would it still be good to have the ability to test that params like tab width etc are being sent and honoured?

[lukas-reineke]

I'm looking at the Neovim LSP code for vim.lsp, that's where we find client.request for example. It's documented as private and seems to be a runtime function? As in it's returned by another function at runtime, so it's harder to be mocked. Similarly for vim.lsp.rpc.client. They may need to be mocked using another method, like intercepting package.loaded, there's some ideas in this Stack Overflow answer.

This is a lot simpler, we don't need an actual client object. Just make a table that you can pass into the on_attach function. It only has to implement the functions the plugin is using. And those should be super simple.

Does a client still need to be attached to the buffer in order to trigger formatting? Would that state need to be faked too?

yes and no. We don't have to actually attach it, but we need to overwrite vim.lsp.buf_get_clients to make that work. But that is super simple in lua, you can just redefine the function. vim.lsp.buf_get_clients = function() return "super easy" end

client.request() also takes a params argument. Would it still be good to have the ability to test that params like tab width etc are being sent and honoured?

Being sent, yes. That should be tested. But that should be easy when client.request is our mock function. Being honored is out of scope, that's problem of the LSP server.

[tombh]

I'm a bit lost. Does this seem like the right direction?

[tombh]

Great, that cleared it up for me, thanks. What do you think of that one spec now? It's very simple, but core, and passing, so it's progress. I can start to add a few more, including one to test the Normal to Insert mode edge case. Then I'll submit this PR. Then think about a solution to #42 that can be submitted with its own specs in a separate PR.

[lukas-reineke]

Looking good :+1: I'm fine with merging it with one spec as the base. Can you format the code with stylua please? And also add the specs folder to luacheck in .github/workflows/pr_check.yml (you will also need to add the busted global variables to .luacheckrc)

[tombh]

Ok, made those changes too. Only thing is that now the tests are getting stuck on Github Actions 🤔 They run until they're timed out after 6 hours, and there's nothing helpful in the logs. I wonder if Vim is prompting for user input or something.

Also, do you have stylua and luacheck setup on your formatter? Or you run them separately from the CLI? I do have formatting, using this very plugin in fact, but it doesn't conform, is that normal? Do I have add something to my lsp-config to get it to pick up the rc files?

[lukas-reineke]

the tests are getting stuck on Github Actions

not totaly sure either. Because there isn't even the Running lsp-format specs... output, I suspect the git clone doesn't work. Maybe try to do this in the github action with actions/checkout like in pr_check

do you have stylua and luacheck setup on your formatter?

I do. I use EFM or this. https://github.com/lukas-reineke/dotfiles/blob/2e24258034f855500ea16018e1ad67589585ae9f/vim/lua/lsp/init.lua#L308-L355

[tombh]

I'd missed out an arg to the nvim --headless call, and Vim defaults to notify the user in its UI not stdout, hence the stuck builds. So I fixed it and added a 5 minute timeout, and made Vim a bit more verbose.

Also added 2 more basic tests for toggling, enable/disable.

I declare this PR ready then. I'll start another PR to explore #42.

PS. Thanks for your dotfile tips, I've got null-ls, so I added them to that. I see you use sumneko too, how do you manage competing diagnostics from it and luacheck?

[lukas-reineke]

how do you manage competing diagnostics from it and luacheck

I just ignore that TBH. You could ignore specific errors in luacheck that sumneko covers, but because CI only runs luacheck I want to have the same errors locally as well.

[tombh]

🚀