labath created this revision.
Herald added a subscriber: mgorny.

Adding a new test would require one to duplicate a significant part of
the existing test that we have. This attempts to reduce that by moving
some part of that code to the test fixture. The StandardStartupTest
fixture automatically starts up the server and connects it to the
client. I also add a more low-level TestBase fixture, which allows one
to start up the client and server in a custom way (I am going to need
this for the test I am writing).


https://reviews.llvm.org/D41066

Files:
  unittests/tools/lldb-server/CMakeLists.txt
  unittests/tools/lldb-server/tests/CMakeLists.txt
  unittests/tools/lldb-server/tests/MessageObjects.h
  unittests/tools/lldb-server/tests/TestBase.cpp
  unittests/tools/lldb-server/tests/TestBase.h
  unittests/tools/lldb-server/tests/TestClient.cpp
  unittests/tools/lldb-server/tests/TestClient.h
  unittests/tools/lldb-server/tests/ThreadIdsInJstopinfoTest.cpp

Index: unittests/tools/lldb-server/tests/ThreadIdsInJstopinfoTest.cpp
===================================================================
--- unittests/tools/lldb-server/tests/ThreadIdsInJstopinfoTest.cpp
+++ unittests/tools/lldb-server/tests/ThreadIdsInJstopinfoTest.cpp
@@ -7,38 +7,31 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "TestBase.h"
 #include "TestClient.h"
+#include "llvm/Support/Path.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
 #include <string>
 
 using namespace llgs_tests;
+using namespace llvm;
 
-class ThreadsInJstopinfoTest : public ::testing::Test {
-protected:
-  virtual void SetUp() { TestClient::Initialize(); }
-};
+TEST_F(StandardStartupTest, TestStopReplyContainsThreadPcs) {
+  // This inferior spawns 4 threads, then forces a break.
+  ASSERT_THAT_ERROR(
+      Client->SetInferior({getInferiorPath("thread_inferior"), "4"}),
+      Succeeded());
 
-TEST_F(ThreadsInJstopinfoTest, TestStopReplyContainsThreadPcsLlgs) {
-  std::vector<std::string> inferior_args;
-  // This inferior spawns N threads, then forces a break.
-  inferior_args.push_back(THREAD_INFERIOR);
-  inferior_args.push_back("4");
-
-  auto test_info = ::testing::UnitTest::GetInstance()->current_test_info();
-
-  TestClient client(test_info->name(), test_info->test_case_name());
-  ASSERT_THAT_ERROR(client.StartDebugger(), llvm::Succeeded());
-  ASSERT_THAT_ERROR(client.SetInferior(inferior_args), llvm::Succeeded());
-  ASSERT_THAT_ERROR(client.ListThreadsInStopReply(), llvm::Succeeded());
-  ASSERT_THAT_ERROR(client.ContinueAll(), llvm::Succeeded());
-  unsigned int pc_reg = client.GetPcRegisterId();
+  ASSERT_THAT_ERROR(Client->ListThreadsInStopReply(), Succeeded());
+  ASSERT_THAT_ERROR(Client->ContinueAll(), Succeeded());
+  unsigned int pc_reg = Client->GetPcRegisterId();
   ASSERT_NE(pc_reg, UINT_MAX);
 
-  auto jthreads_info = client.GetJThreadsInfo();
+  auto jthreads_info = Client->GetJThreadsInfo();
   ASSERT_TRUE(jthreads_info);
 
-  auto stop_reply = client.GetLatestStopReply();
+  auto stop_reply = Client->GetLatestStopReply();
   auto stop_reply_pcs = stop_reply.GetThreadPcs();
   auto thread_infos = jthreads_info->GetThreadInfos();
   ASSERT_EQ(stop_reply_pcs.size(), thread_infos.size())
@@ -49,10 +42,8 @@
     ASSERT_TRUE(thread_infos.find(tid) != thread_infos.end())
         << "Thread ID: " << tid << " not in JThreadsInfo.";
     auto pc_value = thread_infos[tid].ReadRegisterAsUint64(pc_reg);
-    ASSERT_THAT_EXPECTED(pc_value, llvm::Succeeded());
+    ASSERT_THAT_EXPECTED(pc_value, Succeeded());
     ASSERT_EQ(stop_reply_pcs[tid], *pc_value)
         << "Mismatched PC for thread: " << tid;
   }
-
-  ASSERT_THAT_ERROR(client.StopDebugger(), llvm::Succeeded());
 }
Index: unittests/tools/lldb-server/tests/TestClient.h
===================================================================
--- unittests/tools/lldb-server/tests/TestClient.h
+++ unittests/tools/lldb-server/tests/TestClient.h
@@ -7,28 +7,35 @@
 //
 //===----------------------------------------------------------------------===//
 
+#ifndef LLDB_SERVER_TESTS_TESTCLIENT_H
+#define LLDB_SERVER_TESTS_TESTCLIENT_H
+
 #include "MessageObjects.h"
 #include "Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h"
 #include "lldb/Target/ProcessLaunchInfo.h"
 #include "lldb/Utility/ArchSpec.h"
+#include "lldb/Utility/Connection.h"
 #include "llvm/ADT/Optional.h"
 #include <memory>
 #include <string>
 
 namespace llgs_tests {
-// TODO: Make the test client an abstract base class, with different children
-// for different types of connections: llgs v. debugserver
 class TestClient
     : public lldb_private::process_gdb_remote::GDBRemoteCommunicationClient {
 public:
-  static void Initialize();
   static bool IsDebugServer();
   static bool IsLldbServer();
 
-  TestClient(const std::string &test_name, const std::string &test_case_name);
-  virtual ~TestClient();
-  llvm::Error StartDebugger();
-  llvm::Error StopDebugger();
+  /// Launches the server, connects it to the client and returns the client. If
+  /// Log is non-empty, the server will write it's log to this file.
+  static llvm::Expected<std::unique_ptr<TestClient>> launch(llvm::StringRef Log);
+
+  /// Launches the server, while specifying the inferior on its command line.
+  /// When the client connects, it already has a process ready.
+  static llvm::Expected<std::unique_ptr<TestClient>>
+  launch(llvm::StringRef Log, llvm::ArrayRef<llvm::StringRef> InferiorArgs);
+
+  ~TestClient() override;
   llvm::Error SetInferior(llvm::ArrayRef<std::string> inferior_args);
   llvm::Error ListThreadsInStopReply();
   llvm::Error SetBreakpoint(unsigned long address);
@@ -45,18 +52,20 @@
   unsigned int GetPcRegisterId();
 
 private:
+  TestClient(std::unique_ptr<lldb_private::Connection> Conn);
+
+  llvm::Error QueryProcessInfo();
   llvm::Error Continue(llvm::StringRef message);
-  std::string GenerateLogFileName(const lldb_private::ArchSpec &arch) const;
   std::string FormatFailedResult(
       const std::string &message,
       lldb_private::process_gdb_remote::GDBRemoteCommunication::PacketResult
           result);
 
   llvm::Optional<ProcessInfo> m_process_info;
   llvm::Optional<StopReply> m_stop_reply;
-  lldb_private::ProcessLaunchInfo m_server_process_info;
-  std::string m_test_name;
-  std::string m_test_case_name;
-  unsigned int m_pc_register;
+  unsigned int m_pc_register = UINT_MAX;
 };
+
 } // namespace llgs_tests
+
+#endif // LLDB_SERVER_TESTS_TESTCLIENT_H
Index: unittests/tools/lldb-server/tests/TestClient.cpp
===================================================================
--- unittests/tools/lldb-server/tests/TestClient.cpp
+++ unittests/tools/lldb-server/tests/TestClient.cpp
@@ -15,6 +15,7 @@
 #include "lldb/Target/ProcessLaunchInfo.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
 #include <cstdlib>
 #include <future>
@@ -26,72 +27,73 @@
 using namespace llvm;
 
 namespace llgs_tests {
-void TestClient::Initialize() { HostInfo::Initialize(); }
-
 bool TestClient::IsDebugServer() {
   return sys::path::filename(LLDB_SERVER).contains("debugserver");
 }
 
 bool TestClient::IsLldbServer() { return !IsDebugServer(); }
 
-TestClient::TestClient(const std::string &test_name,
-                       const std::string &test_case_name)
-    : m_test_name(test_name), m_test_case_name(test_case_name),
-      m_pc_register(UINT_MAX) {}
+TestClient::TestClient(std::unique_ptr<Connection> Conn) {
+  SetConnection(Conn.release());
+
+  SendAck(); // Send this as a handshake.
+}
+
+TestClient::~TestClient() {
+  std::string response;
+  // Debugserver (non-conformingly?) sends a reply to the k packet instead of
+  // simply closing the connection.
+  PacketResult result =
+      IsDebugServer() ? PacketResult::Success : PacketResult::ErrorDisconnected;
+  EXPECT_THAT_ERROR(SendMessage("k", response, result), Succeeded());
+}
 
-TestClient::~TestClient() {}
+Expected<std::unique_ptr<TestClient>> TestClient::launch(StringRef Log) {
+  return launch(Log, {});
+}
 
-llvm::Error TestClient::StartDebugger() {
+Expected<std::unique_ptr<TestClient>> TestClient::launch(StringRef Log, ArrayRef<StringRef> InferiorArgs) {
   const ArchSpec &arch_spec = HostInfo::GetArchitecture();
   Args args;
   args.AppendArgument(LLDB_SERVER);
-  if (IsLldbServer()) {
+  if (IsLldbServer())
     args.AppendArgument("gdbserver");
-    args.AppendArgument("--log-channels=gdb-remote packets");
-  } else {
-    args.AppendArgument("--log-flags=0x800000");
-  }
   args.AppendArgument("--reverse-connect");
-  std::string log_file_name = GenerateLogFileName(arch_spec);
-  if (log_file_name.size())
-    args.AppendArgument("--log-file=" + log_file_name);
+
+  if (!Log.empty()) {
+    args.AppendArgument(("--log-file=" + Log).str());
+    if (IsLldbServer())
+      args.AppendArgument("--log-channels=gdb-remote packets");
+    else
+      args.AppendArgument("--log-flags=0x800000");
+  }
 
   Status status;
   TCPSocket listen_socket(true, false);
   status = listen_socket.Listen("127.0.0.1:0", 5);
   if (status.Fail())
     return status.ToError();
 
-  char connect_remote_address[64];
-  snprintf(connect_remote_address, sizeof(connect_remote_address),
-           "localhost:%u", listen_socket.GetLocalPortNumber());
+  args.AppendArgument(
+      ("localhost:" + Twine(listen_socket.GetLocalPortNumber())).str());
 
-  args.AppendArgument(connect_remote_address);
+  if (!InferiorArgs.empty()) {
+    args.AppendArgument("--");
+    for (StringRef arg : InferiorArgs)
+      args.AppendArgument(arg);
+  }
 
-  m_server_process_info.SetArchitecture(arch_spec);
-  m_server_process_info.SetArguments(args, true);
-  status = Host::LaunchProcess(m_server_process_info);
+  ProcessLaunchInfo Info;
+  Info.SetArchitecture(arch_spec);
+  Info.SetArguments(args, true);
+  status = Host::LaunchProcess(Info);
   if (status.Fail())
     return status.ToError();
 
-  char connect_remote_uri[64];
-  snprintf(connect_remote_uri, sizeof(connect_remote_uri), "connect://%s",
-           connect_remote_address);
   Socket *accept_socket;
   listen_socket.Accept(accept_socket);
-  SetConnection(new ConnectionFileDescriptor(accept_socket));
-
-  SendAck(); // Send this as a handshake.
-  return llvm::Error::success();
-}
-
-llvm::Error TestClient::StopDebugger() {
-  std::string response;
-  // Debugserver (non-conformingly?) sends a reply to the k packet instead of
-  // simply closing the connection.
-  PacketResult result =
-      IsDebugServer() ? PacketResult::Success : PacketResult::ErrorDisconnected;
-  return SendMessage("k", response, result);
+  auto Conn = llvm::make_unique<ConnectionFileDescriptor>(accept_socket);
+  return std::unique_ptr<TestClient>(new TestClient(std::move(Conn)));
 }
 
 Error TestClient::SetInferior(llvm::ArrayRef<std::string> inferior_args) {
@@ -117,14 +119,8 @@
     return E;
   if (Error E = SendMessage("qLaunchSuccess"))
     return E;
-  std::string response;
-  if (Error E = SendMessage("qProcessInfo", response))
+  if (Error E = QueryProcessInfo())
     return E;
-  auto create_or_error = ProcessInfo::Create(response);
-  if (auto create_error = create_or_error.takeError())
-    return create_error;
-
-  m_process_info = *create_or_error;
   return Error::success();
 }
 
@@ -223,6 +219,17 @@
   return m_pc_register;
 }
 
+llvm::Error TestClient::QueryProcessInfo() {
+  std::string response;
+  if (Error E = SendMessage("qProcessInfo", response))
+    return E;
+  auto create_or_error = ProcessInfo::Create(response);
+  if (!create_or_error)
+    return create_or_error.takeError();
+  m_process_info = *create_or_error;
+  return Error::success();
+}
+
 Error TestClient::Continue(StringRef message) {
   assert(m_process_info.hasValue());
 
@@ -237,21 +244,4 @@
   return Error::success();
 }
 
-std::string TestClient::GenerateLogFileName(const ArchSpec &arch) const {
-  char *log_directory = getenv("LOG_FILE_DIRECTORY");
-  if (!log_directory)
-    return "";
-
-  if (!llvm::sys::fs::is_directory(log_directory)) {
-    GTEST_LOG_(WARNING) << "Cannot access log directory: " << log_directory;
-    return "";
-  }
-
-  std::string log_file_name;
-  raw_string_ostream log_file(log_file_name);
-  log_file << log_directory << "/lldb-" << m_test_case_name << '-'
-           << m_test_name << '-' << arch.GetArchitectureName() << ".log";
-  return log_file.str();
-}
-
 } // namespace llgs_tests
Index: unittests/tools/lldb-server/tests/TestBase.h
===================================================================
--- /dev/null
+++ unittests/tools/lldb-server/tests/TestBase.h
@@ -0,0 +1,48 @@
+//===-- TestBase.h ----------------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_SERVER_TESTS_TESTBASE_H
+#define LLDB_SERVER_TESTS_TESTBASE_H
+
+#include "TestClient.h"
+#include "lldb/Host/HostInfo.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gtest/gtest.h"
+
+namespace llgs_tests {
+
+class TestBase: public ::testing::Test {
+public:
+  static void SetUpTestCase() { lldb_private::HostInfo::Initialize(); }
+
+  static std::string getInferiorPath(llvm::StringRef Name) {
+    llvm::SmallString<64> Path(LLDB_TEST_INFERIOR_PATH);
+    llvm::sys::path::append(Path, Name + LLDB_TEST_INFERIOR_SUFFIX);
+    return Path.str();
+  }
+
+  static std::string getLogFileName();
+};
+
+class StandardStartupTest: public TestBase {
+public:
+  void SetUp() override {
+    auto ClientOr = TestClient::launch(getLogFileName());
+    ASSERT_THAT_EXPECTED(ClientOr, llvm::Succeeded());
+    Client = std::move(*ClientOr);
+  }
+
+protected:
+  std::unique_ptr<TestClient> Client;
+};
+
+} // namespace llgs_tests
+
+#endif // LLDB_SERVER_TESTS_TESTBASE_H
Index: unittests/tools/lldb-server/tests/TestBase.cpp
===================================================================
--- /dev/null
+++ unittests/tools/lldb-server/tests/TestBase.cpp
@@ -0,0 +1,36 @@
+//===-- TestBase.cpp --------------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "TestBase.h"
+#include <cstdlib>
+
+using namespace llgs_tests;
+using namespace llvm;
+
+std::string TestBase::getLogFileName() {
+  const auto *test_info =
+      ::testing::UnitTest::GetInstance()->current_test_info();
+  assert(test_info);
+
+  const char *Dir = getenv("LOG_FILE_DIRECTORY");
+  if (!Dir)
+    return "";
+
+  if (!llvm::sys::fs::is_directory(Dir)) {
+    GTEST_LOG_(WARNING) << "Cannot access log directory: " << Dir;
+    return "";
+  }
+
+  SmallString<64> DirStr(Dir);
+  sys::path::append(DirStr, std::string("server-") +
+                                test_info->test_case_name() + "-" +
+                                test_info->name() + ".log");
+  return DirStr.str();
+}
+
Index: unittests/tools/lldb-server/tests/MessageObjects.h
===================================================================
--- unittests/tools/lldb-server/tests/MessageObjects.h
+++ unittests/tools/lldb-server/tests/MessageObjects.h
@@ -7,6 +7,9 @@
 //
 //===----------------------------------------------------------------------===//
 
+#ifndef LLDB_SERVER_TESTS_MESSAGEOBJECTS_H
+#define LLDB_SERVER_TESTS_MESSAGEOBJECTS_H
+
 #include "lldb/lldb-types.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SmallString.h"
@@ -102,4 +105,7 @@
   return llvm::make_error<llvm::StringError>(error,
                                              llvm::inconvertibleErrorCode());
 }
+
 } // namespace llgs_tests
+
+#endif // LLDB_SERVER_TESTS_MESSAGEOBJECTS_H
Index: unittests/tools/lldb-server/tests/CMakeLists.txt
===================================================================
--- unittests/tools/lldb-server/tests/CMakeLists.txt
+++ unittests/tools/lldb-server/tests/CMakeLists.txt
@@ -1,4 +1,5 @@
 add_lldb_unittest(LLDBServerTests
+  TestBase.cpp
   TestClient.cpp
   MessageObjects.cpp
   ThreadIdsInJstopinfoTest.cpp
@@ -15,3 +16,5 @@
   LINK_COMPONENTS
     Support
   )
+
+add_dependencies(LLDBServerTests lldb-server ${ALL_LLDB_TEST_EXECUTABLES})
Index: unittests/tools/lldb-server/CMakeLists.txt
===================================================================
--- unittests/tools/lldb-server/CMakeLists.txt
+++ unittests/tools/lldb-server/CMakeLists.txt
@@ -1,8 +1,12 @@
+set(ALL_LLDB_TEST_EXECUTABLES)
+
 function(add_lldb_test_executable test_name)
   set(EXCLUDE_FROM_ALL ON)
   add_llvm_executable(${test_name} NO_INSTALL_RPATH ${ARGN})
   set(outdir ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR})
   set_output_directory(${test_name} BINARY_DIR ${outdir} LIBRARY_DIR ${outdir})
+  list(APPEND ALL_LLDB_TEST_EXECUTABLES ${test_name})
+  set(ALL_LLDB_TEST_EXECUTABLES ${ALL_LLDB_TEST_EXECUTABLES} PARENT_SCOPE)
 endfunction()
 
 add_lldb_test_executable(thread_inferior inferior/thread_inferior.cpp)
@@ -13,6 +17,8 @@
   add_definitions(-DLLDB_SERVER="$<TARGET_FILE:lldb-server>")
 endif()
 
-add_definitions(-DTHREAD_INFERIOR="${CMAKE_CURRENT_BINARY_DIR}/thread_inferior")
+add_definitions(
+  -DLLDB_TEST_INFERIOR_PATH="${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}"
+  -DLLDB_TEST_INFERIOR_SUFFIX="${CMAKE_EXECUTABLE_SUFFIX}"
+  )
 add_subdirectory(tests)
-add_dependencies(LLDBServerTests thread_inferior)
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
  • [Lldb-commits] [PATCH] D4106... Pavel Labath via Phabricator via lldb-commits

Reply via email to