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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.


================
Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:64
   *Serialized->mutable_location()->mutable_file_path() = std::string();
   Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized);
   EXPECT_FALSE(Deserialized);
----------------
kadircet wrote:
> nit: can be directly asserted
> 
> 
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!


================
Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:159
   Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized);
   EXPECT_TRUE(Deserialized);
 
----------------
kadircet wrote:
> maybe make this an assert too (and also consider issuing ASSERT_FALSE directly on the expression instead of an extra assingment)
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84535