[PATCH] D52554: [WIP] [clangd] Tests for special methods code-completion

[Jan Korous via Phabricator via cfe-commits]

jkorous marked 6 inline comments as done.
jkorous added inline comments.


================
Comment at: clangd/CodeCompleteTests.cpp:2043
 
+TEST(CompletionTestNoExplicitMembers, Struct) {
+  clangd::CodeCompleteOptions Opts;
----------------
sammccall wrote:
> sammccall wrote:
> > (Should this be ImplicitMembers?)
> nit: these cases should probably be moved up with the other code completion ones, and called something like `TEST(CompletionTest, NoExplicitMembers)` or so for consistency.
> 
> It's OK to have related tests in one test case.
> 
> In fact, this large set of closely-related cases seems like a good place for Go-style table-driven tests.
Ultimately got rid of the name completely.


================
Comment at: clangd/CodeCompleteTests.cpp:2043
 
+TEST(CompletionTestNoExplicitMembers, Struct) {
+  clangd::CodeCompleteOptions Opts;
----------------
jkorous wrote:
> sammccall wrote:
> > sammccall wrote:
> > > (Should this be ImplicitMembers?)
> > nit: these cases should probably be moved up with the other code completion ones, and called something like `TEST(CompletionTest, NoExplicitMembers)` or so for consistency.
> > 
> > It's OK to have related tests in one test case.
> > 
> > In fact, this large set of closely-related cases seems like a good place for Go-style table-driven tests.
> Ultimately got rid of the name completely.
I reconsidered the simple yet verbatim approach.


================
Comment at: clangd/CodeCompleteTests.cpp:2054
+
+  EXPECT_TRUE(ResultsDot.Completions.empty());
+
----------------
sammccall wrote:
> EXPECT_THAT(ResultsDot.Completions, IsEmpty())
> 
> (when the assertion fails, the failure message will print the contents of the container)
I didn't know this. It's pretty neat. Thanks!


================
Comment at: clangd/CodeCompleteTests.cpp:2139
+
+TEST(CompletionTestMethodDeclared, Struct) {
+  clangd::CodeCompleteOptions Opts;
----------------
sammccall wrote:
> doesn't this test a strict superset of what CompletionTestNoTestMembers tests?
> i.e. this also asserts that the implicit members are not included.
> 
> ISTM we could combine many of these tests (and that in fact many of them are covered by TestAfterDotCompletion.
You are right, I pruned the list of testcases a bit.


================
Comment at: clangd/CodeCompleteTests.cpp:2338
+
+TEST(CompletionTestSpecialMethodsDeclared, ExplicitStructTemplateSpecialization) {
+  clangd::CodeCompleteOptions Opts;
----------------
sammccall wrote:
> (I think we could get away with a representative set of cases, rather than testing the intersection of every feature. e.g. test an explicitly declared operator= on a struct, but combining that with an explicitly specialized struct template seems unneccesary)
I simplified the whole test and removed some cases that were not really unique. Do you think the rest is still too exhaustive? (Asking before deleting.)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52554