[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 ↗

(On Diff #417474)

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 ↗

(On Diff #417474)

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/eventlistener.py

+import lldb
+from lldbsuite.test.lldbtest import *
+
+class EventListenerTestBase(TestBase):
+
+    """
+    Base class for lldb event listener tests.
+
+    This class will initialize a listener for a particular event_mask, so when
+    the user calls `assertEvent`, it starts a blocking event listening loop
+    that stops after calling a user-provided callback on a matching event.
+
+    This makes the class mostly caller-driven, as it only fetches one matching
+    event, per `assertEvent` call.
+    """
+    NO_DEBUG_INFO_TESTCASE = True
+
+    def setUp(self, event_mask):
+        TestBase.setUp(self)
+
+        self.listener = lldb.SBListener("lldb.test.listener")
+
+        self.src_broadcaster = self.dbg.GetBroadcaster()
+        self.src_broadcaster.AddListener(self.listener, event_mask)
+
+    def assertEvent(self, callback):
+        event = lldb.SBEvent()
+
+        while True:
+            if self.listener.GetNextEvent(event):
+                if event.BroadcasterMatchesRef(self.src_broadcaster):
+                    callback(self, event)
+                    return
+                continue

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 *
+from lldbsuite.test.eventlistener import EventListenerTestBase
 
-class TestDiagnosticReporting(TestBase):
+class TestDiagnosticReporting(EventListenerTestBase):
 
     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))
+        EventListenerTestBase.setUp(self,
+                                    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]
+        def event_checker(self, event):
+            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"
             )
+
+        self.assertEvent(event_checker)
+

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
 
-class TestProgressReporting(TestBase):
+import lldbsuite.test.lldbutil as lldbutil
 
-    mydir = TestBase.compute_mydir(__file__)
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.eventlistener import EventListenerTestBase
 
-    eBroadcastBitStopProgressThread = (1 << 0)
 
-    def setUp(self):
-        TestBase.setUp(self)
-        self.progress_events = []
+class TestProgressReporting(EventListenerTestBase):
 
-    def fetch_events(self):
-        event = lldb.SBEvent()
+    mydir = TestBase.compute_mydir(__file__)
 
-        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))
+    def setUp(self):
+        EventListenerTestBase.setUp(self, 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()
+        def event_checker(self, event):
+            ret_args = lldb.SBDebugger.GetProgressFromEvent(event)
+            self.assertTrue(ret_args)
 
-        self.assertGreater(len(self.progress_events), 0)
+        self.assertEvent(event_checker)