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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D122193#3399921 <https://reviews.llvm.org/D122193#3399921>, @mib wrote:

> 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 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).


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

https://reviews.llvm.org/D122193