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

[Neal via Phabricator via lldb-commits]

nealsid added a comment.

Thank you, Pavel - will address the specific code feedback in subsequent patch.

In D115126#3178906 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115126