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

[Neal via Phabricator via lldb-commits]

nealsid added a comment.

Thanks, Pavel.



================
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.
----------------
labath wrote:
> 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.
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.  


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


================
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.
----------------
labath wrote:
> You already have a const on the variable declaration. You don't need to put one on every member as well.
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.


================
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.
----------------
labath wrote:
> Why not just do the replacements in operator<< ?
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 :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115126