[Lldb-commits] [PATCH] D115126: [LLDB] Add unit tests for Editline keyboard shortcuts

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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.



================
Comment at: lldb/unittests/Editline/EditlineTest.cpp:133-139
+  // 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.
----------------
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.


================
Comment at: lldb/unittests/Editline/EditlineTest.cpp:235
 
-  int ch;
+  char ch;
   while ((ch = fgetc(output_file)) != EOF) {
----------------
This isn't right. The return type of fgetc is deliberately not `char` so that `EOF` does not conflict with an actual character.


================
Comment at: lldb/unittests/Editline/EditlineTest.cpp:345
+  // mapped back to a specific input.
+  const std::string testNumber;
+  // Whether this keyboard shortcut is only bound in multi-line mode.
----------------
You already have a const on the variable declaration. You don't need to put one on every member as well.


================
Comment at: lldb/unittests/Editline/EditlineTest.cpp:352-359
+  // 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.
----------------
Why not just do the replacements in operator<< ?


================
Comment at: lldb/unittests/Editline/EditlineTest.cpp:454-455
+
+  ASSERT_THAT(output, ContainsRegex(editlineExpectedOutputRegex(kbtv)))
+      << "Key sequence was not bound to expected command name.";
+}
----------------
Why not just `Contains(kbtv)`? It doesn't look like you're making use of the regex functionality.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115126/new/

https://reviews.llvm.org/D115126