This is an archive of the discontinued LLVM Phabricator instance.

[lldb][test] Prevent \n in calls to lldb's expect() test helper.
ClosedPublic

Authored by rupprecht on Nov 15 2019, 10:34 AM.

Details

Summary

expect() forwards its command to sendline(). This can be problematic if the command already contains a newline: sendline() unconditionally adds a newline to the command, which causes the command to run twice (hitting enter in lldb runs the previous command). The expect() helper looks for the prompt and finds the first one, but because the command has run a second time, the buffer will contain the contents of the second time the command ran, causing potential erroneous matching.

Simplify the editline test, which was using different commands to workaround this misunderstanding.

Diff Detail

Event Timeline

rupprecht created this revision.Nov 15 2019, 10:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 15 2019, 10:34 AM

Build result: pass - 60084 tests passed, 0 failed and 729 were skipped.
Log files: console-log.txt, CMakeCache.txt

labath accepted this revision.Nov 18 2019, 1:06 AM

Thanks for getting to the bottom of this. Adding the \n check is fine. The "expect" function was meant to be an pexpect equivalent of the "expect" function in regular tests (which also just handles a single command). If anyone is doing something fancier than that, he should probably interact with pexpect directly.

lldb/packages/Python/lldbsuite/test/terminal/TestEditline.py
26–33

Maybe update the example to use the command "print" too?

This revision is now accepted and ready to land.Nov 18 2019, 1:06 AM
This revision was automatically updated to reflect the committed changes.

Hey Jordan, the test is failing on the sanitized bot on GreenDragon. The error is Could not terminate the child. Could you have a look?

http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-sanitized/541/console

davide added a subscriber: davide.Nov 21 2019, 10:59 AM

Hey Jordan, it looks like some of the changes to TestEditLine [or adjacent to it] broke the sanitized build on macOS.
Can I ask you to take a look? Thanks

http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-sanitized/539/console

Looks like we happened to be looking at this at the same time :D

[This is probably not the right patch, as the last run on that bot still doesn't have this commit.]

My understanding is that all pexpect tests are failing on that bot and are skipped with @skipIfAsan. Probably this one needs to be skipped too. @teemperor was trying to reproduce some of those failures locally and failed... Maybe that bot is cursed...

[This is probably not the right patch, as the last run on that bot still doesn't have this commit.]

My understanding is that all pexpect tests are failing on that bot and are skipped with @skipIfAsan. Probably this one needs to be skipped too. @teemperor was trying to reproduce some of those failures locally and failed... Maybe that bot is cursed...

Fair.

[This is probably not the right patch, as the last run on that bot still doesn't have this commit.]

-> D70137 is the correct patch that actually adds this test -- this is testonly cleanup

My understanding is that all pexpect tests are failing on that bot and are skipped with @skipIfAsan. Probably this one needs to be skipped too. @teemperor was trying to reproduce some of those failures locally and failed... Maybe that bot is cursed...

Fair.

I looked at the failure in some detail. (I don't have hardware to repro it on though). The test itself runs fine but the cleanup (I guess the self.quit()) at the end seems to fail: pexpect.exceptions.ExceptionPexpect: Could not terminate the child. Is that in line with the failures that @skipIfAsan is intended for?

[This is probably not the right patch, as the last run on that bot still doesn't have this commit.]

-> D70137 is the correct patch that actually adds this test -- this is testonly cleanup

My understanding is that all pexpect tests are failing on that bot and are skipped with @skipIfAsan. Probably this one needs to be skipped too. @teemperor was trying to reproduce some of those failures locally and failed... Maybe that bot is cursed...

Fair.

I looked at the failure in some detail. (I don't have hardware to repro it on though). The test itself runs fine but the cleanup (I guess the self.quit()) at the end seems to fail: pexpect.exceptions.ExceptionPexpect: Could not terminate the child. Is that in line with the failures that @skipIfAsan is intended for?

Yeah that's the issue all expect tests have with Asan. It's related to all the timeouts in pexpect probably.