This is an archive of the discontinued LLVM Phabricator instance.

[lldb][NFC] Add basic test for GUI command
ClosedPublic

Authored by teemperor on Aug 30 2019, 1:19 PM.

Details

Summary

This adds a basic test for the GUI command. Just tests that it starts up, that we can quit the gui
and help window, and that the basic UI elements are rendered. Mostly testing the waters how
testing this command will do on the bots or if that will cause some serious issues when we do
fancy ncurses stuff.

Diff Detail

Repository
rL LLVM

Event Timeline

teemperor created this revision.Aug 30 2019, 1:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 30 2019, 1:19 PM

Given how much fun Pavel had fixing my previous pexpect tests, I thought: "Why stop the fun after just two tests?". So here we go!

It is great to have tests. The main issue I would see is some of the text might not render correctly if the screen size is too small. Can we control the size (char width and height) of the screen in pexpect tests?

Seems like revision 370054 sets the screen size quite wide. Just checking that this change will be used for this test?

teemperor updated this revision to Diff 218161.Aug 30 2019, 1:36 PM
  • Set terminal dimension.

Yeah, Pavel added support for that (but I forgot to set them here because the test I used as a template didn't had the dimensions set).

labath accepted this revision.Sep 2 2019, 12:04 AM

Given how much fun Pavel had fixing my previous pexpect tests, I thought: "Why stop the fun after just two tests?". So here we go!

It looks like Jonas had a bit of fun too. ;)

Thank you for writing this, I think it's important to start experimenting with this. This test alone will increase our coverage of the gui command by a factor of infinity. :D

I do see some problems ahead. Like, this method is sufficient to test that some text is displayed on the screen, but it would be pretty hard to use it to check that the text is graphically laid out if a way that makes sense. Theoretically, we might assert entire ansi escape sequences, but that would be: a) clunky; b) susceptible to flakyness due to different curses versions choosing different (but equivalent) escape sequences. For that, we might need some sort of "terminal emulator" to process the escape sequences and produce the final window contents. Then, we could assert that, similarly to how some gui test frameworks talk to the X server to to get the actual window contents, click buttons, etc.

But that's just thinking about future. This looks fine to me right now.

As for the terminal size, the reason the other test sets the screen size so wide, is so that it avoids wrapping of file names, as that is what the test is asserting. That isn't the case here, though it's true that the test could be susceptible to window-size-related problems. If we don't specify the dimensions argument then pexpect is supposed to choose some sensible default itself. If we find that the default pexpect chooses is not suitable for us (perhaps because it picks the size of the parent terminal), then we can hardcode some reasonable default of our own into the launch method. However, I also don't think that's something we have to do now, as we don't have enough experience to know what that default is.

Anyway, LGMT, with one question: Given the recent test suite restructure, shouldn't this go somewhere under commands/gui or something?

lldb/packages/Python/lldbsuite/test/functionalities/gui_command/main.c
2 ↗(On Diff #218161)

You only need this if you actually intend to attach to the program (which, it seems, you don't).

This revision is now accepted and ready to land.Sep 2 2019, 12:04 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 2 2019, 1:00 AM

Yeah, the directory was pre-restructure. Moved to the right path before committing (and removed the other stuff you pointed out). Thanks!