[clangd] Switch from EXPECT_TRUE to ASSERT_TRUE in remote marshalling tests

[kbobyrev]

Summary

When dereferencing Optional's it makes sense to use ASSERT_TRUE for better
test failures readability. Switch from EXPECT_TRUE to ASSERT_TRUE where
it is appropriate.

[kadircet]

thanks LGTM!

clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp

63–64

nit: can be directly asserted

68–69

nit: can be directly asserted

76–77

nit: can be directly asserted

83–84

nit: again can be directly asserted

155

maybe make this an assert too (and also consider issuing ASSERT_FALSE directly on the expression instead of an extra assingment)

160

same as the previous one

165

same as the previous one

185–186

nit: maybe directly issue EXPECT_FALSE(WrongMarshaller.toProtobuf(Sym)) instead of going through an extra variable.

267–268

this should be done before the EXPECT_EQ above (which accesses headers_size())

[kbobyrev]

Address most comments.

clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp

63–64

Do you mean "directly checked (whether via EXPECTED or ASSERT, I guess EXPECTED should be still better here, right?)" here and elsewhere? If so, done!

155

Why is would asserting be better here? If I'm not dereferencing it I guess this should be worse because only the first assertion failure is checked, right?

[kbobyrev]

Find one more case when checking directly makes sense.

[kbobyrev]

clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp

155

Should be OK as discussed in PM.

clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp

Marshaller ProtobufMarshaller(
       testPath("remote/machine/projects/llvm-project/"),
       testPath("home/my-projects/llvm-project/"));
   clangd::Ref Original;
   Original.Location.FileURI =
       testPathURI("remote/machine/projects/llvm-project/clang-tools-extra/"
                   "clangd/unittests/remote/MarshallingTests.cpp",
                   Strings);
   auto Serialized = ProtobufMarshaller.toProtobuf(Original);
-  EXPECT_TRUE(Serialized);
+  ASSERT_TRUE(Serialized);
   EXPECT_EQ(Serialized->location().file_path(),
             "clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp");
   auto Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized);
-  EXPECT_TRUE(Deserialized);
+  ASSERT_TRUE(Deserialized);
   EXPECT_STREQ(Deserialized->Location.FileURI,
                testPathURI("home/my-projects/llvm-project/clang-tools-extra/"
                            "clangd/unittests/remote/MarshallingTests.cpp",
                            Strings));
 
   // Can't have empty paths.
   *Serialized->mutable_location()->mutable_file_path() = std::string();
-  Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized);
-  EXPECT_FALSE(Deserialized);
+  EXPECT_FALSE(ProtobufMarshaller.fromProtobuf(*Serialized));
 
   clangd::Ref WithInvalidURI;
   // Invalid URI results in serialization failure.
   WithInvalidURI.Location.FileURI = "This is not a URI";
-  Serialized = ProtobufMarshaller.toProtobuf(WithInvalidURI);
-  EXPECT_FALSE(Serialized);
+  EXPECT_FALSE(ProtobufMarshaller.toProtobuf(WithInvalidURI));
 
   // Can not use URIs with scheme different from "file".
   auto UnittestURI =
       URI::create(testPath("project/lib/HelloWorld.cpp"), "unittest");
-  EXPECT_TRUE(bool(UnittestURI));
+  ASSERT_TRUE(bool(UnittestURI));
   WithInvalidURI.Location.FileURI =
       Strings.save(UnittestURI->toString()).begin();
-  Serialized = ProtobufMarshaller.toProtobuf(WithInvalidURI);
-  EXPECT_FALSE(Serialized);
+  EXPECT_FALSE(ProtobufMarshaller.toProtobuf(WithInvalidURI));
 
   // Paths transmitted over the wire can not be absolute, they have to be
   // relative.
   Ref WithAbsolutePath;
   *WithAbsolutePath.mutable_location()->mutable_file_path() =
       "/usr/local/user/home/HelloWorld.cpp";
-  Deserialized = ProtobufMarshaller.fromProtobuf(WithAbsolutePath);
-  EXPECT_FALSE(Deserialized);
+  EXPECT_FALSE(ProtobufMarshaller.fromProtobuf(WithAbsolutePath));
 }
 
 TEST(RemoteMarshallingTest, SymbolSerialization) {
   clangd::Symbol Sym;
 
   auto ID = SymbolID::fromStr("057557CEBF6E6B2D");
-  EXPECT_TRUE(bool(ID));
+  ASSERT_TRUE(bool(ID));
   Sym.ID = *ID;
 
   index::SymbolInfo Info;
   Info.Kind = index::SymbolKind::Function;
   Info.SubKind = index::SymbolSubKind::AccessorGetter;
   Info.Lang = index::SymbolLanguage::CXX;
   Info.Properties = static_cast<index::SymbolPropertySet>(
       index::SymbolProperty::TemplateSpecialization);
TEST(RemoteMarshallingTest, SymbolSerialization) {
 
   Sym.Flags = clangd::Symbol::SymbolFlag::IndexedForCodeCompletion;
 
   Marshaller ProtobufMarshaller(testPath("home/"), testPath("home/"));
 
   // Check that symbols are exactly the same if the path to indexed project is
   // the same on indexing machine and the client.
   auto Serialized = ProtobufMarshaller.toProtobuf(Sym);
-  EXPECT_TRUE(Serialized);
+  ASSERT_TRUE(Serialized);
   auto Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized);
-  EXPECT_TRUE(Deserialized);
+  ASSERT_TRUE(Deserialized);
   EXPECT_EQ(toYAML(Sym), toYAML(*Deserialized));
   // Serialized paths are relative and have UNIX slashes.
   EXPECT_EQ(convert_to_slash(Serialized->definition().file_path(),
                              llvm::sys::path::Style::posix),
             Serialized->definition().file_path());
   EXPECT_TRUE(
       llvm::sys::path::is_relative(Serialized->definition().file_path()));
 
   // Missing definition is OK.
   Sym.Definition = clangd::SymbolLocation();
   Serialized = ProtobufMarshaller.toProtobuf(Sym);
-  EXPECT_TRUE(Serialized);
+  ASSERT_TRUE(Serialized);
   Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized);
   EXPECT_TRUE(Deserialized);
 
   // Relative path is absolute.
   *Serialized->mutable_canonical_declaration()->mutable_file_path() =
       convert_to_slash("/path/to/Declaration.h");
