This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by kbobyrev on Jul 24 2020, 9:01 AM.

Details

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.

Diff Detail

Event Timeline

kbobyrev created this revision.Jul 24 2020, 9:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 24 2020, 9:01 AM
kadircet accepted this revision.Jul 26 2020, 4:13 AM

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())

This revision is now accepted and ready to land.Jul 26 2020, 4:13 AM
kbobyrev updated this revision to Diff 280743.Jul 26 2020, 11:53 AM
kbobyrev marked 7 inline comments as done.

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 updated this revision to Diff 280745.Jul 26 2020, 11:57 AM

Find one more case when checking directly makes sense.

kbobyrev marked 5 inline comments as done.Jul 27 2020, 1:43 AM
kbobyrev added inline comments.
clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp
155

Should be OK as discussed in PM.

This revision was automatically updated to reflect the committed changes.
kbobyrev marked an inline comment as done.