[Lldb-commits] [PATCH] D56014: [lldb] - Fix crash when listing the history with the key up.

[George Rimar via Phabricator via lldb-commits]

grimar added a comment.

In D56014#1339419 <https://reviews.llvm.org/D56014#1339419>, @davide wrote:

> testcase?


I investigated how to add it and I do not see a good way write a test :(

Seems the proper place to test it would be `unittest/Editline`:
https://github.com/llvm-mirror/lldb/blob/master/unittests/Editline/EditlineTest.cpp

It allows to send the text using the virtual terminal:
https://github.com/llvm-mirror/lldb/blob/master/unittests/Editline/EditlineTest.cpp#L294

And I was able to simulate the key up press ("lldb-previous-line" command) sending the "\x1b[A" line
and to trigger the call of the private `RecallHistory` method of `Editline`, where this patch did the fix.
(Since we do not want to make methods public just for the test I believe)

But this is not enough. Case fixed by this patch assumes that some expression history exists.
Class responsible for work with the history is `EditlineHistory`:
https://github.com/llvm-mirror/lldb/blob/master/source/Host/common/Editline.cpp#L162
It contains logic to find the path for history file and logic to load/save it with the editline library API.
This class is a privately implemented in Editline.cpp and used internally by `Editline`.

I am not sure it is a good idea to export it for the unit test. I do not think it is correct
to duplicate its logic either or to create a history file manually, given that it seems
to contain a mini header, probably specific to the editline library version.
And also (perhaps that would not be needed, but FTR) it does not seem to be a good idea to use
the editline API in EditlineTest.cpp, I think it should remain encapsulated in Editline.cpp, like now.

Given all above it seems it is really problematic to write a test for this now.
My suggestion probably is to go without it (given that patch fixes an obvious mistype).


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

https://reviews.llvm.org/D56014