Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test the lsp interaction #210

Merged
merged 1 commit into from
Jun 25, 2024
Merged

Test the lsp interaction #210

merged 1 commit into from
Jun 25, 2024

Conversation

thufschmitt
Copy link
Member

@thufschmitt thufschmitt commented Jun 19, 2024

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 thufschmitt changed the base branch from main to modules June 19, 2024 11:36
@thufschmitt thufschmitt changed the base branch from modules to transparent-regen June 19, 2024 11:36
@thufschmitt thufschmitt force-pushed the test-the-lsp branch 2 times, most recently from 483433a to 86b627b Compare June 21, 2024 04:42
Copy link
Contributor

@yannham yannham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

I don't know how annoying that would be to use that here. Maybe pulling the python toolchain is much easier than the Rust toolchain. 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. Or maybe we can reuse the Nickel LSP testing infrastructure.

tests/lsp/test_hover.py Outdated Show resolved Hide resolved
@thufschmitt
Copy link
Member Author

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)

Ensure that autocompletion works fine for simple things
@yannham
Copy link
Contributor

yannham commented Jun 21, 2024

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
Copy link
Member Author

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 thufschmitt merged commit 43bce85 into transparent-regen Jun 25, 2024
1 check passed
@thufschmitt thufschmitt deleted the test-the-lsp branch June 25, 2024 08:56
@thufschmitt
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants