[LLDB] Add unit tests for Editline keyboard shortcuts

[nealsid]

Summary

This patch adds tests that verify keyboard shortcuts are set up for multi-line/single-line mode. Along the way I also added a few other small changes:

• Removed usage of shared_ptr in a case where object ownership was not shared.
• Changed a file's open mode to be read/write.
• Removed non-emacs configuration of keyboard shortcuts, because Editline config is hardcoded to "emacs" in ConfigureEditor.

[nealsid]

Fix casing typo in friend class declaration.

[labath]

All of the changes seem fine, but it's not clear to me what is the value of the new test. To me, it just seems like a change-detector forcing the two versions of the keybindings to stay in sync. The need for the friend declarations and everything also enforces the feeling that this test is not checking what it should be. Did you actually run into some kind of a problem/bug here that you think this test would have prevented?

Personally, I think it would be more useful if we verified the actual behavior of our editline code. We have some code here, in TestMultilineNavigation.py, and a couple of other places, but it would definitely be useful to have more of those.

[nealsid]

In D115126#3173236, @labath wrote:

All of the changes seem fine, but it's not clear to me what is the value of the new test. To me, it just seems like a change-detector forcing the two versions of the keybindings to stay in sync. The need for the friend declarations and everything also enforces the feeling that this test is not checking what it should be. Did you actually run into some kind of a problem/bug here that you think this test would have prevented?

Personally, I think it would be more useful if we verified the actual behavior of our editline code. We have some code here, in TestMultilineNavigation.py, and a couple of other places, but it would definitely be useful to have more of those.

Thank you! My thought process on the tests was that the exact binding is user facing, and a stray extra character or accidental deletion in the strings that represent the key sequences should be tested against, as well as any change that caused the shortcut to be registered incorrectly. If I used the same table in the dev & test code, it would not catch the first case, and a lost keyboard shortcut can be a visible bug, although, in this case, is easily fixable through the .editrc file.

The motivation is another change I have in progress that turns the Editline code into a table that stores the command, callback, key sequence, and help text in a table and loops over it to add them (which sounds suspiciously like this patch, I admit ;-)), and I wanted to make sure I didn't break any existing shortcuts when making that change, and some other changes I have planned, such as removing the conditional WCHAR support.

I also was on the fence about adding the test classes as friends. I could have exposed the functionality to retrieve a shortcut through our Editline class, but the test would be the only client today.

Definitely +1 to having keyboard shortcut tests as part of the Python tests. Mind if I take that as a TODO? I see how the coverage of both kinds of tests overlaps, but having the gtest tests can help find bugs earlier. Thanks again.

[nealsid]

Random unrelated question, and if I missed docs somewhere, I apologize. Did I forget something in creating the revision that caused Harbormaster to not do arc lint or arc test? https://reviews.llvm.org/B137600. Thanks.

[nealsid]

Testing ARC, no action necessary.

[labath]

Well.. I would say that the most user-facing aspect is the /action/ that happens after the user pressed the appropriate key. But at the end of the day, that doesn't matter that much -- we can (and do) test non-user-facing interfaces as well. But those tests make most sense when there is some complex logic in the code. Making sure that a table does not change by keeping another copy of the table is the very definition of a change-detector test (https://testing.googleblog.com/2015/01/testing-on-toilet-change-detector-tests.html). Like, it may have some value while you're doing the refactoring, but that value quickly drops to zero (or even becomes negative) after you complete it.

If it was a clean test, I could say "whatever, we can delete it if it becomes a burden" , but I can see several problems with the test (and the friend declaration is not the biggest of them), and I don't think it's worth trying to fix those. If you really want to work on that, then I'm not going to stop you, but I would be also perfectly happy to approve a patch that turns the keybindings into a table without an additional test.

lldb/unittests/Editline/EditlineTest.cpp

124–130

I find this hard to believe. I think it's more likely that this is working around some problem in the test itself. I can't say what is the precise problem without more investigation, but for example the synchronization around testOutputBuffer seems inadequate. GetEditlineOutput could deadlock if the read thread reads the newline before it reaches the wait call. It also does not handle spurious wakeups. And the accesses to testOutputBuffer are completely thread-unsafe.

226

