Rework ui_dispatch tests to avoid need for Qt #1792
Conversation
| @@ -615,10 +614,10 @@ class FastUITraitChangeNotifyWrapper(TraitChangeNotifyWrapper): | |||
| """ | |||
|
|
|||
| def dispatch(self, handler, *args): | |||
| if threading.current_thread().ident == ui_thread: | |||
| if threading.current_thread() == threading.main_thread(): | |||
There was a problem hiding this comment.
This is the fix that should have been part of #1740.
| @@ -153,11 +159,7 @@ def _get_handlers(self): | |||
| thread. | |||
| """ | |||
| thread_local = self.thread_local | |||
| if isinstance(thread_local, dict): | |||
There was a problem hiding this comment.
Drive-by cleanup: this condition is never true in modern Python.
|
@flongford Do you have bandwidth for review? This is part of a longer-term goal of insulating Traits from changes in Qt / PySide, and eventually decoupling Traits and TraitsUI entirely. |
There was a problem hiding this comment.
LGTM!
Thanks for the explanation of the code changes @mdickinson. The approach to separate the concept of "a ui handler" from "Qt's ui handler" looks like what we want here. The asyncio package also seems like an appropriate choice for creating a a test "ui handler".
I could mostly follow along with the what's being changed, though I've left some comments for my own understanding.
| """ | ||
| UI handler that dispatches to the asyncio event loop. | ||
| """ | ||
| event_loop.call_soon_threadsafe(lambda: handler(*args, **kwargs)) |
| else: | ||
| ui_handler(handler, *args) | ||
| _ui_handler(handler, *args) |
There was a problem hiding this comment.
Should we also guard against _ui_handler being undefined here, as we do in ui_dispatch?
There was a problem hiding this comment.
Yes, we definitely should! Now I have to go figure out why the tests aren't exercising this ...
There was a problem hiding this comment.
Hmm. I think they are exercising this, but our tests for the exception don't distinguish between a RuntimeError with a nice message and a TypeError from trying to use None as a callable. I need to re-raise exceptions and catch them ...
| """ Tests for dynamic notifiers with `dispatch='ui'` set by method call. | ||
| """ | ||
| class TestMethodUINotifiers( | ||
| BaseTestUINotifiers, unittest.IsolatedAsyncioTestCase |
There was a problem hiding this comment.
Huh, TIL about unittest.IsolatedAsyncioTestCase!
|
|
||
| obj = self.obj_factory() | ||
| def test_notification_from_separate_thread_failure_case(self): |
There was a problem hiding this comment.
I think I understand the context for this test case, but just to be clear: do we accept that any background process unknown to the UI will be able to modify objects, but ideally we also want to know about it?
If so, is this a new edge case that we can just test more easily without Qt, or does this happen often?
There was a problem hiding this comment.
So the key point of the test (and of the dispatch="ui" functionality) is that even though the object is modified off the main thread, the trait change handlers are run in the main UI thread.
That said, this pattern isn't ideal in the first place; my preference is to use something like Traits Futures so that we're not modifying the same object from multiple threads in the first place.
Not sure whether this answers your question or not ... ?
There was a problem hiding this comment.
Ah, I see - so the main result of this test is that we don't see any notifiers and we observe an exception, since the handlers are always on the main UI tread.
That helps understand the context, yes, thanks!
There was a problem hiding this comment.
Right - the notifiers don't get executed at all in this case.
|
@flongford I've responded to comments above and made one minor update, for better exception handling. |
Since #1788, we have only one test module that makes use of the Qt event loop. That test module contains tests for the behaviour of handlers that use
dispatch='ui'mechanism to redispatch off-thread notifications to the ui thread.This PR reworks that test module, with some significant collateral damage along the way.
In detail:
test_ui_notifiers) to avoid the need for the Qt event loop; instead, it tests against aui_handlerbased on asyncio, which redispatches to the running asyncio event loopget_ui_handlercounterpart toset_ui_handler, and exposes both functions intraits.apiget_ui_handlerandset_ui_handlertrait_notifiers:ui_handlerhas been made private, whileui_threadis removed altogetherthreading.local()being a dict (which it hasn't been in living memory), and tidying up some uses of thread identity.