feat: Add clangd integration tests and documentation #29
Conversation
This commit introduces integration tests for clangd, the C/C++ language server. Key changes include: - New test suite for clangd, covering definition and diagnostics. These tests require a compile_commands.json file, which is generated by the bear tool in CI, but needs to be done manually locally. - Addition of a C/C++ (clangd) section to the main README.md, with setup instructions. - Configuration of the GitHub Actions workflow to run clangd integration tests, including installation of clang, clangd and bear. - Adjustment to the test helper `normalizePaths` to correctly handle paths from clangd output. This is neccessary because the generated compile_commands.json file is generated in the workspaces/clangd directory and contains absolute paths to the source files.
There was a problem hiding this comment.
Thank you very much for this!
Would you be willing to add tests for the rest of the tools before this is merged? If not that's totally fine.
I do want to be mindful of not letting these integration tests get out of hand. I originally wrote these as a way to help develop the tools but if we want to support more language servers it could quickly become a chore to have a comprehensive test suite for everything. What's your sense for this? I'm only bringing this up because if you want to just add one quick test for each tool for the remaining tools, that's fine by me.
| filesToOpen := []string{ | ||
| "src/main.cpp", | ||
| "src/types.cpp", | ||
| "src/helper.cpp", | ||
| "src/consumer.cpp", | ||
| "src/another_consumer.cpp", | ||
| "src/clean.cpp", | ||
| } |
There was a problem hiding this comment.
Does it work without this here? Opening all files was a hack I added for the typescript language server only but other language servers don't need to open every file, only the project
There was a problem hiding this comment.
Yeah, the tests were failing without it because clangd does not start indexing until the first time a file is opened (clangd discussion). Then it will eventually do background indexing for other files defined in compile_commands.json. I actually had a bug with how I was invoking clangd previously, which I have now fixed. We only need to open a single file for clangd to index all the files when we correctly pass it the compile-commands-dir.
Right now, the most robust way to do the tests is to open any file and wait, but I think long-term, using LSP progress notifications would be better. However, this requires larger changes to the code base. I am not an LSP expert, so I used Gemini to get a sense of how that may work:
https://g.co/gemini/share/3648aef3aa59
|
Thanks! I've added tests for hover and references. I initially planned for just one of each, but ended up adding a few more thanks to Copilot's assistance. In the process, I found a references edge case. I agree that we should be mindful of the integration test suite becoming too extensive. We don't necessarily need comprehensive tests for every language. Ideally, we could focus on tests for LSP server functionalities that might have slight variations across different servers and could lead to bugs if not handled, e.g. the original scoring issue. I also find these tests very helpful during development. As a C++ developer, having/writing C++ tests made it significantly easier for me to reproduce and address issues. Perhaps we could encourage contributors interested in specific language servers to add relevant tests (e.g., myself for C++). If I encounter a clangd issue, I'd likely submit a PR with both the fix and accompanying tests to prevent regressions. I don't anticipate a major maintenance burden from this approach. Snapshots can be recreated if the return format changes, and it should generally be obvious during that process if a regression has occurred. |
|
@Shamazo looks great, thank you! Waiting for progress notifications wouldn't be hard if it works as described, I just haven't gotten around to it yet: Let me know how this mcp server works out for you as you use it more! |
This commit introduces integration tests for clangd, the C/C++ language server.
Key changes include:
normalizePathsto correctly handle paths from clangd output. This is neccessary because the generated compile_commands.json file is generated in the workspaces/clangd directory and contains absolute paths to the source files.