This isn't right. The return type of fgetc is deliberately not char so that EOF does not conflict with an actual character.

333

You already have a const on the variable declaration. You don't need to put one on every member as well.

340–347

Why not just do the replacements in operator<< ?

442–443

Why not just Contains(kbtv)? It doesn't look like you're making use of the regex functionality.

[nealsid]

Thank you, Pavel - will address the specific code feedback in subsequent diff to this revision.

In D115126#3178906, @labath wrote:

Well.. I would say that the most user-facing aspect is the /action/ that happens after the user pressed the appropriate key. But at the end of the day, that doesn't matter that much -- we can (and do) test non-user-facing interfaces as well. But those tests make most sense when there is some complex logic in the code. Making sure that a table does not change by keeping another copy of the table is the very definition of a change-detector test (https://testing.googleblog.com/2015/01/testing-on-toilet-change-detector-tests.html). Like, it may have some value while you're doing the refactoring, but that value quickly drops to zero (or even becomes negative) after you complete it.

I'd disagree completely that the keypresses & key bindings are not user-facing.

The TOTT is interesting, but the changes I'm protecting against don't fall into that category. One reason is that, if the same table were to be used, a stray or inadvertent change could compile, pass tests, and make it to users.

Another reason is that editline's API takes varargs, which means that we rely on editline to handle all error cases correctly. I already ran into one issue where el_set(el, EL_BIND, ...) supports printing all bound keys (rather than making the caller specify which key to return a binding for), but the arguments it expects don't pass initial validation in el_set().

(these links may not be the same version as what ships on macOS or Linux distros)

https://salsa.debian.org/debian/libedit/-/blob/master/src/eln.c#L176
https://salsa.debian.org/debian/libedit/-/blob/master/src/map.c#L1315

I've never actually seen tests like the one in TOTT; I had 8 amazing years at Google, but one thing that always stuck out culturally was how every variation of "hello, world" could be turned into a conference-starting PhD thesis.

If it was a clean test, I could say "whatever, we can delete it if it becomes a burden" , but I can see several problems with the test (and the friend declaration is not the biggest of them), and I don't think it's worth trying to fix those. If you really want to work on that, then I'm not going to stop you, but I would be also perfectly happy to approve a patch that turns the keybindings into a table without an additional test.

I think it's worth it! It may not be the most important thing to LLDB users but there's clearly some code that doesn't get touched very often, and making it easier to make changes if requests come along will be a good thing. Also, for my own reasons, it is giving me some familiarity with the LLDB & LLVM code bases, especially the common code, before moving onto more core parts.

[nealsid]

Patch to address CR feedback.

[nealsid]

Thanks, Pavel.

lldb/unittests/Editline/EditlineTest.cpp

124–130

I spent a few hours on this before sending the initial patch; it may be some interaction with the file APIs and the Psuedoterminal class we have in LLVM. I could not debug anything it was doing that was leading to this behavior, but reliably saw EOF being sent before data that had been written to the stream. I also tried various things like flushing it manually, which did not work, but sleeping before closing the secondary FD enabled all data to be read on the test side. If you have some time, I'd be happy to have a gchat or screen sharing session and see if we can brainstorm something.

Regarding the synchronization, good call; I built on top of the existing threading, which did not require synchronizing reading the output of editline, because the actual output was never used. Now, I just call provide a method to call thread::join for tests that need to read the output to ensure it's all been written.

226

Ah, yeah, I changed it back. For some reason I thought EOF was sent as an actual EOF character like CTRL-D, not a special platform-specific integer.

333

I like const on the struct fields since it expresses the intent (a compile-time data structure) better, and, without it, if the array variable decl type was changed to remove the const, modifying the fields would be permitted.

340–347

I started that approach, but doing it at compile time seems easier because the output can be matched to the specific test by looking through the table, and...it's less code at runtime :)

[nealsid]

Remove extra header and using decl

[labath]

lldb/include/lldb/Host/Editline.h

392–393

This also isn't something we normally do. I'd recommend just making a function like DumpKeyBindings() or something. It's true that it would only be used in tests right now, but I can imagine some day adding an lldb command to dump current bindings, so it does not seem completely out of place. If you wanted to, you could even add it right now and do your test that way.

