[Lldb-commits] [PATCH] D122193: Reland "[lldb/test] Add events listener helper class to lldbtest"

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added a comment.

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

In D122193#3399109 <https://reviews.llvm.org/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 related to state change.

> - 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 it 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 <https://reviews.llvm.org/rG8bf893466632cc2597188b431160effcd8cedeef>
>
> 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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122193/new/

https://reviews.llvm.org/D122193