[PATCH] D137693: [NFC] [C++20] [Modules] [clangd] Add test for code completion for C++20 Named Modules

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

I think at this point we specifically don't want to commit to preserving particular modules behavior, this is more or less what the unsupported state means (https://github.com/clangd/clangd/issues/1293).

In particular the pattern where modules are built externally by clang, clang must be version-locked to clangd, and module content is never indexed, doesn't seem to be a good foundation for most users, so having the freedom to tear it down later seems better than encouraging people to rely on it.



================
Comment at: clang-tools-extra/clangd/test/CMakeLists.txt:2
 set(CLANGD_TEST_DEPS
+  clang
   clangd
----------------
A dependency on clang here probably isn't acceptable:
 - it's a *large* increase in what needs to be built for `check-clangd`, which we try quite hard to keep small
 - the conceptual reason is that clangd is consuming modules built by (version-locked) clang, for various reasons  this shouldn't be required (not least: users likely don't have a version-locked clang installed)


================
Comment at: clang-tools-extra/clangd/test/completion-modules.test:1
+# Tests that the importee can find the exported entities.
+# RUN: rm -fr %t
----------------
We generally find lit tests too hard to maintain for this sort of thing.

In particular:
- lots of noise added to each test
- failures are too hard to parse
- too much redundant setup work duplicated in each testcase
- substituting filenames tends to run into escaping issues with windows and backslashes, etc
- test scope ends up being too large
- use of real filesystem doesn't verify the implementation is VFS-clean

It's usually OK to have a single smoke test if needed, but generally these are written as unittests.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137693/new/

https://reviews.llvm.org/D137693