lldb/unittests/Editline/EditlineTest.cpp

86

const on a return value is useless

124–130

Well.. I tried running this locally, and the test passed just fine even with this code deleted. I don't think we should commit code like this without understanding the problem in more detail. (Like, if it's a problem with llvm code, then lets fix that; if it's a problem with libc code (I doubt that), then let's file a bug before working around it.)If you want to discuss this in a more interactive fashion, you can find me on the #lldb channel @ irc.oftc.net.

333

Well, we definitely have have different views on redundancy. I could write a PhD thesis on why I think this is wrong, but right now, let me just say this: it isn't consistent with how we define const structs anywhere else in llvm or lldb. Here's some examples: https://github.com/llvm/llvm-project/blob/e356027016c6365b3d8924f54c33e2c63d931492/lldb/unittests/Process/Utility/RegisterContextTest.cpp#L20, https://github.com/llvm/llvm-project/blob/165545c7a431aa3682fd9468014771c1c5228226/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp#L55, https://github.com/llvm/llvm-project/blob/e356027016c6365b3d8924f54c33e2c63d931492/llvm/unittests/Support/DJBTest.cpp#L21. (Full disclosure: I wrote the last two; while searching for examples, I also found a lot patterns like this which did not have any const annotation whatsoever.)

340–347

Ok, I can accept that.

[nealsid]

Thanks, Pavel. Some comments responded to, will take another look at the buffered/unbuffered I/O issue and address the remaining comments.

lldb/include/lldb/Host/Editline.h

392–393

Can you clarify this? I see a lot of friend class declarations in the LLDB codebase, both prod and test.

A command to dump key bindings could be useful in the future, but the requirement of parsing editline's stdout and manipulating it into some sort of structure for an API sounds like a maintenance tax until that command is implemented.

lldb/unittests/Editline/EditlineTest.cpp

124–130

Cool, I'll take another look..what platform were you running on?

333

I understand it may not be done this way elsewhere, but it still captures the intent of the data structure better. The idea of having a dependency on the declaration to enforce constness, rather than in the definition, doesn't reflect the point of the data structure.

[labath]

lldb/include/lldb/Host/Editline.h

392–393

We have a lot (too many) of friend declarations, but I'm not aware of any case where a production class befriends a test class (particularly one with a gtest-generated name). I know we sometimes make things protected (even though they could be private) for the sake of testing. That approach would also work here, but I think the new function would be better.

I didn't mean to parse the generated output. The function basically just be a wrapper for el_set(el, EL_BIND, nullptr) (I'm assuming that output is suitable for human consumption). The lldb command would then just call this function when invoked.

lldb/unittests/Editline/EditlineTest.cpp

124–130

It was x86_64-linux glibc-2.33 kernel-5.10.

333

Well, that's the difference. I don't see the constness as a necessary/intrinsic property of the data structure. I just view it as a glorified container -- a nicer version of std::tuple. For all it cares someone may want to dynamically generate test cases. It's up to the user to say what he wants to do with it. And the user -- the actual variable -- wants to provide a compile time list of test cases, and there it's perfectly natural to be const.

And in general, it's very rare to see const on a data member in c++, so upon seeing that, my mind automatically goes searching for some subtle reason for why it has to be there -- and then gets confused when it doesn't find any.

lldb/include/lldb/Host/Editline.h

private:
   /// the prompt to match.
   void SetCurrentLine(int line_index);
 
   /// Determines the width of the prompt in characters.  The width is guaranteed
   /// to be the same for
   /// all lines of the current multi-line session.
   int GetPromptWidth();
 
-  /// Returns true if the underlying EditLine session's keybindings are
-  /// Emacs-based, or false if
-  /// they are VI-based.
-  bool IsEmacs();
-
   /// Returns true if the current EditLine buffer contains nothing but spaces,
   /// or is empty.
   bool IsOnlySpaces();
 
   /// Helper method used by MoveCursor to determine relative line position.
   int GetLineIndexForLocation(CursorLocation location, int cursor_row);
 
   /// Move the cursor from one well-established location to another using
#endif
   const char *m_fix_indentation_callback_chars = nullptr;
 
   CompleteCallbackType m_completion_callback;
 
   SuggestionCallbackType m_suggestion_callback;
 
   std::size_t m_previous_autosuggestion_size = 0;
   std::mutex m_output_mutex;
+
+  friend class EditlineKeyboardBindingTest_MultilineEditlineKeybindings_Test;
+  friend class EditlineKeyboardBindingTest_SinglelineEditlineKeybindings_Test;
 };
 }
 
 #endif // #if defined(__cplusplus)
 #endif // LLDB_HOST_EDITLINE_H

lldb/source/Host/common/Editline.cpp

 
 void Editline::SetCurrentLine(int line_index) {
   m_current_line_index = line_index;
   m_current_prompt = PromptForIndex(line_index);
 }
 
 int Editline::GetPromptWidth() { return (int)PromptForIndex(0).length(); }
 
-bool Editline::IsEmacs() {
-  const char *editor;
-  el_get(m_editline, EL_EDITOR, &editor);
-  return editor[0] == 'e';
-}
-
 bool Editline::IsOnlySpaces() {
   const LineInfoW *info = el_wline(m_editline);
   for (const EditLineCharType *character = info->buffer;
        character < info->lastchar; character++) {
     if (*character != ' ')
       return false;
   }
   return true;
if (multiline) {
     el_set(m_editline, EL_BIND, "^p", "lldb-previous-line", NULL);
     el_set(m_editline, EL_BIND, "^n", "lldb-next-line", NULL);
     el_set(m_editline, EL_BIND, "^?", "lldb-delete-previous-char", NULL);
     el_set(m_editline, EL_BIND, "^d", "lldb-delete-next-char", NULL);
     el_set(m_editline, EL_BIND, ESCAPE "[3~", "lldb-delete-next-char", NULL);
     el_set(m_editline, EL_BIND, ESCAPE "[\\^", "lldb-revert-line", NULL);
 
     // Editor-specific bindings
-    if (IsEmacs()) {
-      el_set(m_editline, EL_BIND, ESCAPE "<", "lldb-buffer-start", NULL);
-      el_set(m_editline, EL_BIND, ESCAPE ">", "lldb-buffer-end", NULL);
-      el_set(m_editline, EL_BIND, ESCAPE "[A", "lldb-previous-line", NULL);
-      el_set(m_editline, EL_BIND, ESCAPE "[B", "lldb-next-line", NULL);
-      el_set(m_editline, EL_BIND, ESCAPE ESCAPE "[A", "lldb-previous-history",
-             NULL);
-      el_set(m_editline, EL_BIND, ESCAPE ESCAPE "[B", "lldb-next-history",
-             NULL);
-      el_set(m_editline, EL_BIND, ESCAPE "[1;3A", "lldb-previous-history",
-             NULL);
-      el_set(m_editline, EL_BIND, ESCAPE "[1;3B", "lldb-next-history", NULL);
-    } else {
-      el_set(m_editline, EL_BIND, "^H", "lldb-delete-previous-char", NULL);
-
-      el_set(m_editline, EL_BIND, "-a", ESCAPE "[A", "lldb-previous-line",
-             NULL);
-      el_set(m_editline, EL_BIND, "-a", ESCAPE "[B", "lldb-next-line", NULL);
-      el_set(m_editline, EL_BIND, "-a", "x", "lldb-delete-next-char", NULL);
-      el_set(m_editline, EL_BIND, "-a", "^H", "lldb-delete-previous-char",
-             NULL);
-      el_set(m_editline, EL_BIND, "-a", "^?", "lldb-delete-previous-char",
-             NULL);
-
-      // Escape is absorbed exiting edit mode, so re-register important
-      // sequences without the prefix
-      el_set(m_editline, EL_BIND, "-a", "[A", "lldb-previous-line", NULL);
-      el_set(m_editline, EL_BIND, "-a", "[B", "lldb-next-line", NULL);
-      el_set(m_editline, EL_BIND, "-a", "[\\^", "lldb-revert-line", NULL);
-    }
+    el_set(m_editline, EL_BIND, ESCAPE ESCAPE "[B", "lldb-next-history", NULL);
+    el_set(m_editline, EL_BIND, ESCAPE "[1;3A", "lldb-previous-history", NULL);
   }
 }
 
 // Editline public methods
 
 Editline *Editline::InstanceFor(EditLine *editline) {
   Editline *editor;
   el_get(editline, EL_CLIENTDATA, &editor);

lldb/unittests/Editline/EditlineTest.cpp

 #include "TestingSupport/SubsystemRAII.h"
 #include "lldb/Host/Editline.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/Pipe.h"
 #include "lldb/Host/PseudoTerminal.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/StringList.h"
 
+#define ESCAPE "\x1b"
+
 using namespace lldb_private;
+using namespace testing;
 
 namespace {
 const size_t TIMEOUT_MILLIS = 5000;
 }
 
 class FilePointer {
 public:
   FilePointer() = delete;
public:
 
   bool GetLine(std::string &line, bool &interrupted, size_t timeout_millis);
 
   bool GetLines(lldb_private::StringList &lines, bool &interrupted,
                 size_t timeout_millis);
 
   void ConsumeAllOutput();
 
+  const std::string GetEditlineOutput() { return testOutputBuffer.str(); }
+
 private:
   bool IsInputComplete(lldb_private::Editline *editline,
                        lldb_private::StringList &lines);
 
   std::unique_ptr<lldb_private::Editline> _editline_sp;
 
   PseudoTerminal _pty;
   int _pty_primary_fd;
   int _pty_secondary_fd;
 
   std::unique_ptr<FilePointer> _el_secondary_file;
+  std::stringstream testOutputBuffer;
 };
 
 EditlineAdapter::EditlineAdapter()
     : _editline_sp(), _pty(), _pty_primary_fd(-1), _pty_secondary_fd(-1),
       _el_secondary_file() {
   lldb_private::Status error;
 
   // Open the first primary pty available.
   EXPECT_THAT_ERROR(_pty.OpenFirstAvailablePrimary(O_RDWR), llvm::Succeeded());
 
   // Grab the primary fd.  This is a file descriptor we will:
   // (1) write to when we want to send input to editline.
   // (2) read from when we want to see what editline sends back.
   _pty_primary_fd = _pty.GetPrimaryFileDescriptor();
 
   // Open the corresponding secondary pty.
   EXPECT_THAT_ERROR(_pty.OpenSecondary(O_RDWR), llvm::Succeeded());
   _pty_secondary_fd = _pty.GetSecondaryFileDescriptor();
 
-  _el_secondary_file.reset(new FilePointer(fdopen(_pty_secondary_fd, "rw")));
+  _el_secondary_file.reset(new FilePointer(fdopen(_pty_secondary_fd, "w+")));
   EXPECT_FALSE(nullptr == *_el_secondary_file);
   if (*_el_secondary_file == nullptr)
     return;
 
+  // We have to set the output stream we pass to Editline as not using
+  // buffered I/O.  Otherwise we are missing editline's output when we
+  // close the stream in the keybinding test (i.e. the EOF comes
+  // before data previously written to the stream by editline).  This
+  // behavior isn't as I understand the spec becuse fclose should
+  // flush the stream, but my best guess is that it's some unexpected
+  // interaction with stream I/O and ptys.
+  EXPECT_EQ(setvbuf(*_el_secondary_file, nullptr, _IONBF, 0), 0)
+      << "Could not set editline output stream to use unbuffered I/O.";
+
   // Create an Editline instance.
   _editline_sp.reset(new lldb_private::Editline(
       "gtest editor", *_el_secondary_file, *_el_secondary_file,
       *_el_secondary_file, false));
   _editline_sp->SetPrompt("> ");
 
   // Hookup our input complete callback.
   auto input_complete_cb = [this](Editline *editline, StringList &lines) {
bool EditlineAdapter::IsInputComplete(lldb_private::Editline *editline,
   }
 
   return (start_block_count > 0) && (brace_balance == 0);
 }
 
 void EditlineAdapter::ConsumeAllOutput() {
   FilePointer output_file(fdopen(_pty_primary_fd, "r"));
 
-  int ch;
+  char ch;
   while ((ch = fgetc(output_file)) != EOF) {
 #if EDITLINE_TEST_DUMP_OUTPUT
     char display_str[] = {0, 0, 0};
     switch (ch) {
     case '\t':
       display_str[0] = '\\';
       display_str[1] = 't';
       break;
     case '\n':
       display_str[0] = '\\';
       display_str[1] = 'n';
       break;
     case '\r':
       display_str[0] = '\\';
       display_str[1] = 'r';
       break;
     default:
       display_str[0] = ch;
       break;
     }
     printf("<stdout> 0x%02x (%03d) (%s)\n", ch, ch, display_str);
-// putc(ch, stdout);
 #endif
+    testOutputBuffer << ch;
   }
 }
 
 class EditlineTestFixture : public ::testing::Test {
   SubsystemRAII<FileSystem> subsystems;
   EditlineAdapter _el_adapter;
-  std::shared_ptr<std::thread> _sp_output_thread;
+  std::thread _sp_output_thread;
 
 public:
   static void SetUpTestCase() {
     // We need a TERM set properly for editline to work as expected.
     setenv("TERM", "vt100", 1);
   }
 
   void SetUp() override {
     // Validate the editline adapter.
     EXPECT_TRUE(_el_adapter.IsValid());
     if (!_el_adapter.IsValid())
       return;
 
     // Dump output.
-    _sp_output_thread =
-        std::make_shared<std::thread>([&] { _el_adapter.ConsumeAllOutput(); });
+    _sp_output_thread = std::thread([&] { _el_adapter.ConsumeAllOutput(); });
   }
 
   void TearDown() override {
     _el_adapter.CloseInput();
-    if (_sp_output_thread)
-      _sp_output_thread->join();
+    if (_sp_output_thread.joinable())
+      _sp_output_thread.join();
   }
 
   EditlineAdapter &GetEditlineAdapter() { return _el_adapter; }
 };
 
 TEST_F(EditlineTestFixture, EditlineReceivesSingleLineText) {
   // Send it some text via our virtual keyboard.
   const std::string input_text("Hello, world");
TEST_F(EditlineTestFixture, EditlineReceivesMultiLineText) {
   // input.
   std::vector<std::string> reported_lines;
   for (const std::string &line : el_reported_lines)
     reported_lines.push_back(line);
 
   EXPECT_THAT(reported_lines, testing::ContainerEq(input_lines));
 }
 
+namespace lldb_private {
+
+// Parameter structure for parameterized tests.
+struct KeybindingTestValue {
+  // A number that is used to name the test, so test output can be
+  // mapped back to a specific input.
+  const std::string testNumber;
+  // Whether this keyboard shortcut is only bound in multi-line mode.
+  bool multilineOnly;
+  // The actual key sequence.
+  const std::string keySequence;
+  // The command the key sequence is supposed to execute.
+  const std::string commandName;
+  // This is initialized to the keySequence by default, but gtest has
+  // some errors when the test name as created by the overloaded
+  // operator<< has embedded newlines.  This is even true when we
+  // specify a custom test name function, as we do below when we
+  // instantiate the test case.  In cases where the keyboard shortcut
+  // has a newline or carriage return, this field in the struct can be
+  // set to something that is printable.
+  const std::string &printableKeySequence = keySequence;
+};
+
+std::ostream &operator<<(std::ostream &os, const KeybindingTestValue &kbtv) {
+  return os << "{" << kbtv.printableKeySequence << "  =>  " << kbtv.commandName
+            << " (multiline only: " << kbtv.multilineOnly << ")}";
+}
+
+// The keyboard shortcuts that we're testing.
+const KeybindingTestValue keySequences[] = {
+    {"1", false, "^w", "ed-delete-prev-word"},
+    {"2", false, "\t", "lldb-complete"},
+    {"3", false, ESCAPE "[1;5C", "em-next-word"},
+    {"4", false, ESCAPE "[1;5D", "ed-prev-word"},
+    {"5", false, ESCAPE "[5C", "em-next-word"},
+    {"6", false, ESCAPE "[5D", "ed-prev-word"},
+    {"7", false, ESCAPE ESCAPE "[C", "em-next-word"},
+    {"8", false, ESCAPE ESCAPE "[D", "ed-prev-word"},
+    {"9", true, "\n", "lldb-end-or-add-line", "<CR>"},
+    {"10", true, "\r", "lldb-end-or-add-line", "<LF>"},
+    {"11", true, ESCAPE "\n", "lldb-break-line", ESCAPE "<CR>"},
+    {"12", true, ESCAPE "\r", "lldb-break-line", ESCAPE "<LF>"},
+    {"13", true, "^p", "lldb-previous-line"},
+    {"14", true, "^n", "lldb-next-line"},
+    {"15", true, "^?", "lldb-delete-previous-char"},
+    {"16", true, "^d", "lldb-delete-next-char"},
+    {"17", true, ESCAPE "[3~", "lldb-delete-next-char"},
+    {"18", true, ESCAPE "[\\^", "lldb-revert-line"},
+    {"19", true, ESCAPE "<", "lldb-buffer-start"},
+    {"20", true, ESCAPE ">", "lldb-buffer-end"},
+    {"21", true, ESCAPE "[A", "lldb-previous-line"},
+    {"22", true, ESCAPE "[B", "lldb-next-line"},
+    {"23", true, ESCAPE ESCAPE "[A", "lldb-previous-history"},
+    {"24", true, ESCAPE ESCAPE "[B", "lldb-next-history"},
+    {"25", true, ESCAPE "[1;3A", "lldb-previous-history"},
+    {"26", true, ESCAPE "[1;3B", "lldb-next-history"},
+};
+
+class EditlineKeyboardBindingTest
+    : public EditlineTestFixture,
+      public testing::WithParamInterface<KeybindingTestValue> {};
+
+// Helper method to call into libedit to have it output a keyboard
+// shortcut mapping.
+void retrieveEditlineShortcutKey(::EditLine *el,
+                                 const std::string &keySequence) {
+  ASSERT_EQ(el_set(el, EL_BIND, keySequence.c_str(), nullptr), 0)
+      << "Retrieving editline keybinding failed for " << keySequence;
+}
+
+// Helper function that, given a test parameter value, returns a
+// string that matches the editline output for a given shortcut key.
+const std::string editlineExpectedOutput(KeybindingTestValue kbtv) {
+  std::stringstream builder;
+  builder << kbtv.keySequence + "\t->\t" << kbtv.commandName;
+  return builder.str();
+}
+
+// Test cases for editline in single-line mode.
+TEST_P(EditlineKeyboardBindingTest, SingleLineEditlineKeybindings) {
+  KeybindingTestValue kbtv = GetParam();
+
+  auto &editLine = GetEditlineAdapter().GetEditline();
+
+  editLine.ConfigureEditor(false);
+
+  retrieveEditlineShortcutKey(editLine.m_editline, kbtv.keySequence);
+
+  const std::string output = GetEditlineAdapter().GetEditlineOutput();
+
+  // If the shortcut key is only in multiline mode, verify that it is
+  // not mapped to the command.  It could still be mapped by default,
+  // so we just check if our command doesn't appear in the output.
+  if (kbtv.multilineOnly) {
+    ASSERT_THAT(output, Not(HasSubstr(kbtv.commandName)))
+        << "Multiline key was still bound in single-line mode.";
+    return;
+  }
+
+  // Otherwise, compare the output to make sure our command is mapped
+  // to the shortcut key.
+  ASSERT_THAT(output, StrCaseNe(editlineExpectedOutput(kbtv)))
+      << "Key sequence was not bound to expected command name.";
+}
+
+// Test cases for editline in multi-line mode.
+TEST_P(EditlineKeyboardBindingTest, MultiLineEditlineKeybindings) {
+  KeybindingTestValue kbtv = GetParam();
+
+  auto &editLine = GetEditlineAdapter().GetEditline();
+
+  editLine.ConfigureEditor(true);
+
+  retrieveEditlineShortcutKey(editLine.m_editline, kbtv.keySequence);
+
+  const std::string output = GetEditlineAdapter().GetEditlineOutput();
+
+  ASSERT_THAT(output, StrCaseNe(editlineExpectedOutput(kbtv)))
+      << "Key sequence was not bound to expected command name.";
+}
+
+INSTANTIATE_TEST_SUITE_P(KeyboardShortcuts, EditlineKeyboardBindingTest,
+                         testing::ValuesIn(keySequences),
+                         [](const TestParamInfo<KeybindingTestValue> &kvb) {
+                           return kvb.param.testNumber;
+                         });
+
+} // namespace lldb_private
+
 #endif