nealsid added a comment. In D115126#3173236 <https://reviews.llvm.org/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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115126/new/ https://reviews.llvm.org/D115126 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
