simark added inline comments.
================ Comment at: clangd/ClangdLSPServer.cpp:302 +// FIXME: This function needs to be properly tested. +void ClangdLSPServer::onChangeConfiguration( ---------------- ilya-biryukov wrote: > 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. > 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. What do you mean by "we'll have to match the json input/output directly"? That we'll have to match the complete JSON output textually? Couldn't the test parse the JSON into some data structures, then we could assert specific things, like that this particular field is present and contains a certain substring, for example? > 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. Ok, I see the complication with the Content-Length. I am not familiar with lit yet, so I don't know what it is capable of. But being able to craft and send arbitrary LSP messages would certainly be helpful in the future for all kinds of black box test, so having a framework that allows to do this would be helpful, I think. I'm not familiar enough with the ecosystem to do this right now, but I'll keep it in mind. One question about this particular test. Would there be some race condition here? If we do: 1. Start clangd with compile_commands.json #1 2. Ask for the definition of a function, expecting a result 3. Change the configuration to compile_commands.json #2 4. Ask for the definition of the same function, expecting a different result Since clangd is multi-threaded and does work in the background, are we sure that we'll get the result we want in #4? > 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. I agree. For the time being, is it fine to leave the FIXME there? We can work on improving the test frameworks to get rid of it. 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
