[lldb/test] Add events listener helper function to lldbtest

[mib]

Summary

This patch introduces 2 new lldb utility functions:

• lldbutil.start_listening_from: This can be called in the test setup to create a listener and set it up for a specific event mask and add it to the user-provided broadcaster's list.
• lldbutil.fetch_next_event: This will use fetch a single event from the provided istener and return it if it matches the provided broadcaster.

The motivation behind this is to easily test new kinds of events
(i.e. Swift type-system progress events). However, this patch also
updates TestProgressReporting.py and TestDiagnosticReporting.py
to make use of these new helper functions.

Signed-off-by: Med Ismail Bennani <[e-mail]>

[mib]

Reflow docstring

[labath]

I like this implementation a lot. The less threads we use, the better. I have a couple of remarks/questions though:

• given how simple the new approach is, is a dedicated test class really needed? It seems like it should be sufficient to add two utility functions to: create a listener; and fetch an event from it. This would be more flexible, and could be placed next to lldbutil.expect_state_changes which is our existing helper function for dealing with state-changed events
• instead of taking a callback, I think it would be simpler if the event retrieval function just returned the event, and let the caller do what ever it deems fit
• listener.GetNextEvent will now wait indefinitely if the event is not sent (due to a bug). In this setup I'd replace it with WaitForEvent(timeout), where timeout can be zero if we can guarantee that the event has been sent by the time we call the event retrieval function (this would be ideal as failures would be instantaneous), or a reasonably large value (but less than infinity) if we can't. I think the first option should be possible since that's essentially what eBroadcastBitStopDiagnosticThread was doing.

This reverts commit 8bf893466632cc2597188b431160effcd8cedeef

If you hadn't said that I don't think anyone would notice, as the implementation is completely different. :)

[mib]

Hey @labath, thanks for the feedback! Here are some other questions ?

In D122193#3399109, @labath wrote:

I like this implementation a lot. The less threads we use, the better. I have a couple of remarks/questions though:

• given how simple the new approach is, is a dedicated test class really needed? It seems like it should be sufficient to add two utility functions to: create a listener; and fetch an event from it. This would be more flexible, and could be placed next to lldbutil.expect_state_changes which is our existing helper function for dealing with state-changed events

I think we could replace the class by a function. Also, at the moment, I'm not sure the tested events (progress/diagnostic) are really related to state change (regarding where this should go).

• instead of taking a callback, I think it would be simpler if the event retrieval function just returned the event, and let the caller do what ever it deems fit

Should we remove the assertEvent method and let the users handle the check themselves by returning the event ?

• listener.GetNextEvent will now wait indefinitely if the event is not sent (due to a bug). In this setup I'd replace it with WaitForEvent(timeout), where timeout can be zero if we can guarantee that the event has been sent by the time we call the event retrieval function (this would be ideal as failures would be instantaneous), or a reasonably large value (but less than infinity) if we can't. I think the first option should be possible since that's essentially what eBroadcastBitStopDiagnosticThread was doing.

I expected to use the test suite timeout to stop the infinite loop, but with this suggestion and the previous one, what should the function return after timing out ?

This reverts commit 8bf893466632cc2597188b431160effcd8cedeef

If you hadn't said that I don't think anyone would notice, as the implementation is completely different. :)

Right :p but I wanted to keep the history

[labath]

In D122193#3399921, @mib wrote:

Hey @labath, thanks for the feedback! Here are some other questions ?

In D122193#3399109, @labath wrote:

I like this implementation a lot. The less threads we use, the better. I have a couple of remarks/questions though:

• given how simple the new approach is, is a dedicated test class really needed? It seems like it should be sufficient to add two utility functions to: create a listener; and fetch an event from it. This would be more flexible, and could be placed next to lldbutil.expect_state_changes which is our existing helper function for dealing with state-changed events

I think we could replace the class by a function. Also, at the moment, I'm not sure the tested events (progress/diagnostic) are really related to state change (regarding where this should go).

They're not related to state change, but they are related to events. My point was simply that it'd be nice to have event-handling code grouped together.

• instead of taking a callback, I think it would be simpler if the event retrieval function just returned the event, and let the caller do what ever it deems fit

Should we remove the assertEvent method and let the users handle the check themselves by returning the event ?

Yes, that's the general idea.

