From 41c997f3162455c9f416298126d4700c911aae45 Mon Sep 17 00:00:00 2001 From: Darren Burns Date: Wed, 7 Jun 2023 17:49:25 +0100 Subject: [PATCH 01/10] Working on pilot exception catching --- src/textual/app.py | 13 ++++++++++++- src/textual/message_pump.py | 11 +++++++++++ src/textual/pilot.py | 4 +++- tests/test_pilot.py | 35 ++++++++++++++++++++++++++++++++++- 4 files changed, 60 insertions(+), 3 deletions(-) diff --git a/src/textual/app.py b/src/textual/app.py index 4aed6b0394..0ae89697c7 100644 --- a/src/textual/app.py +++ b/src/textual/app.py @@ -183,7 +183,6 @@ class CssPathError(Exception): ReturnType = TypeVar("ReturnType") - CSSPathType = Union[ str, PurePath, @@ -367,6 +366,10 @@ def __init__( self._animate = self._animator.bind(self) self.mouse_position = Offset(0, 0) + self._exception: Exception | None = None + """The unhandled exception which is leading to the app shutting down, + or None if the app is still running with no unhandled exceptions.""" + self.title = ( self.TITLE if self.TITLE is not None else f"{self.__class__.__name__}" ) @@ -1108,6 +1111,9 @@ async def run_app(app) -> None: # Shutdown the app cleanly await app._shutdown() await app_task + # Re-raise the exception which caused panic so test frameworks are aware + if self._exception: + raise self._exception async def run_async( self, @@ -1782,9 +1788,14 @@ def render(renderable: RenderableType) -> list[Segment]: def _handle_exception(self, error: Exception) -> None: """Called with an unhandled exception. + Always results in the app exiting. + Args: error: An exception instance. """ + if self.is_headless: + self._exception = error + if hasattr(error, "__rich__"): # Exception has a rich method, so we can defer to that for the rendering self.panic(error) diff --git a/src/textual/message_pump.py b/src/textual/message_pump.py index 2c191d3de7..469b3197fa 100644 --- a/src/textual/message_pump.py +++ b/src/textual/message_pump.py @@ -491,14 +491,25 @@ async def _process_messages_loop(self) -> None: """Process messages until the queue is closed.""" _rich_traceback_guard = True self._thread_id = threading.get_ident() + from .app import App + while not self._closed: try: message = await self._get_message() + if isinstance(self, App): + print(f"APP received message = {message!r}") + print(f"App queue = {self._message_queue._queue}") except MessagePumpClosed: + if isinstance(self, App): + print(f"APP MESSAGE PUMP CLOSED - NO LONGER PROCESSING MESSAGES") + print(f"App queue = {self._message_queue._queue}") break except CancelledError: raise except Exception as error: + if isinstance(self, App): + print(f"Exception occurred in process messages loop") + print(f"App queue = {self._message_queue._queue}") raise error from None # Combine any pending messages that may supersede this one diff --git a/src/textual/pilot.py b/src/textual/pilot.py index 041e00e13e..d278e460a2 100644 --- a/src/textual/pilot.py +++ b/src/textual/pilot.py @@ -133,7 +133,7 @@ async def hover( app.post_message(MouseMove(**message_arguments)) await self.pause() - async def _wait_for_screen(self, timeout: float = 30.0) -> bool: + async def _wait_for_screen(self, timeout: float = 10.0) -> bool: """Wait for the current screen to have processed all pending events. Args: @@ -162,8 +162,10 @@ def decrement_counter() -> None: if count: # Wait for the count to return to zero, or a timeout try: + print("NOW WE WAIT") await asyncio.wait_for(count_zero_event.wait(), timeout=timeout) except asyncio.TimeoutError: + print("WE'VE TIMED OUT") return False return True diff --git a/tests/test_pilot.py b/tests/test_pilot.py index 04491dea9a..d631146c77 100644 --- a/tests/test_pilot.py +++ b/tests/test_pilot.py @@ -1,7 +1,11 @@ from string import punctuation +import pytest + from textual import events -from textual.app import App +from textual.app import App, ComposeResult +from textual.binding import Binding +from textual.widgets import Label KEY_CHARACTERS_TO_TEST = "akTW03" + punctuation """Test some "simple" characters (letters + digits) and all punctuation.""" @@ -19,3 +23,32 @@ def on_key(self, event: events.Key) -> None: for char in KEY_CHARACTERS_TO_TEST: await pilot.press(char) assert keys_pressed[-1] == char + + +async def test_pilot_exception_catching_compose(): + """Ensuring that test frameworks are aware of exceptions + inside compose methods when running via Pilot run_test().""" + + class FailingApp(App): + def compose(self) -> ComposeResult: + 1 / 0 + yield Label("Beep") + + with pytest.raises(ZeroDivisionError): + async with FailingApp().run_test(): + pass + + +async def test_pilot_exception_catching_action(): + """Ensure that exceptions inside action handlers are presented + to the test framework when running via Pilot run_test().""" + + class FailingApp(App): + BINDINGS = [Binding("b", "beep", "beep")] + + def action_beep(self) -> None: + 1 / 0 + + with pytest.raises(ZeroDivisionError): + async with FailingApp().run_test() as pilot: + await pilot.press("b") From e73245aab2b97c5a8e7d36208d161ec3d97a74e7 Mon Sep 17 00:00:00 2001 From: Darren Burns Date: Thu, 8 Jun 2023 10:49:37 +0100 Subject: [PATCH 02/10] Handling exceptions in wait_for_screen --- src/textual/app.py | 9 ++++++++- src/textual/message_pump.py | 10 ---------- src/textual/pilot.py | 24 ++++++++++++++++-------- 3 files changed, 24 insertions(+), 19 deletions(-) diff --git a/src/textual/app.py b/src/textual/app.py index 0ae89697c7..a8b4dc61b0 100644 --- a/src/textual/app.py +++ b/src/textual/app.py @@ -370,6 +370,9 @@ def __init__( """The unhandled exception which is leading to the app shutting down, or None if the app is still running with no unhandled exceptions.""" + self._exception_event: asyncio.Event | None = None + """An event that will be set when the first exception is encountered.""" + self.title = ( self.TITLE if self.TITLE is not None else f"{self.__class__.__name__}" ) @@ -1086,6 +1089,7 @@ async def run_app(app) -> None: message_hook_context_var.set(message_hook) app._loop = asyncio.get_running_loop() app._thread_id = threading.get_ident() + app._exception_event = asyncio.Event() await app._process_messages( ready_callback=on_app_ready, headless=headless, @@ -1793,8 +1797,11 @@ def _handle_exception(self, error: Exception) -> None: Args: error: An exception instance. """ - if self.is_headless: + # If we're running via pilot and this is the first exception encountered, + # take note of it so that we can re-raise for test frameworks later. + if self.is_headless and self._exception is None: self._exception = error + self._exception_event.set() if hasattr(error, "__rich__"): # Exception has a rich method, so we can defer to that for the rendering diff --git a/src/textual/message_pump.py b/src/textual/message_pump.py index 469b3197fa..584ccb36e5 100644 --- a/src/textual/message_pump.py +++ b/src/textual/message_pump.py @@ -491,25 +491,15 @@ async def _process_messages_loop(self) -> None: """Process messages until the queue is closed.""" _rich_traceback_guard = True self._thread_id = threading.get_ident() - from .app import App while not self._closed: try: message = await self._get_message() - if isinstance(self, App): - print(f"APP received message = {message!r}") - print(f"App queue = {self._message_queue._queue}") except MessagePumpClosed: - if isinstance(self, App): - print(f"APP MESSAGE PUMP CLOSED - NO LONGER PROCESSING MESSAGES") - print(f"App queue = {self._message_queue._queue}") break except CancelledError: raise except Exception as error: - if isinstance(self, App): - print(f"Exception occurred in process messages loop") - print(f"App queue = {self._message_queue._queue}") raise error from None # Combine any pending messages that may supersede this one diff --git a/src/textual/pilot.py b/src/textual/pilot.py index d278e460a2..c2a65a6354 100644 --- a/src/textual/pilot.py +++ b/src/textual/pilot.py @@ -133,14 +133,15 @@ async def hover( app.post_message(MouseMove(**message_arguments)) await self.pause() - async def _wait_for_screen(self, timeout: float = 10.0) -> bool: + async def _wait_for_screen(self, timeout: float = 4.0) -> bool: """Wait for the current screen to have processed all pending events. Args: timeout: A timeout in seconds to wait. Returns: - `True` if all events were processed, or `False` if the wait timed out. + `True` if all events were processed. `False` if the wait timed + out or an exception occurred while in the application. """ children = [self.app, *self.app.screen.walk_children(with_self=True)] count = 0 @@ -160,12 +161,19 @@ def decrement_counter() -> None: count += 1 if count: - # Wait for the count to return to zero, or a timeout - try: - print("NOW WE WAIT") - await asyncio.wait_for(count_zero_event.wait(), timeout=timeout) - except asyncio.TimeoutError: - print("WE'VE TIMED OUT") + # Wait for the count to return to zero, or a timeout, or an exception + await asyncio.wait( + [ + count_zero_event.wait(), + self.app._exception_event.wait(), + ], + timeout=timeout, + return_when=asyncio.FIRST_COMPLETED, + ) + + # We've either timed out, encountered an exception, or we've finished + # decrementing all the counters (all events processed in children). + if count > 0: return False return True From 5e9a1cfd928e8982a4a5cc141008f1f4e1f642e6 Mon Sep 17 00:00:00 2001 From: Darren Burns Date: Thu, 8 Jun 2023 11:42:20 +0100 Subject: [PATCH 03/10] Ensure pending tasks cancelled, fix TabbedContent issue --- src/textual/app.py | 3 +-- src/textual/pilot.py | 4 +++- src/textual/widgets/_tabs.py | 4 +--- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/textual/app.py b/src/textual/app.py index a8b4dc61b0..a48aae41f7 100644 --- a/src/textual/app.py +++ b/src/textual/app.py @@ -370,7 +370,7 @@ def __init__( """The unhandled exception which is leading to the app shutting down, or None if the app is still running with no unhandled exceptions.""" - self._exception_event: asyncio.Event | None = None + self._exception_event: asyncio.Event = asyncio.Event() """An event that will be set when the first exception is encountered.""" self.title = ( @@ -1089,7 +1089,6 @@ async def run_app(app) -> None: message_hook_context_var.set(message_hook) app._loop = asyncio.get_running_loop() app._thread_id = threading.get_ident() - app._exception_event = asyncio.Event() await app._process_messages( ready_callback=on_app_ready, headless=headless, diff --git a/src/textual/pilot.py b/src/textual/pilot.py index c2a65a6354..5124dfd0cc 100644 --- a/src/textual/pilot.py +++ b/src/textual/pilot.py @@ -162,7 +162,7 @@ def decrement_counter() -> None: if count: # Wait for the count to return to zero, or a timeout, or an exception - await asyncio.wait( + _, pending = await asyncio.wait( [ count_zero_event.wait(), self.app._exception_event.wait(), @@ -170,6 +170,8 @@ def decrement_counter() -> None: timeout=timeout, return_when=asyncio.FIRST_COMPLETED, ) + for task in pending: + task.cancel() # We've either timed out, encountered an exception, or we've finished # decrementing all the counters (all events processed in children). diff --git a/src/textual/widgets/_tabs.py b/src/textual/widgets/_tabs.py index 3efab6d921..5e4e312bc7 100644 --- a/src/textual/widgets/_tabs.py +++ b/src/textual/widgets/_tabs.py @@ -422,9 +422,7 @@ def watch_active(self, previously_active: str, active: str) -> None: active_tab = self.query_one(f"#tabs-list > #{active}", Tab) self.query("#tabs-list > Tab.-active").remove_class("-active") active_tab.add_class("-active") - self.call_after_refresh( - self._highlight_active, animate=previously_active != "" - ) + self.call_later(self._highlight_active, animate=previously_active != "") self.post_message(self.TabActivated(self, active_tab)) else: underline = self.query_one(Underline) From ad234edb6c640c9698b9f032c6b6e550ce0131cb Mon Sep 17 00:00:00 2001 From: Darren Burns Date: Thu, 8 Jun 2023 11:51:07 +0100 Subject: [PATCH 04/10] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 23739959d7..81d931e436 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Fixed setting `TreeNode.label` on an existing `Tree` node not immediately https://github.com/Textualize/textual/pull/2713 - Correctly implement `__eq__` protocol in DataTable https://github.com/Textualize/textual/pull/2705 +- Fixed exceptions in Pilot tests being silently ignored https://github.com/Textualize/textual/pull/2754 ### Changed From 5fa75d243355a43fc87591f088a01eca33465175 Mon Sep 17 00:00:00 2001 From: Darren Burns Date: Thu, 8 Jun 2023 12:05:17 +0100 Subject: [PATCH 05/10] Passing tasks to asyncio.wait to avoid deprecation --- src/textual/pilot.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/textual/pilot.py b/src/textual/pilot.py index 5124dfd0cc..f77827b1cd 100644 --- a/src/textual/pilot.py +++ b/src/textual/pilot.py @@ -133,7 +133,7 @@ async def hover( app.post_message(MouseMove(**message_arguments)) await self.pause() - async def _wait_for_screen(self, timeout: float = 4.0) -> bool: + async def _wait_for_screen(self, timeout: float = 10.0) -> bool: """Wait for the current screen to have processed all pending events. Args: @@ -164,8 +164,8 @@ def decrement_counter() -> None: # Wait for the count to return to zero, or a timeout, or an exception _, pending = await asyncio.wait( [ - count_zero_event.wait(), - self.app._exception_event.wait(), + asyncio.create_task(count_zero_event.wait()), + asyncio.create_task(self.app._exception_event.wait()), ], timeout=timeout, return_when=asyncio.FIRST_COMPLETED, From af038d93bea2b128dde04cf96b4028a87b732e50 Mon Sep 17 00:00:00 2001 From: Darren Burns Date: Thu, 8 Jun 2023 14:05:17 +0100 Subject: [PATCH 06/10] Raise exception if wait_for_screen doesnt succeed within timeout --- src/textual/pilot.py | 22 +++++++++++++++++----- tests/test_screen_modes.py | 2 +- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/src/textual/pilot.py b/src/textual/pilot.py index f77827b1cd..77b92e645c 100644 --- a/src/textual/pilot.py +++ b/src/textual/pilot.py @@ -42,6 +42,10 @@ def _get_mouse_message_arguments( return message_arguments +class WaitForScreenTimeout(Exception): + pass + + @rich.repr.auto(angular=True) class Pilot(Generic[ReturnType]): """Pilot object to drive an app.""" @@ -133,7 +137,7 @@ async def hover( app.post_message(MouseMove(**message_arguments)) await self.pause() - async def _wait_for_screen(self, timeout: float = 10.0) -> bool: + async def _wait_for_screen(self, timeout: float = 30.0) -> bool: """Wait for the current screen to have processed all pending events. Args: @@ -162,17 +166,25 @@ def decrement_counter() -> None: if count: # Wait for the count to return to zero, or a timeout, or an exception + wait_for = [ + asyncio.create_task(count_zero_event.wait()), + asyncio.create_task(self.app._exception_event.wait()), + ] _, pending = await asyncio.wait( - [ - asyncio.create_task(count_zero_event.wait()), - asyncio.create_task(self.app._exception_event.wait()), - ], + wait_for, timeout=timeout, return_when=asyncio.FIRST_COMPLETED, ) + for task in pending: task.cancel() + timed_out = len(wait_for) == len(pending) + if timed_out: + raise WaitForScreenTimeout( + "Timed out while waiting for widgets to process pending messages." + ) + # We've either timed out, encountered an exception, or we've finished # decrementing all the counters (all events processed in children). if count > 0: diff --git a/tests/test_screen_modes.py b/tests/test_screen_modes.py index 6fd5c185d3..4edb1de22d 100644 --- a/tests/test_screen_modes.py +++ b/tests/test_screen_modes.py @@ -182,7 +182,7 @@ def compose(self) -> ComposeResult: yield Label("fast") def on_mount(self) -> None: - self.set_interval(0.01, self.ping) + self.call_later(self.set_interval, 0.01, self.ping) def ping(self) -> None: pings.append(str(self.app.query_one(Label).renderable)) From 7d128be1ed255170a284eb25452d35a8176369c9 Mon Sep 17 00:00:00 2001 From: Darren Burns Date: Thu, 8 Jun 2023 15:13:09 +0100 Subject: [PATCH 07/10] Update clear_options to immediately refresh content tracking --- src/textual/widgets/_option_list.py | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/textual/widgets/_option_list.py b/src/textual/widgets/_option_list.py index 3b365fa7bf..d68e24e0bb 100644 --- a/src/textual/widgets/_option_list.py +++ b/src/textual/widgets/_option_list.py @@ -630,14 +630,7 @@ def clear_options(self) -> Self: self.highlighted = None self._mouse_hovering_over = None self.virtual_size = Size(self.scrollable_content_region.width, 0) - # TODO: See https://github.com/Textualize/textual/issues/2582 -- it - # should not be necessary to do this like this here; ideally here in - # clear_options it would be a forced refresh, and also in a - # `on_show` it would be the same (which, I think, would actually - # solve the problem we're seeing). But, until such a time as we get - # to the bottom of 2582... this seems to delay the refresh enough - # that things fall into place. - self._request_content_tracking_refresh() + self._refresh_content_tracking() return self def _set_option_disabled(self, index: int, disabled: bool) -> Self: From 685015c974e79eeecbed6215ece239c362071272 Mon Sep 17 00:00:00 2001 From: Darren Burns Date: Thu, 8 Jun 2023 15:21:08 +0100 Subject: [PATCH 08/10] force=True on content tracking refresh in option_list --- src/textual/widgets/_option_list.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/textual/widgets/_option_list.py b/src/textual/widgets/_option_list.py index d68e24e0bb..65fd77470d 100644 --- a/src/textual/widgets/_option_list.py +++ b/src/textual/widgets/_option_list.py @@ -630,7 +630,7 @@ def clear_options(self) -> Self: self.highlighted = None self._mouse_hovering_over = None self.virtual_size = Size(self.scrollable_content_region.width, 0) - self._refresh_content_tracking() + self._refresh_content_tracking(force=True) return self def _set_option_disabled(self, index: int, disabled: bool) -> Self: From d88fb58f2d075bb136c1e3ee54477adacd18f6cb Mon Sep 17 00:00:00 2001 From: Darren Burns Date: Thu, 8 Jun 2023 15:51:29 +0100 Subject: [PATCH 09/10] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a8360031ce..472c4bfda7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Fixed setting `TreeNode.label` on an existing `Tree` node not immediately https://github.com/Textualize/textual/pull/2713 - Correctly implement `__eq__` protocol in DataTable https://github.com/Textualize/textual/pull/2705 - Fixed exceptions in Pilot tests being silently ignored https://github.com/Textualize/textual/pull/2754 +- Fixed issue where internal data of `OptionList` could be invalid for short window after `clear_options` https://github.com/Textualize/textual/pull/2754 - Fixed `Tooltip` causing a `query_one` on a lone `Static` to fail https://github.com/Textualize/textual/issues/2723 ### Changed From 1058bd4fff59f1a0b8586b3dc7dda718ff7bb279 Mon Sep 17 00:00:00 2001 From: Darren Burns Date: Thu, 8 Jun 2023 16:45:40 +0100 Subject: [PATCH 10/10] Update a docstring --- src/textual/pilot.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/textual/pilot.py b/src/textual/pilot.py index 77b92e645c..64cd602bf2 100644 --- a/src/textual/pilot.py +++ b/src/textual/pilot.py @@ -138,14 +138,17 @@ async def hover( await self.pause() async def _wait_for_screen(self, timeout: float = 30.0) -> bool: - """Wait for the current screen to have processed all pending events. + """Wait for the current screen and its children to have processed all pending events. Args: timeout: A timeout in seconds to wait. Returns: - `True` if all events were processed. `False` if the wait timed - out or an exception occurred while in the application. + `True` if all events were processed. `False` if an exception occurred, + meaning that not all events could be processed. + + Raises: + WaitForScreenTimeout: If the screen and its children didn't finish processing within the timeout. """ children = [self.app, *self.app.screen.walk_children(with_self=True)] count = 0