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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