• listener.GetNextEvent will now wait indefinitely if the event is not sent (due to a bug). In this setup I'd replace it with WaitForEvent(timeout), where timeout can be zero if we can guarantee that the event has been sent by the time we call the event retrieval function (this would be ideal as failures would be instantaneous), or a reasonably large value (but less than infinity) if we can't. I think the first option should be possible since that's essentially what eBroadcastBitStopDiagnosticThread was doing.

I expected to use the test suite timeout to stop the infinite loop, but with this suggestion and the previous one, what should the function return after timing out ?

The test suite timeout is rather large and relies on the an optional python module. I think it would be better to have a separate timeout here, which would also give a better error message

The function can just fail if the event retrieval times out. Most of the lldbutil methods achieve that by taking an test object so they can call some unittest methods on (fail(msg) would probably be appropriate here).

[mib]

Address @labath comments:

• Split the class into 2 functions
• Let the user handle the event

[labath]

lldb/packages/Python/lldbsuite/test/lldbutil.py

1265

1 is definitely not a good default. If you can't/don't want to use zero, then I'd recommend at least 10 seconds.

1273

else:
  test.fail("got event '%s' from unexpected broadcaster '%s'%(event.GetDescription(), event.GetBroadcaster().GetName())

(or something along those line)

lldb/packages/Python/lldbsuite/test/lldbutil.py

for expected_state in states:
                 lldb.eStateRunning,
                 "Restarted event followed by a running event")
             event = get_next_event()
 
         test.assertEqual(
             lldb.SBProcess.GetStateFromEvent(event),
             expected_state)
 
+def start_listening_from(broadcaster, event_mask):
+    """Creates a listener for a specific event mask and add it to the source broadcaster."""
+
+    listener = lldb.SBListener("lldb.test.listener")
+    broadcaster.AddListener(listener, event_mask)
+    return listener
+
+def fetch_next_event(test, listener, broadcaster, timeout=10):
+    """Fetch one event from the listener and return it if it matches the provided broadcaster.
+    Fails otherwise."""
+
+    event = lldb.SBEvent()
+
+    if listener.WaitForEvent(timeout, event):
+        if event.BroadcasterMatchesRef(broadcaster):
+            return event
+
+        test.fail("received event '%s' from unexpected broadcaster '%s'." %
+                  (event.GetDescription(), event.GetBroadcaster().GetName()))
+
+    test.fail("couldn't fetch an event before reaching the timeout.")
+
+
 # ===================================
 # Utility functions related to Frames
 # ===================================
 
 
 def get_parent_frame(frame):
     """
     Returns the parent frame of the input frame object; None if not available.

lldb/test/API/functionalities/diagnostic_reporting/TestDiagnosticReporting.py

 """
 Test that we are able to broadcast and receive diagnostic events from lldb
 """
 import lldb
-from lldbsuite.test.lldbtest import *
-from lldbsuite.test.decorators import *
+
 import lldbsuite.test.lldbutil as lldbutil
-import threading
 
+from lldbsuite.test.lldbtest import *
 
 class TestDiagnosticReporting(TestBase):
 
     mydir = TestBase.compute_mydir(__file__)
 
-    eBroadcastBitStopDiagnosticThread = (1 << 0)
-
     def setUp(self):
         TestBase.setUp(self)
-        self.diagnostic_events = []
 
-    def fetch_events(self):
-        event = lldb.SBEvent()
-
-        done = False
-        while not done:
-            if self.listener.WaitForEvent(1, event):
-                event_mask = event.GetType()
-                if event.BroadcasterMatchesRef(self.test_broadcaster):
-                    if event_mask & self.eBroadcastBitStopDiagnosticThread:
-                        done = True
-                elif event.BroadcasterMatchesRef(self.diagnostic_broadcaster):
-                    self.diagnostic_events.append(
-                        lldb.SBDebugger.GetDiagnosticFromEvent(event))
+        self.broadcaster = self.dbg.GetBroadcaster()
+        self.listener = lldbutil.start_listening_from(self.broadcaster,
+                                        lldb.SBDebugger.eBroadcastBitWarning |
+                                        lldb.SBDebugger.eBroadcastBitError)
 
     def test_dwarf_symbol_loading_diagnostic_report(self):
         """Test that we are able to fetch diagnostic events"""
-        self.listener = lldb.SBListener("lldb.diagnostic.listener")
-        self.test_broadcaster = lldb.SBBroadcaster('lldb.broadcaster.test')
-        self.listener.StartListeningForEvents(
-            self.test_broadcaster, self.eBroadcastBitStopDiagnosticThread)
-
-        self.diagnostic_broadcaster = self.dbg.GetBroadcaster()
-        self.diagnostic_broadcaster.AddListener(
-            self.listener, lldb.SBDebugger.eBroadcastBitWarning)
-        self.diagnostic_broadcaster.AddListener(
-            self.listener, lldb.SBDebugger.eBroadcastBitError)
-
-        listener_thread = threading.Thread(target=self.fetch_events)
-        listener_thread.start()
 
         self.yaml2obj("minidump.yaml", self.getBuildArtifact("minidump.core"))
 
         self.dbg.CreateTarget(None)
         self.target = self.dbg.GetSelectedTarget()
         self.process = self.target.LoadCore(
             self.getBuildArtifact("minidump.core"))
 
-        self.test_broadcaster.BroadcastEventByType(
-            self.eBroadcastBitStopDiagnosticThread)
-        listener_thread.join()
-
-        self.assertEquals(len(self.diagnostic_events), 1)
-
-        diagnostic_event = self.diagnostic_events[0]
+        event = lldbutil.fetch_next_event(self, self.listener, self.broadcaster)
+        diagnostic_data = lldb.SBDebugger.GetDiagnosticFromEvent(event)
         self.assertEquals(
-            diagnostic_event.GetValueForKey("type").GetStringValue(100),
+            diagnostic_data.GetValueForKey("type").GetStringValue(100),
             "warning")
         self.assertEquals(
-            diagnostic_event.GetValueForKey("message").GetStringValue(100),
+            diagnostic_data.GetValueForKey("message").GetStringValue(100),
             "unable to retrieve process ID from minidump file, setting process ID to 1"
         )

lldb/test/API/functionalities/progress_reporting/TestProgressReporting.py

 """
 Test that we are able to broadcast and receive progress events from lldb
 """
 import lldb
-from lldbsuite.test.lldbtest import *
-from lldbsuite.test.decorators import *
+
 import lldbsuite.test.lldbutil as lldbutil
-import threading
+
+from lldbsuite.test.lldbtest import *
+
 
 class TestProgressReporting(TestBase):
 
     mydir = TestBase.compute_mydir(__file__)
 
-    eBroadcastBitStopProgressThread = (1 << 0)
-
     def setUp(self):
         TestBase.setUp(self)
-        self.progress_events = []
-
-    def fetch_events(self):
-        event = lldb.SBEvent()
-
-        done = False
-        while not done:
-            if self.listener.WaitForEvent(1, event):
-                event_mask = event.GetType();
-                if event.BroadcasterMatchesRef(self.test_broadcaster):
-                    if event_mask & self.eBroadcastBitStopProgressThread:
-                        done = True;
-                elif event.BroadcasterMatchesRef(self.progress_broadcaster):
-                    ret_args = lldb.SBDebugger().GetProgressFromEvent(event);
-                    self.assertGreater(len(ret_args), 1)
-
-                    message = ret_args[0]
-                    if message:
-                        self.progress_events.append((message, event))
+        self.broadcaster = self.dbg.GetBroadcaster()
+        self.listener = lldbutil.start_listening_from(self.broadcaster,
+                                        lldb.SBDebugger.eBroadcastBitProgress)
 
     def test_dwarf_symbol_loading_progress_report(self):
         """Test that we are able to fetch dwarf symbol loading progress events"""
         self.build()
 
-        self.listener = lldb.SBListener("lldb.progress.listener")
-        self.test_broadcaster = lldb.SBBroadcaster('lldb.broadcaster.test')
-        self.listener.StartListeningForEvents(self.test_broadcaster,
-                                              self.eBroadcastBitStopProgressThread)
-
-        self.progress_broadcaster = self.dbg.GetBroadcaster()
-        self.progress_broadcaster.AddListener(self.listener, lldb.SBDebugger.eBroadcastBitProgress)
-
-        listener_thread = threading.Thread(target=self.fetch_events)
-        listener_thread.start()
-
         lldbutil.run_to_source_breakpoint(self, 'break here', lldb.SBFileSpec('main.c'))
 
-        self.test_broadcaster.BroadcastEventByType(self.eBroadcastBitStopProgressThread)
-        listener_thread.join()
+        event = lldbutil.fetch_next_event(self, self.listener, self.broadcaster)
+        ret_args = lldb.SBDebugger.GetProgressFromEvent(event)
+        self.assertGreater(len(ret_args), 0)
+        message = ret_args[0]
+        self.assertGreater(len(message), 0)
 
-        self.assertGreater(len(self.progress_events), 0)