Page MenuHomePhabricator

Increase testsuite packet-timeout 5secs -> 1min
ClosedPublic

Authored by jankratochvil on Jul 25 2019, 4:30 AM.

Details

Summary

rL357954 did increase packet-timeout 1sec -> 5secs. Which is IMO about the maximum timeout reasonable for regular use. But for testsuite I think the timeout should be higher as the testsuite runs in parallel and it can be run even on slow hosts and with other load (moreover if it runs on some slow arch).
I have chosen 60 secs, that should be enough hopefully. Larger value could make debugging with hanging lldb-server annoying.
This patch was based on this testsuite timeout:
http://lab.llvm.org:8014/builders/lldb-x86_64-fedora/builds/546/steps/test/logs/stdio

FAIL: test_connect (TestGDBRemoteClient.TestGDBRemoteClient)
   Test connecting to a remote gdb server
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/jkratoch/slave-lldb-x86_64-fedora/lldb-x86_64-fedora/llvm/tools/lldb/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteClient.py", line 13, in test_connect
    process = self.connect(target)
  File "/home/jkratoch/slave-lldb-x86_64-fedora/lldb-x86_64-fedora/llvm/tools/lldb/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py", line 480, in connect
    self.assertTrue(error.Success(), error.description)
AssertionError: False is not True : failed to get reply to handshake packet

Diff Detail

Repository
rLLDB LLDB

Event Timeline

jankratochvil created this revision.Jul 25 2019, 4:30 AM
jankratochvil marked 3 inline comments as done.Jul 25 2019, 4:34 AM
jankratochvil added inline comments.
lldb/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestNoWatchpointSupportInfo.py
24

Here it was always timing out (and still reporting PASS) which was not a problem for 5 seconds timeout but with 5 minutes timeout it was hanging during the testsuite run.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
214

The failed TestGDBRemoteClient testcases did timeout despite this increase to 6 seconds (which made more sense before rL357954 when the default timeout was just 1 second and not the current 5 seconds).
If this patch is not welcome maybe we could try running the testsuite just increasing this seconds(6) to some seconds(60).

lldb/tools/lldb-test/lldb-test.cpp
984

I somehow failed to find how to access the timeout by API, I can try again.

Why not unify now? It shouldn't be too hard to at least unify all the instances in dotest python code to a single place. That would already cut the number of places that need to be kept synced in half...

lldb/tools/lldb-test/lldb-test.cpp
984

I wouldn't be surprised if there is no api for that.

There isn't an SB API for "settings set" yet. I've put off doing that because the "settings" subsystem is currently unfinished. In the original design you would be specify qualifiers in the setting hierarchy, like:

settings set target[arch=x86_64].whatever

That would be pretty handy in a lot of cases - for instance there are some settings that you have to set when doing KEXT development on macOS that are not appropriate for user-space debugging and this trips up the kernel developers all the time.

But we've never gotten around to designing this fully, and I've don't want to design the SB API's for settings till we know how this is going to work. Apparently I didn't put this on the Projects page, I'll go rectify that in a bit.

jankratochvil marked an inline comment as done.Jul 25 2019, 2:47 PM

Why not unify now? It shouldn't be too hard to at least unify all the instances in dotest python code to a single place. That would already cut the number of places that need to be kept synced in half...

Done; but I do not like much that from lldbsuite.test.lldbtest import Base in vscode.py but then other choices (passing Base.setUpCommands() from callers) looks even more messy to me.

jankratochvil planned changes to this revision.Jul 25 2019, 7:46 PM

Why not unify now? It shouldn't be too hard to at least unify all the instances in dotest python code to a single place. That would already cut the number of places that need to be kept synced in half...

Done; but I do not like much that from lldbsuite.test.lldbtest import Base in vscode.py but then other choices (passing Base.setUpCommands() from callers) looks even more messy to me.

I think I'd go for threading it in from VSCodeTestCaseBase. Maybe not as an argument to request_attach and friends, but possibly as an argument to the DebugCommunication object constructor.

jankratochvil retitled this revision from Increase testsuite packet-timeout 5secs -> 5mins to Increase testsuite packet-timeout 5secs -> 1min.
jankratochvil edited the summary of this revision. (Show Details)

I think I'd go for threading it in from VSCodeTestCaseBase. Maybe not as an argument to request_attach and friends, but possibly as an argument to the DebugCommunication object constructor.

That looks as a good place to me, thanks. Be careful with the review I do not know Python almost at all.

labath accepted this revision.Jul 29 2019, 8:37 AM

LGTM, thanks.

BTW, I was pretty much a python noob too when starting with lldb, but our test suite has forced me to become somewhat of an expert. :)

lldb/tools/lldb-test/lldb-test.cpp
979

lldb-test is not confined by the SB API. The reason this (unlike the symbols.enable-external-lookup thingy) does not work is because ProcessGDBRemote is a plugin, and so the API to access the setting is hidden by the plugin encapsulation...

This revision is now accepted and ready to land.Jul 29 2019, 8:37 AM
jankratochvil marked 2 inline comments as done.Jul 29 2019, 9:03 AM
jankratochvil added inline comments.
lldb/tools/lldb-test/lldb-test.cpp
979

Thanks for the explanation; still SB API could provide some function to lookup the plugin by its name to access its settings etc. I have rather deleted the comment there.

This revision was automatically updated to reflect the committed changes.
jankratochvil marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptJul 29 2019, 9:11 AM