[lldb-dap] Add progress events to the packet list

[JDevlieghere]

Before #134048, TestDAP_Progress relied on wait_for_event to block until
the progressEnd came in. However, progress events were not added to the
packet list, so this call would always time out. This PR makes it so
that packets are added to the packet list, and you can block on them.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)

Changes

Before #134048, TestDAP_Progress would wait up to 15 seconds before checking whether the events came in. Now the test is a lot faster, but we risk checking the events before they've all arrived. Make up to 10 attempts to find the end event, with a 100ms sleep in between, giving the test up to a second for the event to arrive after it has been broadcast.

---

Full diff: https://github.com/llvm/llvm-project/pull/134157.diff

1 Files Affected:

• (modified) lldb/test/API/tools/lldb-dap/progress/TestDAP_Progress.py (+18-5)

diff --git a/lldb/test/API/tools/lldb-dap/progress/TestDAP_Progress.py b/lldb/test/API/tools/lldb-dap/progress/TestDAP_Progress.py
index ffe3d38eb49a3..48bb04abc9c01 100755
--- a/lldb/test/API/tools/lldb-dap/progress/TestDAP_Progress.py
+++ b/lldb/test/API/tools/lldb-dap/progress/TestDAP_Progress.py
@@ -12,6 +12,8 @@
 
 
 class TestDAP_progress(lldbdap_testcase.DAPTestCaseBase):
+    MAX_ATTEMPS = 10
+
     def verify_progress_events(
         self,
         expected_title,
@@ -19,10 +21,22 @@ def verify_progress_events(
         expected_not_in_message=None,
         only_verify_first_update=False,
     ):
-        self.assertTrue(len(self.dap_server.progress_events) > 0)
+        # Make up to 10 attempts (= 1 second) for the end event to arrive.
+        end_found = False
+        for _ in range(self.MAX_ATTEMPS):
+            for event in self.dap_server.progress_events:
+                event_type = event["event"]
+                if "progressEnd" in event_type:
+                    end_found = True
+                    break
+            # Wait 100ms before checking again.
+            time.sleep(0.10)
+        self.assertTrue(end_found)
+
+        # Make sure we found the start and update event. If we got this far, we
+        # already know we got an end event.
         start_found = False
         update_found = False
-        end_found = False
         for event in self.dap_server.progress_events:
             event_type = event["event"]
             if "progressStart" in event_type:
@@ -38,12 +52,11 @@ def verify_progress_events(
                 if expected_not_in_message is not None:
                     self.assertNotIn(expected_not_in_message, message)
                 update_found = True
-            if "progressEnd" in event_type:
-                end_found = True
 
         self.assertTrue(start_found)
         self.assertTrue(update_found)
-        self.assertTrue(end_found)
+
+        # Clear the progress events.
         self.dap_server.progress_events.clear()
 
     @skipIfWindows

[Jlalond]

LGTM Round 2, left a comment this time on unifying this logic into the test base (Because I thought this is what the wait call already did!)

[Jlalond]

Because we're in effect building a new state machine, wouldn't it make sense to add this to the underlying test case base? I was under the impression that wait for event would return quickly if progress already existed. I think having a function in the test base for this would be very useful. Plus, the underlying class leverages some concurrency primitives that we could reuse correct?

If you don't want to implement that, you can always kick it back to me to move this logic into the base class.

[JDevlieghere]

Yeah, that's a good point. Here's my understanding why how this works:

We have a packet read thread (read_packet_thread) which looks at incoming packets. Certain packets are recognized and added to lists (e.g. the progress_events) and they don't get added to the packets list, which means you can't wait on them:

            elif event.startswith("progress"):
                # Progress events come in as 'progressStart', 'progressUpdate',
                # and 'progressEnd' events. Keep these around in case test
                # cases want to verify them.
                self.progress_events.append(packet)
                # No need to add 'progress' event packets to our packets list.
                return keepGoing

If we wanted to make this work with wait_for_event, we can remove the last two lines and that way, we'll block until the packet is received. I actually like that approach better. Let me update the PR.

[JDevlieghere]

@Jlalond changed the approach and the description. Please take another look.

[Jlalond]

Much improved! I should've noticed this in my first patch but great fix for the flakiness