search

nickel-lang / organist

Control all your tooling from a single console
MIT License
361 stars 20 forks source link

Test the lsp interaction #210

Closed thufschmitt closed 6 days ago

thufschmitt commented 1 week ago

Having a good interaction with the LSP is crucial for Organist (it's kind-of its reason d'être), and we're sometimes pushing it to its limits, meaning that it's easy to break it every once in a while.

This PR add some tests to ensure that the most important interactions work properly.

Depends on #203

thufschmitt commented 1 week ago

I don't want to block this PR on some yakk shaving, but just for the record, we use a combo of a basic programmatic interaction with the LSP + snapshot testing to the same effect in Nickel (https://github.com/tweag/nickel/tree/master/lsp/lsp-harness). The actual tests are here.

Yes, I saw that. I didn't want to reuse it because it felt very internal and currently doesn't work out-of-the-box outside the Nickel tree (because of that). But making it available independently of Nickel and reusing it here could be a great follow-up.

Maybe some tests can also just be moved to core Nickel directly - the more we test the LSP at the source of the development, the better

These tests are very Organist-specific, so I'm not sure that you would want them in Nickel (but it's indeed very helpful for finding bugs or potential improvements in the LSP)

yannham commented 1 week ago

These tests are very Organist-specific, so I'm not sure that you would want them in Nickel (but it's indeed very helpful for finding bugs or potential improvements in the LSP)

I mean, as long as it's existing Nickel code for which users (whether organist users or not) expect the LSP to behave in a certain way, those are very good LSP tests, IMHO. My personal criteria would rather be practical: is it hard to separate from organist or not (for example, you might want to remain up-to-date with the latest organist definitions, which might be more annoying to do if the tests are happening in the Nickel repo).

thufschmitt commented 1 week ago

From a practical perspective, these are pulling in the whole of Organist, to they couldn't be embedded in the Nickel test suite. But with a small bit of work, that could be an extra check in the Nickel CI – à la stackage HEAD or equivalent

thufschmitt commented 6 days ago

Argh, I forgot that this wasn't targetting master. We need dpulls or something equivalent there :angry: