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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
