This is an archive of the discontinued LLVM Phabricator instance.

[lldb/test] Add events listener helper class to lldbtest
ClosedPublic

Authored by mib on Mar 17 2022, 11:14 PM.

Details

Summary

This patch introduces a generic helper class that will listen for
event in a background thread and match it against a source broadcaster.

If the event received matches the source broadcaster, the event is
queued up in a list that the user can access later on.

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 this new helper class.

Signed-off-by: Med Ismail Bennani <medismail.bennani@gmail.com>

Diff Detail

Event Timeline

mib created this revision.Mar 17 2022, 11:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2022, 11:14 PM
mib requested review of this revision.Mar 17 2022, 11:14 PM
mib edited the summary of this revision. (Show Details)Mar 17 2022, 11:15 PM
mib edited the summary of this revision. (Show Details)
mib updated this revision to Diff 416396.Mar 17 2022, 11:17 PM

Reformat

mib edited the summary of this revision. (Show Details)Mar 17 2022, 11:19 PM
labath accepted this revision.Mar 17 2022, 11:59 PM
labath added inline comments.
lldb/packages/Python/lldbsuite/test/lldbutil.py
1635 ↗(On Diff #416396)

This contains way too many underscores. An appropriate name for a private function would be _fetch_events.

1640 ↗(On Diff #416396)

If you're doing an infinite loop, you might as well use a blocking call.

This revision is now accepted and ready to land.Mar 17 2022, 11:59 PM
JDevlieghere added inline comments.Mar 18 2022, 10:51 AM
lldb/packages/Python/lldbsuite/test/lldbutil.py
1624 ↗(On Diff #416396)

We should make this more generic so that the warning and error events can also use this. The way I imagined this was that the thread would be generic and you would pass it in the broadcaster, the listener and the event type and finally a function callback to have the test do whatever it wants with the event.

JDevlieghere requested changes to this revision.Mar 18 2022, 10:51 AM
This revision now requires changes to proceed.Mar 18 2022, 10:51 AM
mib marked an inline comment as done.Mar 18 2022, 11:33 AM
mib added inline comments.
lldb/packages/Python/lldbsuite/test/lldbutil.py
1624 ↗(On Diff #416396)

@JDevlieghere I think this could be achieved very easily by assuming the _fetch_events method will always be called and replace the callback argument by a "pointer" to the event_data_extractor function. wdyk ?'

Do you think it's a fair assumption (that we should always call _fetch_events and match the event against the src_broadcaster argument) ?

JDevlieghere added inline comments.Mar 18 2022, 11:45 AM
lldb/packages/Python/lldbsuite/test/lldbutil.py
1624 ↗(On Diff #416396)

Yes, that's one way to do it. I'm happy as long as we can use it for both TestDiagnosticReporting.py and TestProgressReporting.py. I was on the fence about the broadcaster and listener being part of the utility, but I think it can work. The diagnostics are slightly different because they listen for two event types, but I guess we can abstract over that by ORing the types.

If we do go that route, I think we might want to make this a TestBase class instead that starts and stops the event thread at the beginning and end of the test. Something like GDBRemoteTestBase and PExpectTest.

mib marked 2 inline comments as done.Mar 18 2022, 12:38 PM
mib updated this revision to Diff 416581.Mar 18 2022, 12:58 PM

Address @JDevlieghere comments:

  • Move the EventListener class to a separate module and make it inherit from TestBase to start/stop the background listener thread in setUp/tearDown.
mib retitled this revision from [lldb/test] Add progress events listener helper class to lldbutil to [lldb/test] Add events listener helper class to lldbtest.Mar 18 2022, 12:59 PM
mib edited the summary of this revision. (Show Details)
mib updated this revision to Diff 416586.Mar 18 2022, 1:03 PM

Update EventListener.tearDown comment

JDevlieghere added inline comments.Mar 18 2022, 1:15 PM
lldb/packages/Python/lldbsuite/test/eventlistener.py
7

EventListenerTest

20

Just events maybe?

23

Could we avoid the arguments by making these two things properties of the test? I'm trying to avoid the:

TestBase.setUp(self) 
EventListener.setUp(self)

So that tests would have to set:

class TestDiagnosticReporting(EventListenerTest):
  event_mask = lldb.SBDebugger.eBroadcastBitWarning | lldb.SBDebugger.eBroadcastBitError
  event_extractor = lldb.SBDebugger.GetDiagnosticFromEvent

Presumable those two things don't need the debugger and I think we can avoid the self.src_broadcaster too (see below).

24

Would this always be self.dbg.GetBroadcaster()?

jingham added inline comments.
lldb/packages/Python/lldbsuite/test/lldbutil.py
1624 ↗(On Diff #416396)

It might be good to put an error in there if some one tries to do this with process events. That's pretty unlikely to work, since it would fight with the debugger for those events.

mib marked 4 inline comments as done.Mar 18 2022, 2:42 PM
mib added inline comments.
lldb/packages/Python/lldbsuite/test/eventlistener.py
24

For now, it's always the debugger broadcaster so we can assume that I think.

mib updated this revision to Diff 416612.Mar 18 2022, 2:43 PM
mib marked an inline comment as done.

Address @JDevlieghere comments

mib added a comment.Mar 18 2022, 2:51 PM

@jingham It's a bit restrictive, but as I stated before, this class will expect to receive events from the debugger broadcaster. If you can think of other broadcasters that could be tested with this class, please let me know so we can think of another solution.

JDevlieghere accepted this revision.Mar 18 2022, 2:56 PM

Nice, thanks for bearing with me. LGTM.

This revision is now accepted and ready to land.Mar 18 2022, 2:56 PM
mib updated this revision to Diff 416630.Mar 18 2022, 4:34 PM

Make event_mask and event_data_extractor class attributes to remove EventListenerTestBase.setUp overloading in the derived class tests.

This revision was landed with ongoing or failed builds.Mar 18 2022, 4:36 PM
This revision was automatically updated to reflect the committed changes.

I've reverted this patch because it introduces races in the event processing. See inline comment for details.

lldb/packages/Python/lldbsuite/test/eventlistener.py
38–41

This cannot be done in the tearDown method (without extra synchronisation) as this is what guarantees that the events have been processed and can be read from the event array. This is what caused TestDiagnosticReporting.py to flake. If you want to do the shutdown in the tearDown method, then you'll need to introduce some other means of synchronising and ensuring that the event array can be accessed safely and contains all the things it is supposed to contain (e.g. through a new kind of a message).

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

Why not just queue the events themselves and let the user do whatever it wants with them later?

mib marked an inline comment as done.Mar 21 2022, 2:24 PM
mib added inline comments.
lldb/packages/Python/lldbsuite/test/eventlistener.py
38–41

I knew this was going to bite us eventually :/ Should we get back to the start/stop approach ?

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

We still need to setup the event_mask to filter out unwanted events from the source broadcaster.

We would have to set that up in the derived class setUp override.

@JDevlieghere do you have any other suggestion ?

mib marked an inline comment as done.Mar 21 2022, 4:57 PM

Came up with a new approach in D122193