[Lldb-commits] [PATCH] D32585: Implementation of remote packets for Trace data.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D32585#752115, @ravitheja wrote:

> In https://reviews.llvm.org/D32585#740632, @labath wrote:
>
> > I quite like that you have added just the packet plumbing code without an concrete implementation. However, that is still a significant amount of parsing code that should be accompanied by a test. The test suite for the client side of the protocol is ready (TestGdbRemoteCommunicationClient), so I'd like to see at least that.
>
>
> @labath  I was considering writing Unit tests for the remote packets but I thought then I have to write the mock implementation for the trace operations as well, which might end up being a bigger piece of code than the actual packet parsing code.


I don't think that is the case. If you look at the tests in TestGdbRemoteCommunicationClient.cpp, all of them are quite simple. E.g. a test for TestGdbRemoteCommunicationClient could be something like:

  TraceOptions opt;
  opt.setType(...);
  opt.setBufferSize(...);
  opt.setMetaBufferSize(); // with an appropriate TraceOptions constructor, this could be a one-liner
    std::future<user_id_t> result = std::async(std::launch::async, [&] {
      return client.StartTrace(opt, error);
    });
  HandlePacket(server, "JTrace:start:type:dead:buffersize:beef:metabuffersize:baad:threadid:f00d:jparams:...", "47");
  ASSERT_EQ(47, result.get());
  ASSERT_TRUE(error.Success());

This depends on the packet code being in GdbRemoteCommunicationClient and not ProcessGdbRemote as it is now, but moving it there is a good idea anyway -- ProcessGdbRemote should do higher level stuff, packet parsing should be left in the GDBRCClient.

The server side code unfortunately cannot be tested this way, but I believe I will get around to enabling that soon as well.

> After this patch, I will upload the actual server side code doing the trace stuff for linux, that part of code has unit tests for some core functions.

The problem with that test vector is that the test will only run on linux, and only on processors that support the feature, which means there's a very high chance it will end up being dead code (my guesstimate is our bot does not have the required hardware feature). What is also quite difficult to test with that approach is the failure cases (what happens if the server sends a malformed packed or the response misses some data, etc.).



================
Comment at: include/lldb/Utility/StringExtractor.h:114
 
+  bool Consume_front(const llvm::StringRef &str);
+
----------------
The name looks wrong. Either use `CamelCase` or `snake_case`, don't try to mix the two. (I'd propose camel case).


https://reviews.llvm.org/D32585