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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

The assertions themselves all look good to me.

I don't think these should be new tests, they have substantial overlap with TestAfterDotCompletion and should be incorporated there or those tests split up to reduce the duplication.
(It's hard to avoid overlap/redundancy between tests, and to do so while covering all cases in a systematic way, but it's important for maintenance: one of the reasons it's hard to find the overlapping test here is there are so many!)



================
Comment at: clangd/CodeCompleteTests.cpp:2043
 
+TEST(CompletionTestNoExplicitMembers, Struct) {
+  clangd::CodeCompleteOptions Opts;
----------------
(Should this be ImplicitMembers?)


================
Comment at: clangd/CodeCompleteTests.cpp:2043
 
+TEST(CompletionTestNoExplicitMembers, Struct) {
+  clangd::CodeCompleteOptions Opts;
----------------
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.


================
Comment at: clangd/CodeCompleteTests.cpp:2045
+  clangd::CodeCompleteOptions Opts;
+  Opts.Limit = 1;
+  auto ResultsDot = completions(R"cpp(
----------------
(not clear why the limit is needed?)


================
Comment at: clangd/CodeCompleteTests.cpp:2054
+
+  EXPECT_TRUE(ResultsDot.Completions.empty());
+
----------------
EXPECT_THAT(ResultsDot.Completions, IsEmpty())

(when the assertion fails, the failure message will print the contents of the container)


================
Comment at: clangd/CodeCompleteTests.cpp:2139
+
+TEST(CompletionTestMethodDeclared, Struct) {
+  clangd::CodeCompleteOptions Opts;
----------------
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.


================
Comment at: clangd/CodeCompleteTests.cpp:2150
+
+  ASSERT_TRUE(ResultsDot.Completions.size() == 1);
+  EXPECT_TRUE(ResultsDot.Completions.front().Name == "foomethod");
----------------
EXPECT_THAT(ResultsDot.Completions, ElementsAre(Named("foomethod")));

(As above, this is equivalent to the two assertions here but gives useful failure messages)


================
Comment at: clangd/CodeCompleteTests.cpp:2338
+
+TEST(CompletionTestSpecialMethodsDeclared, ExplicitStructTemplateSpecialization) {
+  clangd::CodeCompleteOptions Opts;
----------------
(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)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52554