-
Notifications
You must be signed in to change notification settings - Fork 15.4k
[lldb-dap] Unify the timeouts for the DAP tests #163292
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Draft because this breaks |
lldb/test/API/tools/lldb-dap/breakpoint-events/TestDAP_breakpointEvents.py
Show resolved
Hide resolved
lldb/test/API/tools/lldb-dap/module-event/TestDAP_module_event.py
Outdated
Show resolved
Hide resolved
Various DAP tests are specifying their own timeouts, with values ranging from "1" to "20". Most of them seem arbitrary, but some come with a comment. The performance characters of running these tests in CI are unpredictable (they generally run much slower than developers expect) and really not something we can make assumptions about. I suspect these timeouts are a contributing factor to the flakiness of the DAP tests. This PR unifies the timeouts around a central value in the DAP server. Fixes llvm#162523
a87d191 to
e32962e
Compare
|
@llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) ChangesVarious DAP tests are specifying their own timeouts, with values ranging from "1" to "20". Most of them seem arbitrary, but some come with a comment. The performance characters of running these tests in CI are unpredictable (they generally run much slower than developers expect) and really not something we can make assumptions about. I suspect these timeouts are a contributing factor to the flakiness of the DAP tests. This PR unifies the timeouts around a central value in the DAP server. Fixes #162523 Patch is 22.92 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/163292.diff 9 Files Affected:
diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
index 8eb64b4ab8b2b..7ce90bdf0d362 100644
--- a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
+++ b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
@@ -27,6 +27,10 @@
Literal,
)
+# set timeout based on whether ASAN was enabled or not. Increase
+# timeout by a factor of 10 if ASAN is enabled.
+DEFAULT_TIMEOUT = 10 * (10 if ("ASAN_OPTIONS" in os.environ) else 1)
+
## DAP type references
@@ -282,26 +286,24 @@ def get_output(self, category: str, clear=True) -> str:
def collect_output(
self,
category: str,
- timeout: float,
pattern: Optional[str] = None,
clear=True,
) -> str:
"""Collect output from 'output' events.
Args:
category: The category to collect.
- timeout: The max duration for collecting output.
pattern:
Optional, if set, return once this pattern is detected in the
collected output.
Returns:
The collected output.
"""
- deadline = time.monotonic() + timeout
+ deadline = time.monotonic() + DEFAULT_TIMEOUT
output = self.get_output(category, clear)
while deadline >= time.monotonic() and (
pattern is None or pattern not in output
):
- event = self.wait_for_event(["output"], timeout=deadline - time.monotonic())
+ event = self.wait_for_event(["output"])
if not event: # Timeout or EOF
break
output += self.get_output(category, clear=clear)
@@ -555,25 +557,20 @@ def predicate(p: ProtocolMessage):
return cast(Optional[Response], self._recv_packet(predicate=predicate))
- def wait_for_event(
- self, filter: List[str] = [], timeout: Optional[float] = None
- ) -> Optional[Event]:
+ def wait_for_event(self, filter: List[str] = []) -> Optional[Event]:
"""Wait for the first event that matches the filter."""
def predicate(p: ProtocolMessage):
return p["type"] == "event" and p["event"] in filter
return cast(
- Optional[Event], self._recv_packet(predicate=predicate, timeout=timeout)
+ Optional[Event],
+ self._recv_packet(predicate=predicate, timeout=DEFAULT_TIMEOUT),
)
- def wait_for_stopped(
- self, timeout: Optional[float] = None
- ) -> Optional[List[Event]]:
+ def wait_for_stopped(self) -> Optional[List[Event]]:
stopped_events = []
- stopped_event = self.wait_for_event(
- filter=["stopped", "exited"], timeout=timeout
- )
+ stopped_event = self.wait_for_event(filter=["stopped", "exited"])
while stopped_event:
stopped_events.append(stopped_event)
# If we exited, then we are done
@@ -582,26 +579,28 @@ def wait_for_stopped(
# Otherwise we stopped and there might be one or more 'stopped'
# events for each thread that stopped with a reason, so keep
# checking for more 'stopped' events and return all of them
- stopped_event = self.wait_for_event(
- filter=["stopped", "exited"], timeout=0.25
+ # Use a shorter timeout for additional stopped events
+ def predicate(p: ProtocolMessage):
+ return p["type"] == "event" and p["event"] in ["stopped", "exited"]
+
+ stopped_event = cast(
+ Optional[Event], self._recv_packet(predicate=predicate, timeout=0.25)
)
return stopped_events
- def wait_for_breakpoint_events(self, timeout: Optional[float] = None):
+ def wait_for_breakpoint_events(self):
breakpoint_events: list[Event] = []
while True:
- event = self.wait_for_event(["breakpoint"], timeout=timeout)
+ event = self.wait_for_event(["breakpoint"])
if not event:
break
breakpoint_events.append(event)
return breakpoint_events
- def wait_for_breakpoints_to_be_verified(
- self, breakpoint_ids: list[str], timeout: Optional[float] = None
- ):
+ def wait_for_breakpoints_to_be_verified(self, breakpoint_ids: list[str]):
"""Wait for all breakpoints to be verified. Return all unverified breakpoints."""
while any(id not in self.resolved_breakpoints for id in breakpoint_ids):
- breakpoint_event = self.wait_for_event(["breakpoint"], timeout=timeout)
+ breakpoint_event = self.wait_for_event(["breakpoint"])
if breakpoint_event is None:
break
@@ -614,14 +613,14 @@ def wait_for_breakpoints_to_be_verified(
)
]
- def wait_for_exited(self, timeout: Optional[float] = None):
- event_dict = self.wait_for_event(["exited"], timeout=timeout)
+ def wait_for_exited(self):
+ event_dict = self.wait_for_event(["exited"])
if event_dict is None:
raise ValueError("didn't get exited event")
return event_dict
- def wait_for_terminated(self, timeout: Optional[float] = None):
- event_dict = self.wait_for_event(["terminated"], timeout)
+ def wait_for_terminated(self):
+ event_dict = self.wait_for_event(["terminated"])
if event_dict is None:
raise ValueError("didn't get terminated event")
return event_dict
diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
index f7b1ed80fceb5..29935bb8046ff 100644
--- a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
+++ b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
@@ -18,7 +18,7 @@
class DAPTestCaseBase(TestBase):
# set timeout based on whether ASAN was enabled or not. Increase
# timeout by a factor of 10 if ASAN is enabled.
- DEFAULT_TIMEOUT = 10 * (10 if ("ASAN_OPTIONS" in os.environ) else 1)
+ DEFAULT_TIMEOUT = dap_server.DEFAULT_TIMEOUT
NO_DEBUG_INFO_TESTCASE = True
def create_debug_adapter(
@@ -118,11 +118,9 @@ def set_function_breakpoints(
self.wait_for_breakpoints_to_resolve(breakpoint_ids)
return breakpoint_ids
- def wait_for_breakpoints_to_resolve(
- self, breakpoint_ids: list[str], timeout: Optional[float] = DEFAULT_TIMEOUT
- ):
+ def wait_for_breakpoints_to_resolve(self, breakpoint_ids: list[str]):
unresolved_breakpoints = self.dap_server.wait_for_breakpoints_to_be_verified(
- breakpoint_ids, timeout
+ breakpoint_ids
)
self.assertEqual(
len(unresolved_breakpoints),
@@ -134,11 +132,10 @@ def wait_until(
self,
predicate: Callable[[], bool],
delay: float = 0.5,
- timeout: float = DEFAULT_TIMEOUT,
) -> bool:
"""Repeatedly run the predicate until either the predicate returns True
or a timeout has occurred."""
- deadline = time.monotonic() + timeout
+ deadline = time.monotonic() + self.DEFAULT_TIMEOUT
while deadline > time.monotonic():
if predicate():
return True
@@ -155,15 +152,13 @@ def assertCapabilityIsNotSet(self, key: str, msg: Optional[str] = None) -> None:
if key in self.dap_server.capabilities:
self.assertEqual(self.dap_server.capabilities[key], False, msg)
- def verify_breakpoint_hit(
- self, breakpoint_ids: List[Union[int, str]], timeout: float = DEFAULT_TIMEOUT
- ):
+ def verify_breakpoint_hit(self, breakpoint_ids: List[Union[int, str]]):
"""Wait for the process we are debugging to stop, and verify we hit
any breakpoint location in the "breakpoint_ids" array.
"breakpoint_ids" should be a list of breakpoint ID strings
(["1", "2"]). The return value from self.set_source_breakpoints()
or self.set_function_breakpoints() can be passed to this function"""
- stopped_events = self.dap_server.wait_for_stopped(timeout)
+ stopped_events = self.dap_server.wait_for_stopped()
normalized_bp_ids = [str(b) for b in breakpoint_ids]
for stopped_event in stopped_events:
if "body" in stopped_event:
@@ -186,11 +181,11 @@ def verify_breakpoint_hit(
f"breakpoint not hit, wanted breakpoint_ids {breakpoint_ids} in stopped_events {stopped_events}",
)
- def verify_all_breakpoints_hit(self, breakpoint_ids, timeout=DEFAULT_TIMEOUT):
+ def verify_all_breakpoints_hit(self, breakpoint_ids):
"""Wait for the process we are debugging to stop, and verify we hit
all of the breakpoint locations in the "breakpoint_ids" array.
"breakpoint_ids" should be a list of int breakpoint IDs ([1, 2])."""
- stopped_events = self.dap_server.wait_for_stopped(timeout)
+ stopped_events = self.dap_server.wait_for_stopped()
for stopped_event in stopped_events:
if "body" in stopped_event:
body = stopped_event["body"]
@@ -208,12 +203,12 @@ def verify_all_breakpoints_hit(self, breakpoint_ids, timeout=DEFAULT_TIMEOUT):
return
self.assertTrue(False, f"breakpoints not hit, stopped_events={stopped_events}")
- def verify_stop_exception_info(self, expected_description, timeout=DEFAULT_TIMEOUT):
+ def verify_stop_exception_info(self, expected_description):
"""Wait for the process we are debugging to stop, and verify the stop
reason is 'exception' and that the description matches
'expected_description'
"""
- stopped_events = self.dap_server.wait_for_stopped(timeout)
+ stopped_events = self.dap_server.wait_for_stopped()
for stopped_event in stopped_events:
if "body" in stopped_event:
body = stopped_event["body"]
@@ -338,26 +333,14 @@ def get_console(self):
def get_important(self):
return self.dap_server.get_output("important")
- def collect_stdout(
- self, timeout: float = DEFAULT_TIMEOUT, pattern: Optional[str] = None
- ) -> str:
- return self.dap_server.collect_output(
- "stdout", timeout=timeout, pattern=pattern
- )
+ def collect_stdout(self, pattern: Optional[str] = None) -> str:
+ return self.dap_server.collect_output("stdout", pattern=pattern)
- def collect_console(
- self, timeout: float = DEFAULT_TIMEOUT, pattern: Optional[str] = None
- ) -> str:
- return self.dap_server.collect_output(
- "console", timeout=timeout, pattern=pattern
- )
+ def collect_console(self, pattern: Optional[str] = None) -> str:
+ return self.dap_server.collect_output("console", pattern=pattern)
- def collect_important(
- self, timeout: float = DEFAULT_TIMEOUT, pattern: Optional[str] = None
- ) -> str:
- return self.dap_server.collect_output(
- "important", timeout=timeout, pattern=pattern
- )
+ def collect_important(self, pattern: Optional[str] = None) -> str:
+ return self.dap_server.collect_output("important", pattern=pattern)
def get_local_as_int(self, name, threadId=None):
value = self.dap_server.get_local_variable_value(name, threadId=threadId)
@@ -393,14 +376,13 @@ def stepIn(
targetId=None,
waitForStop=True,
granularity="statement",
- timeout=DEFAULT_TIMEOUT,
):
response = self.dap_server.request_stepIn(
threadId=threadId, targetId=targetId, granularity=granularity
)
self.assertTrue(response["success"])
if waitForStop:
- return self.dap_server.wait_for_stopped(timeout)
+ return self.dap_server.wait_for_stopped()
return None
def stepOver(
@@ -408,7 +390,6 @@ def stepOver(
threadId=None,
waitForStop=True,
granularity="statement",
- timeout=DEFAULT_TIMEOUT,
):
response = self.dap_server.request_next(
threadId=threadId, granularity=granularity
@@ -417,40 +398,40 @@ def stepOver(
response["success"], f"next request failed: response {response}"
)
if waitForStop:
- return self.dap_server.wait_for_stopped(timeout)
+ return self.dap_server.wait_for_stopped()
return None
- def stepOut(self, threadId=None, waitForStop=True, timeout=DEFAULT_TIMEOUT):
+ def stepOut(self, threadId=None, waitForStop=True):
self.dap_server.request_stepOut(threadId=threadId)
if waitForStop:
- return self.dap_server.wait_for_stopped(timeout)
+ return self.dap_server.wait_for_stopped()
return None
def do_continue(self): # `continue` is a keyword.
resp = self.dap_server.request_continue()
self.assertTrue(resp["success"], f"continue request failed: {resp}")
- def continue_to_next_stop(self, timeout=DEFAULT_TIMEOUT):
+ def continue_to_next_stop(self):
self.do_continue()
- return self.dap_server.wait_for_stopped(timeout)
+ return self.dap_server.wait_for_stopped()
- def continue_to_breakpoint(self, breakpoint_id: str, timeout=DEFAULT_TIMEOUT):
- self.continue_to_breakpoints((breakpoint_id), timeout)
+ def continue_to_breakpoint(self, breakpoint_id: str):
+ self.continue_to_breakpoints((breakpoint_id))
- def continue_to_breakpoints(self, breakpoint_ids, timeout=DEFAULT_TIMEOUT):
+ def continue_to_breakpoints(self, breakpoint_ids):
self.do_continue()
- self.verify_breakpoint_hit(breakpoint_ids, timeout)
+ self.verify_breakpoint_hit(breakpoint_ids)
- def continue_to_exception_breakpoint(self, filter_label, timeout=DEFAULT_TIMEOUT):
+ def continue_to_exception_breakpoint(self, filter_label):
self.do_continue()
self.assertTrue(
- self.verify_stop_exception_info(filter_label, timeout),
+ self.verify_stop_exception_info(filter_label),
'verify we got "%s"' % (filter_label),
)
- def continue_to_exit(self, exitCode=0, timeout=DEFAULT_TIMEOUT):
+ def continue_to_exit(self, exitCode=0):
self.do_continue()
- stopped_events = self.dap_server.wait_for_stopped(timeout)
+ stopped_events = self.dap_server.wait_for_stopped()
self.assertEqual(
len(stopped_events), 1, "stopped_events = {}".format(stopped_events)
)
diff --git a/lldb/test/API/tools/lldb-dap/attach-commands/TestDAP_attachCommands.py b/lldb/test/API/tools/lldb-dap/attach-commands/TestDAP_attachCommands.py
index ed373f2c427a9..9e29f07db80f1 100644
--- a/lldb/test/API/tools/lldb-dap/attach-commands/TestDAP_attachCommands.py
+++ b/lldb/test/API/tools/lldb-dap/attach-commands/TestDAP_attachCommands.py
@@ -71,7 +71,7 @@ def test_commands(self):
breakpoint_ids = self.set_function_breakpoints(functions)
self.assertEqual(len(breakpoint_ids), len(functions), "expect one breakpoint")
self.continue_to_breakpoints(breakpoint_ids)
- output = self.collect_console(timeout=10, pattern=stopCommands[-1])
+ output = self.collect_console(pattern=stopCommands[-1])
self.verify_commands("stopCommands", output, stopCommands)
# Continue after launch and hit the "pause()" call and stop the target.
@@ -81,7 +81,7 @@ def test_commands(self):
time.sleep(0.5)
self.dap_server.request_pause()
self.dap_server.wait_for_stopped()
- output = self.collect_console(timeout=10, pattern=stopCommands[-1])
+ output = self.collect_console(pattern=stopCommands[-1])
self.verify_commands("stopCommands", output, stopCommands)
# Continue until the program exits
@@ -90,7 +90,6 @@ def test_commands(self):
# "exitCommands" that were run after the second breakpoint was hit
# and the "terminateCommands" due to the debugging session ending
output = self.collect_console(
- timeout=10.0,
pattern=terminateCommands[0],
)
self.verify_commands("exitCommands", output, exitCommands)
@@ -141,7 +140,6 @@ def test_terminate_commands(self):
# "terminateCommands"
self.dap_server.request_disconnect(terminateDebuggee=True)
output = self.collect_console(
- timeout=1.0,
pattern=terminateCommands[0],
)
self.verify_commands("terminateCommands", output, terminateCommands)
diff --git a/lldb/test/API/tools/lldb-dap/breakpoint-events/TestDAP_breakpointEvents.py b/lldb/test/API/tools/lldb-dap/breakpoint-events/TestDAP_breakpointEvents.py
index 151ad761a5044..beab4d6c1f5a6 100644
--- a/lldb/test/API/tools/lldb-dap/breakpoint-events/TestDAP_breakpointEvents.py
+++ b/lldb/test/API/tools/lldb-dap/breakpoint-events/TestDAP_breakpointEvents.py
@@ -82,14 +82,14 @@ def test_breakpoint_events(self):
)
# Flush the breakpoint events.
- self.dap_server.wait_for_breakpoint_events(timeout=5)
+ self.dap_server.wait_for_breakpoint_events()
# Continue to the breakpoint
self.continue_to_breakpoints(dap_breakpoint_ids)
verified_breakpoint_ids = []
unverified_breakpoint_ids = []
- for breakpoint_event in self.dap_server.wait_for_breakpoint_events(timeout=5):
+ for breakpoint_event in self.dap_server.wait_for_breakpoint_events():
breakpoint = breakpoint_event["body"]["breakpoint"]
id = breakpoint["id"]
if breakpoint["verified"]:
diff --git a/lldb/test/API/tools/lldb-dap/commands/TestDAP_commands.py b/lldb/test/API/tools/lldb-dap/commands/TestDAP_commands.py
index e61d2480ea4bb..f53813a8a48f6 100644
--- a/lldb/test/API/tools/lldb-dap/commands/TestDAP_commands.py
+++ b/lldb/test/API/tools/lldb-dap/commands/TestDAP_commands.py
@@ -23,7 +23,6 @@ def test_command_directive_quiet_on_success(self):
exitCommands=["?" + command_quiet, command_not_quiet],
)
full_output = self.collect_console(
- timeout=1.0,
pattern=command_not_quiet,
)
self.assertNotIn(command_quiet, full_output)
@@ -51,7 +50,6 @@ def do_test_abort_on_error(
expectFailure=True,
)
full_output = self.collect_console(
- timeout=1.0,
pattern=command_abort_on_error,
)
self.assertNotIn(command_quiet, full_output)
diff --git a/lldb/test/API/tools/lldb-dap/module-event/TestDAP_module_event.py b/lldb/test/API/tools/lldb-dap/module-event/TestDAP_module_event.py
index bb835af12f5ef..1f4afabbd161e 100644
--- a/lldb/test/API/tools/lldb-dap/module-event/TestDAP_module_event.py
+++ b/lldb/test/API/tools/lldb-dap/module-event/TestDAP_module_event.py
@@ -23,15 +23,15 @@ def test_module_event(self):
self.continue_to_breakpoints(breakpoint_ids)
# We're now stopped at breakpoint 1 before the dlopen. Flush all the module events.
- event = self.dap_server.wait_for_event(["module"], 0.25)
+ event = self.dap_server.wait_for_event(["module"])
while event is not None:
- event = self.dap_server.wait_for_event(["module"], 0.25)
+ event = self.dap_server.wait_for_event(["module"])
# Continue to the second breakpoint, before the dlclose.
self.continue_to_breakpoints(breakpoint_ids)
# Make sure we got a module event for libother.
- event = self.dap_server.wait_for_event(["module"], 5)
+ event = self.dap_server.wait_for_event(["module"])
self.assertIsNotNone(event, "didn't get a module event")
module_name = event["body"]["module"]["name"]
module_id = event["body"]["module"]["id"]
@@ -42,7 +42,7 @@ def test_module_event(self):
self.continue_to_breakpoints(breakpoint_ids)
# Make sure we got a module event for libother.
- event = self.dap_server.wait_for_event(["module"], 5)
+ event = self.dap_server.wait_for_event(["module"])
self.assertIsNotNone(event, "didn't get a module event")
reason = event["body"]["reason"]
self.assertEqual(r...
[truncated]
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to follow this up with some other refactors.
One that I think we should consider is the wait_for_events helper. I think thats a bit of an anti-pattern because we could already be in the correct state we're waiting for.
For example, wait_for_stopped could be refactored into:
@property
def is_stopped(self) -> bool: ...
def wait_for_stopped(self, timeout: Optional[float] = None) -> None:
self._run_until(predicate=lambda: self.is_stopped)
which moves this away from the timing of events and towards checking the current state of the DebugCommunication object.
| def verify_breakpoint_hit( | ||
| self, breakpoint_ids: List[Union[int, str]], timeout: float = DEFAULT_TIMEOUT | ||
| ): | ||
| def verify_breakpoint_hit(self, breakpoint_ids: List[Union[int, str]]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reminds me, I want to revisit breakpoint id handling.
We're inconsistent in handling breakpoint ids. Some APIs take them as strings, some convert them to strings and some take them as ints. They're an int in the spec, but I'm not sure why we're converting them into strings...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, this has long bothered me as well.
| # set timeout based on whether ASAN was enabled or not. Increase | ||
| # timeout by a factor of 10 if ASAN is enabled. | ||
| DEFAULT_TIMEOUT = 10 * (10 if ("ASAN_OPTIONS" in os.environ) else 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is one other case where we may want a longer timeout.
When we're using a CMAKE_BUILD_TYPE=Debug the tests do have more timeouts. I think we've tagged a few places, but we could also increase the timeout for those as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this probably warrants a centralized helper, maybe do 2x for debug builds, 10x for sanitized builds, etc.
| self, | ||
| *, | ||
| predicate: Optional[Callable[[ProtocolMessage], bool]] = None, | ||
| timeout: Optional[float] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have this use the default? If we want to make this wait forever we could override the timeout=None, which should cause the _recv_condition to never timeout.
Various DAP tests are specifying their own timeouts, with values ranging from "1" to "20". Most of them seem arbitrary, but some come with a comment.
The performance characters of running these tests in CI are unpredictable (they generally run much slower than developers expect) and really not something we can make assumptions about. I suspect these timeouts are a contributing factor to the flakiness of the DAP tests.
This PR unifies the timeouts around a central value in the DAP server.
Fixes #162523