[Lldb-commits] [PATCH] D68541: Implement 'up' and 'down' shortcuts in lldb gui

Wed Oct 16 03:54:58 PDT 2019

labath added inline comments.

================
Comment at: lldb/packages/Python/lldbsuite/test/commands/gui/basicdebug/TestGuiBasicDebug.py:31
+        #  which is not necessarily the order on the screen.)
+        self.child.timeout = 2
+        self.child.send("s") # step
----------------
llunak wrote:
> labath wrote:
> > What's the reason for that? It'd be best to not mess with the timeout in the test, and particularly to not set such a small time out as it will likely lead to flakyness on slow/heavily loaded machines.
> I added it because writing and debugging the unittest without this was a pain, but fair enough.
> 
Yeah, the pexpect tests leave a lot to be desired, and there are ways to avoid waiting for the entire timeout period when the output does not match (in the common cases), but it needs someone willing to implement it.

The deficiencies of pexpect become particularly obivous in the "gui" tests as there the raw stream of bytes coming out of the pty is way too low-level to be able to match it reliably. If we take the `[^\r\n]` pattern above, it encodes a lot of assumptions about how lldb and ncurses choose to print to the screen -- ncurses could perfectly well decide to insert some line breaks after printing the source code, but then go back (via ansi cursor movement codes), and print the stop reason. The result would be exactly the same as far as the user is concerned, but the test would fail.

For testing the gui mode, I think we should build (or reuse, if something suitable exists) a very simple "terminal emulator" which "understands" cursor movement codes (and maybe some other stuff). Then we could assert that the contents of the line as the user would see it instead of how it happened to be printed out.

Anyway, I don't think you have to do that now, but it is something to think about, should you want to invest more heavily into the lldb "gui".

Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D68541