-
Notifications
You must be signed in to change notification settings - Fork 85
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rework ui_dispatch tests to avoid need for Qt #1792
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,33 +18,22 @@ | |
notification really occurs on the UI thread. | ||
|
||
At present, `dispatch='ui'` and `dispatch='fast_ui'` have the same effect. | ||
|
||
""" | ||
|
||
import asyncio | ||
import contextlib | ||
import threading | ||
import time | ||
import unittest | ||
|
||
# Preamble: Try importing Qt, and set QT_FOUND to True on success. | ||
try: | ||
from pyface.util.guisupport import get_app_qt4 | ||
|
||
# This import is necessary to set the `ui_handler` global variable in | ||
# `traits.trait_notifiers`, which is responsible for dispatching the events | ||
# to the UI thread. | ||
from traitsui.qt import toolkit # noqa: F401 | ||
|
||
qt4_app = get_app_qt4() | ||
|
||
except Exception: | ||
QT_FOUND = False | ||
|
||
else: | ||
QT_FOUND = True | ||
|
||
|
||
from traits.api import ( | ||
Callable, | ||
Float, | ||
get_ui_handler, | ||
HasTraits, | ||
on_trait_change, | ||
set_ui_handler, | ||
) | ||
from traits import trait_notifiers | ||
from traits.api import Callable, Float, HasTraits, on_trait_change | ||
|
||
|
||
class CalledAsMethod(HasTraits): | ||
|
@@ -61,88 +50,162 @@ def on_foo_change(self, obj, name, old, new): | |
self.callback(obj, name, old, new) | ||
|
||
|
||
class BaseTestUINotifiers(object): | ||
""" Tests for dynamic notifiers with `dispatch='ui'`. | ||
def asyncio_ui_handler(event_loop): | ||
""" | ||
Create a UI handler that dispatches to the asyncio event loop. | ||
""" | ||
|
||
def ui_handler(handler, *args, **kwargs): | ||
""" | ||
UI handler that dispatches to the asyncio event loop. | ||
""" | ||
event_loop.call_soon_threadsafe(lambda: handler(*args, **kwargs)) | ||
|
||
return ui_handler | ||
|
||
|
||
@contextlib.contextmanager | ||
def use_asyncio_ui_handler(event_loop): | ||
""" | ||
Context manager that temporarily sets the UI handler to an asyncio handler. | ||
""" | ||
old_handler = get_ui_handler() | ||
set_ui_handler(asyncio_ui_handler(event_loop)) | ||
try: | ||
yield | ||
finally: | ||
set_ui_handler(old_handler) | ||
|
||
|
||
@contextlib.contextmanager | ||
def clear_ui_handler(): | ||
""" | ||
Context manager that temporarily clears the UI handler. | ||
""" | ||
old_handler = get_ui_handler() | ||
set_ui_handler(None) | ||
try: | ||
yield | ||
finally: | ||
set_ui_handler(old_handler) | ||
|
||
|
||
class BaseTestUINotifiers(object): | ||
"""Tests for dynamic notifiers with `dispatch='ui'`.""" | ||
|
||
#### 'TestCase' protocol ################################################## | ||
|
||
def setUp(self): | ||
self.notifications = [] | ||
self.exceptions = [] | ||
self.done = asyncio.Event() | ||
self.obj = self.obj_factory() | ||
trait_notifiers.push_exception_handler(self.handle_exception) | ||
self.addCleanup(trait_notifiers.pop_exception_handler) | ||
|
||
def enterContext(self, cm): | ||
# Backport of Python 3.11's TestCase.enterContext method. | ||
result = type(cm).__enter__(cm) | ||
self.addCleanup(type(cm).__exit__, cm, None, None, None) | ||
return result | ||
|
||
#### 'TestUINotifiers' protocol ########################################### | ||
|
||
def flush_event_loop(self): | ||
""" Post and process the Qt events. """ | ||
qt4_app.sendPostedEvents() | ||
qt4_app.processEvents() | ||
def handle_exception(self, object, name, old, new): | ||
self.exceptions.append((object, name, old, new)) | ||
|
||
def modify_obj(self): | ||
trait_notifiers.push_exception_handler(self.handle_exception) | ||
try: | ||
self.obj.foo = 3 | ||
finally: | ||
trait_notifiers.pop_exception_handler() | ||
|
||
def on_foo_notifications(self, obj, name, old, new): | ||
thread_id = threading.current_thread().ident | ||
event = (thread_id, (obj, name, old, new)) | ||
event = (threading.current_thread(), (obj, name, old, new)) | ||
self.notifications.append(event) | ||
self.done.set() | ||
|
||
#### Tests ################################################################ | ||
|
||
@unittest.skipIf( | ||
not QT_FOUND, "Qt event loop not found, UI dispatch not possible." | ||
) | ||
def test_notification_from_main_thread(self): | ||
|
||
obj = self.obj_factory() | ||
def test_notification_from_main_thread_with_no_ui_handler(self): | ||
# Given | ||
self.enterContext(clear_ui_handler()) | ||
|
||
obj.foo = 3 | ||
self.flush_event_loop() | ||
# When we set obj.foo to 3 on the main thread. | ||
self.modify_obj() | ||
|
||
notifications = self.notifications | ||
self.assertEqual(len(notifications), 1) | ||
# Then the notification is processed synchronously on the main thread. | ||
self.assertEqual( | ||
self.notifications, | ||
[(threading.main_thread(), (self.obj, "foo", 0, 3))], | ||
) | ||
|
||
thread_id, event = notifications[0] | ||
self.assertEqual(event, (obj, "foo", 0, 3)) | ||
def test_notification_from_main_thread_with_registered_ui_handler(self): | ||
# Given | ||
self.enterContext(use_asyncio_ui_handler(asyncio.get_event_loop())) | ||
|
||
ui_thread = trait_notifiers.ui_thread | ||
self.assertEqual(thread_id, ui_thread) | ||
# When we set obj.foo to 3 on the main thread. | ||
self.modify_obj() | ||
|
||
@unittest.skipIf( | ||
not QT_FOUND, "Qt event loop not found, UI dispatch not possible." | ||
) | ||
def test_notification_from_separate_thread(self): | ||
# Then the notification is processed synchronously on the main thread. | ||
self.assertEqual( | ||
self.notifications, | ||
[(threading.main_thread(), (self.obj, "foo", 0, 3))], | ||
) | ||
|
||
obj = self.obj_factory() | ||
def test_notification_from_separate_thread_failure_case(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the key point of the test (and of the 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right - the notifiers don't get executed at all in this case. |
||
# Given no registered ui handler | ||
self.enterContext(clear_ui_handler()) | ||
|
||
# Set obj.foo to 3 on a separate thread. | ||
def set_foo_to_3(obj): | ||
obj.foo = 3 | ||
# When we set obj.foo to 3 on a separate thread. | ||
background_thread = threading.Thread(target=self.modify_obj) | ||
background_thread.start() | ||
self.addCleanup(background_thread.join) | ||
|
||
threading.Thread(target=set_foo_to_3, args=(obj,)).start() | ||
# Then no notification is processed ... | ||
self.assertEqual(self.notifications, []) | ||
|
||
# Wait for a while to make sure the function has finished. | ||
time.sleep(0.1) | ||
# ... and an error was raised | ||
self.assertEqual(self.exceptions, [(self.obj, "foo", 0, 3)]) | ||
|
||
self.flush_event_loop() | ||
# ... but the attribute change was still applied. | ||
self.assertEqual(self.obj.foo, 3) | ||
|
||
notifications = self.notifications | ||
self.assertEqual(len(notifications), 1) | ||
async def test_notification_from_separate_thread(self): | ||
# Given an asyncio ui handler | ||
self.enterContext(use_asyncio_ui_handler(asyncio.get_event_loop())) | ||
|
||
thread_id, event = notifications[0] | ||
self.assertEqual(event, (obj, "foo", 0, 3)) | ||
# When we set obj.foo to 3 on a separate thread. | ||
background_thread = threading.Thread(target=self.modify_obj) | ||
background_thread.start() | ||
self.addCleanup(background_thread.join) | ||
|
||
ui_thread = trait_notifiers.ui_thread | ||
self.assertEqual(thread_id, ui_thread) | ||
# Then the notification will eventually be processed on the main | ||
# thread. | ||
await asyncio.wait_for(self.done.wait(), timeout=5.0) | ||
self.assertEqual( | ||
self.notifications, | ||
[(threading.main_thread(), (self.obj, "foo", 0, 3))], | ||
) | ||
self.assertEqual(self.obj.foo, 3) | ||
|
||
|
||
class TestMethodUINotifiers(BaseTestUINotifiers, unittest.TestCase): | ||
""" Tests for dynamic notifiers with `dispatch='ui'` set by method call. | ||
""" | ||
class TestMethodUINotifiers( | ||
BaseTestUINotifiers, unittest.IsolatedAsyncioTestCase | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Huh, TIL about |
||
): | ||
"""Tests for dynamic notifiers with `dispatch='ui'` set by method call.""" | ||
|
||
def obj_factory(self): | ||
obj = CalledAsMethod() | ||
obj.on_trait_change(self.on_foo_notifications, "foo", dispatch="ui") | ||
return obj | ||
|
||
|
||
class TestDecoratorUINotifiers(BaseTestUINotifiers, unittest.TestCase): | ||
""" Tests for dynamic notifiers with `dispatch='ui'` set by decorator. """ | ||
class TestDecoratorUINotifiers( | ||
BaseTestUINotifiers, unittest.IsolatedAsyncioTestCase | ||
): | ||
"""Tests for dynamic notifiers with `dispatch='ui'` set by decorator.""" | ||
|
||
def obj_factory(self): | ||
return CalledAsDecorator(callback=self.on_foo_notifications) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,27 +27,33 @@ | |
|
||
# Global Data | ||
|
||
# The thread ID for the user interface thread | ||
ui_thread = -1 | ||
# The currently active handler for notifications that must be run on the UI | ||
# thread, or None if no handler has been set. | ||
_ui_handler = None | ||
|
||
# The handler for notifications that must be run on the UI thread | ||
ui_handler = None | ||
|
||
def get_ui_handler(): | ||
""" | ||
Return the current user interface thread handler. | ||
""" | ||
return _ui_handler | ||
|
||
|
||
def set_ui_handler(handler): | ||
""" Sets up the user interface thread handler. | ||
""" | ||
global ui_handler, ui_thread | ||
global _ui_handler | ||
|
||
ui_handler = handler | ||
ui_thread = threading.current_thread().ident | ||
_ui_handler = handler | ||
mdickinson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
|
||
def ui_dispatch(handler, *args, **kw): | ||
if threading.current_thread() == threading.main_thread(): | ||
handler(*args, **kw) | ||
elif _ui_handler is None: | ||
raise RuntimeError("No UI handler has been set.") | ||
else: | ||
ui_handler(handler, *args, **kw) | ||
_ui_handler(handler, *args, **kw) | ||
|
||
|
||
class NotificationExceptionHandlerState(object): | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Drive-by cleanup: this condition is never true in modern Python. |
||
id = threading.current_thread().ident | ||
handlers = thread_local.get(id) | ||
else: | ||
handlers = getattr(thread_local, "handlers", None) | ||
handlers = getattr(thread_local, "handlers", None) | ||
|
||
if handlers is None: | ||
if self.main_thread is not None: | ||
|
@@ -167,10 +169,7 @@ def _get_handlers(self): | |
self._log_exception, False, False | ||
) | ||
handlers = [handler] | ||
if isinstance(thread_local, dict): | ||
thread_local[id] = handlers | ||
else: | ||
thread_local.handlers = handlers | ||
thread_local.handlers = handlers | ||
|
||
return handlers | ||
|
||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the fix that should have been part of #1740. |
||
handler(*args) | ||
else: | ||
ui_handler(handler, *args) | ||
_ui_handler(handler, *args) | ||
Comment on lines
621
to
+622
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we also guard against There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm. I think they are exercising this, but our tests for the exception don't distinguish between a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reworked in cc8653e |
||
|
||
|
||
class NewTraitChangeNotifyWrapper(TraitChangeNotifyWrapper): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
from typing import Callable | ||
|
||
_UI_Handler = Callable[..., None] | None | ||
|
||
|
||
def get_ui_handler() -> _UI_Handler: ... | ||
|
||
def set_ui_handler(handler: _UI_Handler) -> None: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat!