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
