Rework ui_dispatch tests to avoid need for Qt #1792

mdickinson · 2024-04-30T17:56:20Z

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:

reworks that test module (test_ui_notifiers) to avoid the need for the Qt event loop; instead, it tests against a ui_handler based on asyncio, which redispatches to the running asyncio event loop
adds a get_ui_handler counterpart to set_ui_handler, and exposes both functions in traits.api
adds type hints for get_ui_handler and set_ui_handler
removes two public module globals from trait_notifiers: ui_handler has been made private, while ui_thread is removed altogether
fixes a bug where ui dispatch didn't do the right thing (PR MAINT: add main thread check for ui dispatch, solve no ui failure #1740 was incomplete; this bug should have been caught at review time on that PR)
makes another couple of drive-by cleanups, removing a very old check for threading.local() being a dict (which it hasn't been in living memory), and tidying up some uses of thread identity.

mdickinson · 2024-04-30T17:58:27Z

traits/trait_notifiers.py

@@ -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():


This is the fix that should have been part of #1740.

mdickinson · 2024-04-30T18:00:16Z

traits/trait_notifiers.py

@@ -153,11 +159,7 @@ def _get_handlers(self):
            thread.
        """
        thread_local = self.thread_local
-        if isinstance(thread_local, dict):


Drive-by cleanup: this condition is never true in modern Python.

mdickinson · 2024-04-30T18:03:49Z

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

flongford

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.

flongford · 2024-05-01T09:06:57Z

traits/tests/test_ui_notifiers.py

+        """
+        UI handler that dispatches to the asyncio event loop.
+        """
+        event_loop.call_soon_threadsafe(lambda: handler(*args, **kwargs))


flongford · 2024-05-01T22:21:45Z

traits/trait_notifiers.py

        else:
-            ui_handler(handler, *args)
+            _ui_handler(handler, *args)


Should we also guard against _ui_handler being undefined here, as we do in ui_dispatch?

Yes, we definitely should! Now I have to go figure out why the tests aren't exercising this ...

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

Reworked in cc8653e

traits/trait_notifiers.py

flongford · 2024-05-02T10:03:58Z

traits/tests/test_ui_notifiers.py

-    """ Tests for dynamic notifiers with `dispatch='ui'` set by method call.
-    """
+class TestMethodUINotifiers(
+    BaseTestUINotifiers, unittest.IsolatedAsyncioTestCase


Huh, TIL about unittest.IsolatedAsyncioTestCase!

flongford · 2024-05-02T10:17:01Z

traits/tests/test_ui_notifiers.py


-        obj = self.obj_factory()
+    def test_notification_from_separate_thread_failure_case(self):


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?

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

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!

Right - the notifiers don't get executed at all in this case.

mdickinson · 2024-05-03T13:38:59Z

@flongford I've responded to comments above and made one minor update, for better exception handling.

Since #1788 and #1792, we should no longer need a working Qt backend when testing. Accordingly, this PR: - removes PySide6 as a test dependency - removes the need to `apt-get` Qt packages on Ubuntu runners - removes the use of `xvfb-run` on Ubuntu runners

mdickinson · 2025-01-24T20:17:46Z

removes two public module globals from trait_notifiers: ui_handler has been made private

This should not have been done: Pyface directly accesses ui_handler. (It shouldn't, but it does.)

In #1792 we made `traits.trait_notifiers.ui_handler` private, renaming it to `_ui_handler`. However, it turns out that Pyface wants to access the value of `ui_handler` directly, so this change breaks current Pyface. This PR reverts that change. Pyface code links: - https://github.com/enthought/pyface/blob/46f700999284c8104fb2a5468f549677dfadf063/pyface/ui/qt/init.py#L15 - https://github.com/enthought/pyface/blob/46f700999284c8104fb2a5468f549677dfadf063/pyface/ui/wx/init.py#L14

In #1792 we made `traits.trait_notifiers.ui_handler` private, renaming it to `_ui_handler`. However, it turns out that Pyface wants to access the value of `ui_handler` directly, so this change breaks current Pyface. This PR reverts that change. Pyface code links: - https://github.com/enthought/pyface/blob/46f700999284c8104fb2a5468f549677dfadf063/pyface/ui/qt/init.py#L15 - https://github.com/enthought/pyface/blob/46f700999284c8104fb2a5468f549677dfadf063/pyface/ui/wx/init.py#L14 (cherry picked from commit 347e58b)

Rework ui_dispatch tests to avoid need for Qt

b97a36c

mdickinson mentioned this pull request Apr 30, 2024

Remove PySide as a test dependency #1791

Merged

mdickinson commented Apr 30, 2024

View reviewed changes

mdickinson requested a review from flongford April 30, 2024 18:54

flongford approved these changes May 2, 2024

View reviewed changes

Rework exception handling in tests

cc8653e

mdickinson requested a review from flongford May 3, 2024 13:39

mdickinson merged commit 2bc548f into main May 7, 2024
29 checks passed

mdickinson deleted the dev/rework-tests-to-avoid-qt branch May 7, 2024 09:46

mdickinson mentioned this pull request Jan 24, 2025

Make ui_handler publicly available again #1827

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework ui_dispatch tests to avoid need for Qt #1792

Rework ui_dispatch tests to avoid need for Qt #1792

mdickinson commented Apr 30, 2024

mdickinson Apr 30, 2024

mdickinson Apr 30, 2024

mdickinson commented Apr 30, 2024

flongford left a comment •

edited

Loading

flongford May 1, 2024

flongford May 1, 2024

mdickinson May 3, 2024

mdickinson May 3, 2024

mdickinson May 3, 2024

flongford May 2, 2024

flongford May 2, 2024

mdickinson May 3, 2024

flongford May 3, 2024

mdickinson May 3, 2024

mdickinson commented May 3, 2024

mdickinson commented Jan 24, 2025


		obj = self.obj_factory()
		def test_notification_from_separate_thread_failure_case(self):

Rework ui_dispatch tests to avoid need for Qt #1792

Rework ui_dispatch tests to avoid need for Qt #1792

Conversation

mdickinson commented Apr 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdickinson commented Apr 30, 2024

flongford left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdickinson commented May 3, 2024

mdickinson commented Jan 24, 2025

flongford left a comment •

edited

Loading