ilya-biryukov added inline comments.

================
Comment at: clangd/ClangdLSPServer.cpp:302
 
+// FIXME: This function needs to be properly tested.
+void ClangdLSPServer::onChangeConfiguration(
----------------
simark wrote:
> ilya-biryukov wrote:
> > Are you planning to to address this FIXME before checking the code in?
> Following what you said here:
> 
> https://reviews.llvm.org/D39571?id=124024#inline-359345
> 
> I have not really looked into what was wrong with the test, and what is 
> missing in the infrastructure to make it work.  But I assumed that the 
> situation did not change since then.  Can you enlighten me on what the 
> problem was, and what is missing?
We usually write unittests for that kind of thing, since they allow to plug an 
in-memory filesystem, but we only test `ClangdServer` (examples are in 
`unittests/clangd/ClangdTests.cpp`). `ClangdLSPServer` does not allow to plug 
in a virtual filesystem (vfs). Even if we add vfs, it's still hard to unit-test 
because we'll have to match the json input/output directly.

This leaves us with an option of a lit test that runs `clangd` directly, 
similar to tests in `test/clangd`.
The lit test would need to create a temporary directory, create proper 
`compile_commands.json` there, then send the LSP commands with the path to the 
test to clangd.
One major complication is that in LSP we have to specify the size of each 
message, but in our case the size would change depending on created temp path. 
It means we'll have to patch the test input to setup proper paths and message 
sizes.
If we choose to go down this path, 
`clang-tools-extra/test/clang-tidy/vfsoverlay.cpp` does a similar setup (create 
temp-dir, patch up some configuration files to point into the temp directory, 
etc) and could be used as a starting point.

It's not impossible to write that test, it's just a bit involved. Having a test 
would be nice, though, to ensure we don't break this method while doing other 
things. Especially given that this functionality is not used anywhere in clangd.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to