[Lldb-commits] [PATCH] D35311: lldb-server tests: Add support for testing debugserver

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D35311#830779, @beanz wrote:

> This all looks fine to me. The one thing I'd like you to consider is that someday (when lldb-server is supported on Darwin) we will want to run these tests against both debugserver and lldb-server. I'm not asking you to make those changes to this patch, but if there are things you would change about how you're doing things to make that easier to support I'd ask that you consider that.
>
> I have one specific comment below that is along those lines. Otherwise, this LGTM.


I did consider this to some depth while making this change. I think that the easiest way to make that happen would be to create two cmake targets (`add_lldb_unittest(LLDBServerTests...`, `add_lldb_unittest(DebugServerTests...`). The main difference between these would be the value of the LLDB_SERVER define, but other differences are possible as well (for example, if only one of the servers implements some feature). It means we will compile some things twice, but in return it will integrate nicely with the existing llvm unit testing infrastructure.

It may be possible to use the googletest's parameterized test mechanism for this, but I don't know that well enough to tell. In any case, this shouldn't interfere with this patch -- the only thing that will change is that the return value of isdebugserver() will be determined through some runtime mechanism.



================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:32
+bool TestClient::IsDebugServer() {
+#ifdef __APPLE__
+  return true;
----------------
beanz wrote:
> Instead of making this ifdef'd can you just check to see if `LLDB_SERVER` is debugserver or lldb-server?
> 
> One of the items on my todo list is to get rid of all the places where `#ifdef __APPLE__` equates to using debugserver so that we can start evaluating making lldb-server work on Darwin.
I'll change this to detect the server type based on the file name.


https://reviews.llvm.org/D35311