[Lldb-commits] [PATCH] D32930: New framework for lldb client-server communication tests.

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:189
+    if (key.size() >= 9 && key[0] == 'T' && key.substr(3, 6) == "thread") {
+      val.getAsInteger(16, stop_reply->thread);
+      key.substr(1, 2).getAsInteger(16, stop_reply->signal);
----------------
jmajors wrote:
> labath wrote:
> > zturner wrote:
> > > Is error checking needed here?
> > More error checking definitely needed (here and everywhere else). If I break lldb-server while working on it, I'd like to see a clear error message from the test rather than an obscure crash. This should return `Expected<StopReply>` if you don't care about copying or `Expected<std::unique_ptr<StopReply>>` if you do (*), so that the test can ASSERT that the message was received and parsed correctly and print out the error otherwise.
> > 
> > (*) Or the out argument idea I wrote about earlier.
> I converted my pointer containers to Expected<unique_ptr<Type>>. 
> I noticed that ASSERT_* doesn't fail the test, it returns, so I need to make the functions in TestClient return Error or Expected<> objects and check them in the test body.
You'll want to use something like I've got in D33059 for checking the values of `Expected<T>`s and `Error`s.  That's pending code review though.  You can put the logic in the body as you suggested, but it's really cumbersome.


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:26-27
+Expected<std::unique_ptr<ProcessInfo>> ProcessInfo::Create(StringRef response) {
+  std::unique_ptr<ProcessInfo> process_info =
+    std::unique_ptr<ProcessInfo>(new ProcessInfo);
+  auto elements = SplitPairList(response);
----------------
How about `std::unique_ptr<ProcessInfo> process_info(new ProcessInfo);`


https://reviews.llvm.org/D32930