[lldb-dap] Setup DAP for unit testing.

[ashgti]

This is a very simple case that currently only validates we can create a DAP instance and send a message over the transport layer. More in-depth tests will require additional helpers and possibly refactors of DAP to make it more testable, however this is some ground work to have basic support for unit tests.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: John Harrison (ashgti)

Changes

This is a very simple case that currently only validates we can create a DAP instance and send a message over the transport layer. More in-depth tests will require additional helpers and possibly refactors of DAP to make it more testable, however this is some ground work to have basic support for unit tests.

---

Full diff: https://github.com/llvm/llvm-project/pull/139937.diff

3 Files Affected:

• (modified) lldb/tools/lldb-dap/DAP.h (+2-1)
• (modified) lldb/unittests/DAP/CMakeLists.txt (+1)
• (added) lldb/unittests/DAP/DAPTest.cpp (+63)

diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h
index c2e4c2dea582e..2ff66d1cd0182 100644
--- a/lldb/tools/lldb-dap/DAP.h
+++ b/lldb/tools/lldb-dap/DAP.h
@@ -226,7 +226,8 @@ struct DAP {
   /// \param[in] default_repl_mode
   ///     Default repl mode behavior, as configured by the binary.
   /// \param[in] pre_init_commands
-  ///     LLDB commands to execute as soon as the debugger instance is allocaed.
+  ///     LLDB commands to execute as soon as the debugger instance is
+  ///     allocated.
   /// \param[in] transport
   ///     Transport for this debug session.
   DAP(Log *log, const ReplMode default_repl_mode,
diff --git a/lldb/unittests/DAP/CMakeLists.txt b/lldb/unittests/DAP/CMakeLists.txt
index 110733e93b192..6074e9b872c49 100644
--- a/lldb/unittests/DAP/CMakeLists.txt
+++ b/lldb/unittests/DAP/CMakeLists.txt
@@ -3,6 +3,7 @@ add_lldb_unittest(DAPTests
   LLDBUtilsTest.cpp
   TransportTest.cpp
   ProtocolTypesTest.cpp
+  DAPTest.cpp
 
   LINK_LIBS
     lldbDAP
diff --git a/lldb/unittests/DAP/DAPTest.cpp b/lldb/unittests/DAP/DAPTest.cpp
new file mode 100644
index 0000000000000..9d2a9b944678e
--- /dev/null
+++ b/lldb/unittests/DAP/DAPTest.cpp
@@ -0,0 +1,63 @@
+//===-- DAPTest.cpp -------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "DAP.h"
+#include "Protocol/ProtocolBase.h"
+#include "Transport.h"
+#include "lldb/Host/File.h"
+#include "lldb/Host/Pipe.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gtest/gtest.h"
+#include <chrono>
+#include <memory>
+#include <optional>
+
+using namespace llvm;
+using namespace lldb;
+using namespace lldb_dap;
+using namespace lldb_dap::protocol;
+using lldb_private::File;
+using lldb_private::NativeFile;
+using lldb_private::Pipe;
+
+class DAPTest : public testing::Test {
+protected:
+  Pipe input;
+  Pipe output;
+  std::unique_ptr<Transport> toDAP;
+  std::unique_ptr<Transport> fromDAP;
+
+  void SetUp() override {
+    ASSERT_THAT_ERROR(input.CreateNew(false).ToError(), Succeeded());
+    ASSERT_THAT_ERROR(output.CreateNew(false).ToError(), Succeeded());
+    toDAP = std::make_unique<Transport>(
+        "toDAP", nullptr,
+        std::make_shared<NativeFile>(input.GetReadFileDescriptor(),
+                                     File::eOpenOptionReadOnly,
+                                     NativeFile::Unowned),
+        std::make_shared<NativeFile>(output.GetWriteFileDescriptor(),
+                                     File::eOpenOptionWriteOnly,
+                                     NativeFile::Unowned));
+    fromDAP = std::make_unique<Transport>(
+        "fromDAP", nullptr,
+        std::make_shared<NativeFile>(output.GetReadFileDescriptor(),
+                                     File::eOpenOptionReadOnly,
+                                     NativeFile::Unowned),
+        std::make_shared<NativeFile>(input.GetWriteFileDescriptor(),
+                                     File::eOpenOptionWriteOnly,
+                                     NativeFile::Unowned));
+  }
+};
+
+TEST_F(DAPTest, SendProtocolMessages) {
+  DAP dap{nullptr, ReplMode::Auto, {}, *toDAP};
+  dap.Send(Event{"my-event", std::nullopt});
+  ASSERT_THAT_EXPECTED(fromDAP->Read(std::chrono::milliseconds(1)),
+                       HasValue(testing::VariantWith<Event>(testing::FieldsAre(
+                           /*event=*/"my-event", /*body=*/std::nullopt))));
+}

[JDevlieghere]

Very excited to see more unit testing!

[ashgti]

Should I rename toDAP/fromDAP to to_dap/from_dap? I think thats more inline with how lldb names variables, right?

[JDevlieghere]

Should I rename toDAP/fromDAP to to_dap/from_dap? I think thats more inline with how lldb names variables, right?

Yes, good point. Currently we have a weird mix of styles. Once everything has been migrated to use the protocol classes, we should go through the code and fix the remaining inconsistencies.

[ashgti]

Yea, thats my mistake. I made the protocol classes to match the names of the spec, but we don't really have to do that as long as the toJSON/fromJSON adjusts the names, they can be different.

[JDevlieghere]

Yea, thats my mistake. I made the protocol classes to match the names of the spec, but we don't really have to do that as long as the toJSON/fromJSON adjusts the names, they can be different.

I think we discussed this at some point in the past and I'm personally okay with saying that everything outside the protocol dir should follow the LLDB style and the Protocol dir matches the spec (although we're already diverging from that for the enums).

[ashgti]

I added a few extra tests here and some extra base classes to help setup tests.

LMKWYT

[JDevlieghere]

I like the test base classes.

[JDevlieghere]

I somewhat regret the naming of the request handlers (*). Learning from our mistakes, maybe here we should just call it Handler/DisconnectTest.cpp to reduce the verbosity?

(*) In hindsight, I wish I called them RequestHandlerDisconnect instead of DisconnectRequestHandler, though that doesn't apply here as all the unit test files end with Test and I think consistency is more important.

[ashgti]

I renamed the file to Handler/DisconnectTest.cpp but should the test be DisconnectTest or should it be DisconnectRequestHandlerTest since thats the actual class I'm testing?

[JDevlieghere]

I think Handler/DisconnectTest is sufficiently clear.