-  Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized);
-  EXPECT_FALSE(Deserialized);
+  EXPECT_FALSE(ProtobufMarshaller.fromProtobuf(*Serialized));
 
   // Fail with an invalid URI.
   Location.FileURI = "Not A URI";
   Sym.Definition = Location;
-  Serialized = ProtobufMarshaller.toProtobuf(Sym);
-  EXPECT_FALSE(Serialized);
+  EXPECT_FALSE(ProtobufMarshaller.toProtobuf(Sym));
 
   // Schemes other than "file" can not be used.
   auto UnittestURI = URI::create(testPath("home/SomePath.h"), "unittest");
-  EXPECT_TRUE(bool(UnittestURI));
+  ASSERT_TRUE(bool(UnittestURI));
   Location.FileURI = Strings.save(UnittestURI->toString()).begin();
   Sym.Definition = Location;
-  Serialized = ProtobufMarshaller.toProtobuf(Sym);
-  EXPECT_FALSE(Serialized);
+  EXPECT_FALSE(ProtobufMarshaller.toProtobuf(Sym));
 
   // Passing root that is not prefix of the original file path.
   Location.FileURI = testPathURI("home/File.h", Strings);
   Sym.Definition = Location;
   // Check that the symbol is valid and passing the correct path works.
   Serialized = ProtobufMarshaller.toProtobuf(Sym);
-  EXPECT_TRUE(Serialized);
+  ASSERT_TRUE(Serialized);
   Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized);
-  EXPECT_TRUE(Deserialized);
+  ASSERT_TRUE(Deserialized);
   EXPECT_STREQ(Deserialized->Definition.FileURI,
                testPathURI("home/File.h", Strings));
   // Fail with a wrong root.
   Marshaller WrongMarshaller(testPath("nothome/"), testPath("home/"));
-  Serialized = WrongMarshaller.toProtobuf(Sym);
-  EXPECT_FALSE(Serialized);
+  EXPECT_FALSE(WrongMarshaller.toProtobuf(Sym));
 }
 
 TEST(RemoteMarshallingTest, RefSerialization) {
   clangd::Ref Ref;
   Ref.Kind = clangd::RefKind::Spelled | clangd::RefKind::Declaration;
 
   llvm::BumpPtrAllocator Arena;
   llvm::UniqueStringSaver Strings(Arena);
 
   clangd::SymbolLocation Location;
   Location.Start.setLine(124);
   Location.Start.setColumn(21);
   Location.End.setLine(3213);
   Location.End.setColumn(541);
   Location.FileURI = testPathURI(
       "llvm-project/llvm/clang-tools-extra/clangd/Protocol.h", Strings);
   Ref.Location = Location;
 
   Marshaller ProtobufMarshaller(testPath("llvm-project/"),
                                 testPath("llvm-project/"));
 
   auto Serialized = ProtobufMarshaller.toProtobuf(Ref);
-  EXPECT_TRUE(Serialized);
+  ASSERT_TRUE(Serialized);
   auto Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized);
-  EXPECT_TRUE(Deserialized);
+  ASSERT_TRUE(Deserialized);
   EXPECT_EQ(toYAML(Ref), toYAML(*Deserialized));
 }
 
 TEST(RemoteMarshallingTest, IncludeHeaderURIs) {
   llvm::BumpPtrAllocator Arena;
   llvm::UniqueStringSaver Strings(Arena);
 
   llvm::SmallVector<clangd::Symbol::IncludeHeaderWithReferences, 1>
TEST(RemoteMarshallingTest, IncludeHeaderURIs) {
   auto AllHeaders = ValidHeaders;
   AllHeaders.insert(AllHeaders.end(), InvalidHeaders.begin(),
                     InvalidHeaders.end());
   Sym.IncludeHeaders = AllHeaders;
 
   Marshaller ProtobufMarshaller(convert_to_slash("/"), convert_to_slash("/"));
 
   auto Serialized = ProtobufMarshaller.toProtobuf(Sym);
+  ASSERT_TRUE(Serialized);
   EXPECT_EQ(static_cast<size_t>(Serialized->headers_size()),
             ValidHeaders.size());
-  EXPECT_TRUE(Serialized);
   auto Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized);
-  EXPECT_TRUE(Deserialized);
+  ASSERT_TRUE(Deserialized);
 
   Sym.IncludeHeaders = ValidHeaders;
   EXPECT_EQ(toYAML(Sym), toYAML(*Deserialized));
 }
 
 TEST(RemoteMarshallingTest, FuzzyFindRequestSerialization) {
   clangd::FuzzyFindRequest Request;
   Request.ProximityPaths = {testPath("local/Header.h"),