Test flakiness investigation and attempted fixes ❄ (#3498)

* Modifying two flaky animation tests, hopefully removing flakiness :) * Make switch_mode return an AwaitMount * Fix event issue * Add AwaitComplete - a more generalised optionally awaitable object * Use AwaitComplete in Tabs.remove_tab() and update tests accordingly. * Update TabbedContent to use AwaitComplete instead of AwaitTabbedContent * Simplifying - dont use optional awaitables where not required * Update variable name * Update a comment * Add watcher for cursor blink to ensure when blink is switched off, the cursor immediately becomes visible. Ensure we turn of cursor blink inside the input suggetions snapshot test. * Fix cursor blink reactive and disable cursor blink in the command palette snapshot test * More progress * Reworking AwaitComplete * Some more work on tabs flakiness/race-conditions * Ensure active tab is set correctly * Simplify next tab assignment * Simplify removing tabs logic * Make button animation duration configurable; Switch off button animation in snapshot test * Remove a flawed test * Add awaits in some tests * Docstrings * Make active_effect_duration an instance attribute * Fix a Tabs crash * Await the tree reload when the path changes in DirectoryTree * Change AwaitComplete _instances class attr to a set from a list * Make AwaitComplete generic, AwaitComplete._wait_all is now private, and exposes timeout parameter * Actually make AwaitComplete instances a set, not a list * Update CHANGELOG.md regarding flaky-test adjacent changes, AwaitComplete, etc.. * Remove whitespace * Use list() instead of useless comprehension, remove unused import * Ensure loading indicator _start_time is initialised correctly * Switch from time.sleep to asyncio.sleep in a notifications test, rework numbers to try and prevent flakiness * Resolve deadlock by awaiting event on the event loop instead of in the message pump * Renaming for clarity * Debugging for remove_tab test flakiness * Running all tests * Updating snapshots * Remove debugging prints * Fix broken docstring, remove unused import * Rename variable to make it clearer * Add missing return type annotation * Update src/textual/widgets/_tabbed_content.py Co-authored-by: Rodrigo Girão Serrão <[email protected]> * Update src/textual/widgets/_tabbed_content.py Co-authored-by: Rodrigo Girão Serrão <[email protected]> * Update src/textual/widgets/_tabs.py Co-authored-by: Rodrigo Girão Serrão <[email protected]> * Scroll datatable cursor after refresh * Add comment explaining use of call_after_refresh when scrolling data table cursor into view * Add repr to AwaitComplete (auto-repr_ * Remove use of generics from AwaitComplete * Update changelog and improve docstring * Add a missing parameter from DirectoryTree.reset_node docstring. Signed-off-by: Darren Burns <[email protected]> * Improve docstring in DirectoryTree Signed-off-by: Darren Burns <[email protected]> * Rename parameter coroutine to coroutines in await_complete.py, since it's a variable length param. --------- Signed-off-by: Darren Burns <[email protected]> Co-authored-by: Rodrigo Girão Serrão <[email protected]>
Textualize · Oct 25, 2023 · 34fb596 · 34fb596
1 parent 5d7585c
commit 34fb596
Show file tree

Hide file tree

Showing 19 changed files with 332 additions and 251 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,6 +10,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ### Fixed
 
+- Fixed `Input.cursor_blink` reactive not changing blink state after `Input` was mounted https://github.com/Textualize/textual/pull/3498
+- Fixed `Tabs.active` attribute value not being re-assigned after removing a tab or clearing https://github.com/Textualize/textual/pull/3498
+- Fixed `DirectoryTree` race-condition crash when changing path https://github.com/Textualize/textual/pull/3498
 - Fixed issue with `LRUCache.discard` https://github.com/Textualize/textual/issues/3537
 - Fixed `DataTable` not scrolling to rows that were just added https://github.com/Textualize/textual/pull/3552
 - Fixed cache bug with `DataTable.update_cell` https://github.com/Textualize/textual/pull/3551
@@ -23,6 +26,19 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ### Changed
 
+- Breaking change: `Button.ACTIVE_EFFECT_DURATION` classvar converted to `Button.active_effect_duration` attribute https://github.com/Textualize/textual/pull/3498
+- Breaking change: `Input.blink_timer` made private (renamed to `Input._blink_timer`) https://github.com/Textualize/textual/pull/3498
+- Breaking change: `Input.cursor_blink` reactive updated to not run on mount (now `init=False`) https://github.com/Textualize/textual/pull/3498
+- Breaking change: `AwaitTabbedContent` class removed https://github.com/Textualize/textual/pull/3498
+- Breaking change: `Tabs.remove_tab` now returns an `AwaitComplete` instead of an `AwaitRemove` https://github.com/Textualize/textual/pull/3498
+- Breaking change: `Tabs.clear` now returns an `AwaitComplete` instead of an `AwaitRemove` https://github.com/Textualize/textual/pull/3498
+- `TabbedContent.add_pane` now returns an `AwaitComplete` instead of an `AwaitTabbedContent` https://github.com/Textualize/textual/pull/3498
+- `TabbedContent.remove_pane` now returns an `AwaitComplete` instead of an `AwaitTabbedContent` https://github.com/Textualize/textual/pull/3498
+- `TabbedContent.clear_pane` now returns an `AwaitComplete` instead of an `AwaitTabbedContent` https://github.com/Textualize/textual/pull/3498
+- `Tabs.add_tab` now returns an `AwaitComplete` instead of an `AwaitMount` https://github.com/Textualize/textual/pull/3498
+- `DirectoryTree.reload` now returns an `AwaitComplete`, which may be awaited to ensure the node has finished being processed by the internal queue https://github.com/Textualize/textual/pull/3498
+- `Tabs.remove_tab` now returns an `AwaitComplete`, which may be awaited to ensure the tab is unmounted and internal state is updated https://github.com/Textualize/textual/pull/3498
+- `App.switch_mode` now returns an `AwaitMount`, which may be awaited to ensure the screen is mounted https://github.com/Textualize/textual/pull/3498
 - Buttons will now display multiple lines, and have auto height https://github.com/Textualize/textual/pull/3539
 - DataTable now has a max-height of 100vh rather than 100%, which doesn't work with auto
 - Breaking change: empty rules now result in an error https://github.com/Textualize/textual/pull/3566
@@ -33,9 +49,14 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - Added `initial` to all css rules, which restores default (i.e. value from DEFAULT_CSS) https://github.com/Textualize/textual/pull/3566
 - Added HorizontalPad to pad.py https://github.com/Textualize/textual/pull/3571
 
+### Added
+
+- Added `AwaitComplete` class, to be used for optionally awaitable return values https://github.com/Textualize/textual/pull/3498
 
 ## [0.40.0] - 2023-10-11
 
+### Added
+
 - Added `loading` reactive property to widgets https://github.com/Textualize/textual/pull/3509
 
 ## [0.39.0] - 2023-10-10

diff --git a/src/textual/_animator.py b/src/textual/_animator.py
@@ -349,6 +349,7 @@ def _animate(
             duration is None and speed is not None
         ), "An Animation should have a duration OR a speed"
 
+        # If an animation is already scheduled for this attribute, unschedule it.
         animation_key = (id(obj), attribute)
         try:
             del self._scheduled[animation_key]
@@ -359,9 +360,7 @@ def _animate(
             final_value = value
 
         start_time = self._get_time()
-
         easing_function = EASING[easing] if isinstance(easing, str) else easing
-
         animation: Animation | None = None
 
         if hasattr(obj, "__textual_animation__"):

diff --git a/src/textual/app.py b/src/textual/app.py
@@ -1597,28 +1597,40 @@ def mount_all(
         """
         return self.mount(*widgets, before=before, after=after)
 
-    def _init_mode(self, mode: str) -> None:
+    def _init_mode(self, mode: str) -> AwaitMount:
         """Do internal initialisation of a new screen stack mode.
 
         Args:
             mode: Name of the mode.
+
+        Returns:
+            An optionally awaitable object which can be awaited until the screen
+            associated with the mode has been mounted.
         """
 
         stack = self._screen_stacks.get(mode, [])
-        if not stack:
+        if stack:
+            await_mount = AwaitMount(stack[0], [])
+        else:
             _screen = self.MODES[mode]
             new_screen: Screen | str = _screen() if callable(_screen) else _screen
-            screen, _ = self._get_screen(new_screen)
+            screen, await_mount = self._get_screen(new_screen)
             stack.append(screen)
             self._load_screen_css(screen)
+
         self._screen_stacks[mode] = stack
+        return await_mount
 
-    def switch_mode(self, mode: str) -> None:
+    def switch_mode(self, mode: str) -> AwaitMount:
         """Switch to a given mode.
 
         Args:
             mode: The mode to switch to.
 
+        Returns:
+            An optionally awaitable object which waits for the screen associated
+                with the mode to be mounted.
+
         Raises:
             UnknownModeError: If trying to switch to an unknown mode.
         """
@@ -1629,13 +1641,19 @@ def switch_mode(self, mode: str) -> None:
         self.screen.refresh()
 
         if mode not in self._screen_stacks:
-            self._init_mode(mode)
+            await_mount = self._init_mode(mode)
+        else:
+            await_mount = AwaitMount(self.screen, [])
+
         self._current_mode = mode
         self.screen._screen_resized(self.size)
         self.screen.post_message(events.ScreenResume())
+
         self.log.system(f"{self._current_mode!r} is the current mode")
         self.log.system(f"{self.screen} is active")
 
+        return await_mount
+
     def add_mode(
         self, mode: str, base_screen: str | Screen | Callable[[], Screen]
     ) -> None:

diff --git a/src/textual/await_complete.py b/src/textual/await_complete.py
@@ -0,0 +1,48 @@
+from __future__ import annotations
+
+from asyncio import Future, gather
+from typing import Any, Coroutine, Iterator, TypeVar
+
+import rich.repr
+
+ReturnType = TypeVar("ReturnType")
+
+
+@rich.repr.auto(angular=True)
+class AwaitComplete:
+    """An 'optionally-awaitable' object."""
+
+    def __init__(self, *coroutines: Coroutine[Any, Any, Any]) -> None:
+        """Create an AwaitComplete.
+
+        Args:
+            coroutine: One or more coroutines to execute.
+        """
+        self.coroutines = coroutines
+        self._future: Future = gather(*self.coroutines)
+
+    async def __call__(self) -> Any:
+        return await self
+
+    def __await__(self) -> Iterator[None]:
+        return self._future.__await__()
+
+    @property
+    def is_done(self) -> bool:
+        """Returns True if the task has completed."""
+        return self._future.done()
+
+    @property
+    def exception(self) -> BaseException | None:
+        """An exception if it occurred in any of the coroutines."""
+        if self._future.done():
+            return self._future.exception()
+        return None
+
+    @classmethod
+    def nothing(cls):
+        """Returns an already completed instance of AwaitComplete."""
+        instance = cls()
+        instance._future = Future()
+        instance._future.set_result(None)  # Mark it as completed with no result
+        return instance
diff --git a/src/textual/widget.py b/src/textual/widget.py
@@ -8,7 +8,6 @@
 from collections import Counter
 from fractions import Fraction
 from itertools import islice
-from operator import attrgetter
 from types import TracebackType
 from typing import (
     TYPE_CHECKING,

diff --git a/src/textual/widgets/_button.py b/src/textual/widgets/_button.py
@@ -153,9 +153,6 @@ class Button(Static, can_focus=True):
 
     BINDINGS = [Binding("enter", "press", "Press Button", show=False)]
 
-    ACTIVE_EFFECT_DURATION = 0.3
-    """When buttons are clicked they get the `-active` class for this duration (in seconds)"""
-
     label: reactive[TextType] = reactive[TextType]("")
     """The text label that appears within the button."""
 
@@ -211,6 +208,9 @@ def __init__(
 
         self.variant = self.validate_variant(variant)
 
+        self.active_effect_duration = 0.3
+        """Amount of time in seconds the button 'press' animation lasts."""
+
     def __rich_repr__(self) -> rich.repr.Result:
         yield from super().__rich_repr__()
         yield "variant", self.variant, "default"
@@ -266,10 +266,11 @@ def press(self) -> Self:
 
     def _start_active_affect(self) -> None:
         """Start a small animation to show the button was clicked."""
-        self.add_class("-active")
-        self.set_timer(
-            self.ACTIVE_EFFECT_DURATION, partial(self.remove_class, "-active")
-        )
+        if self.active_effect_duration > 0:
+            self.add_class("-active")
+            self.set_timer(
+                self.active_effect_duration, partial(self.remove_class, "-active")
+            )
 
     def action_press(self) -> None:
         """Activate a press of the button."""

diff --git a/src/textual/widgets/_data_table.py b/src/textual/widgets/_data_table.py
@@ -1116,9 +1116,13 @@ def move_cursor(
             cursor_row = row
         if column is not None:
             cursor_column = column
-
         destination = Coordinate(cursor_row, cursor_column)
         self.cursor_coordinate = destination
+
+        # Scroll the cursor after refresh to ensure the virtual height
+        # (calculated in on_idle) has settled. If we tried to scroll before
+        # the virtual size has been set, then it might fail if we added a bunch
+        # of rows then tried to immediately move the cursor.
         self.call_after_refresh(self._scroll_cursor_into_view, animate=animate)
 
     def _highlight_coordinate(self, coordinate: Coordinate) -> None:
@@ -1231,7 +1235,6 @@ def _update_column_widths(self, updated_cells: set[CellKey]) -> None:
             column = self.columns.get(column_key)
             if column is None:
                 continue
-
             console = self.app.console
             label_width = measure(console, column.label, 1)
             content_width = column.content_width
@@ -1289,8 +1292,6 @@ def _update_dimensions(self, new_rows: Iterable[RowKey]) -> None:
             if row.auto_height:
                 auto_height_rows.append((row_index, row, cells_in_row))
 
-        self._clear_caches()
-
         # If there are rows that need to have their height computed, render them correctly
         # so that we can cache this rendering for later.
         if auto_height_rows:
@@ -1616,11 +1617,9 @@ def remove_row(self, row_key: RowKey | str) -> None:
         Raises:
             RowDoesNotExist: If the row key does not exist.
         """
-
         if row_key not in self._row_locations:
             raise RowDoesNotExist(f"Row key {row_key!r} is not valid.")
 
-        self._new_rows.discard(row_key)
         self._require_update_dimensions = True
         self.check_idle()
 

diff --git a/src/textual/widgets/_directory_tree.py b/src/textual/widgets/_directory_tree.py
@@ -5,6 +5,8 @@
 from pathlib import Path
 from typing import TYPE_CHECKING, Callable, ClassVar, Iterable, Iterator
 
+from ..await_complete import AwaitComplete
+
 if TYPE_CHECKING:
     from typing_extensions import Self
 
@@ -152,18 +154,26 @@ def __init__(
         )
         self.path = path
 
-    def _add_to_load_queue(self, node: TreeNode[DirEntry]) -> None:
+    def _add_to_load_queue(self, node: TreeNode[DirEntry]) -> AwaitComplete:
         """Add the given node to the load queue.
 
+        The return value can optionally be awaited until the queue is empty.
+
         Args:
             node: The node to add to the load queue.
+
+        Returns:
+            An optionally awaitable object that can be awaited until the
+            load queue has finished processing.
         """
         assert node.data is not None
         if not node.data.loaded:
             node.data.loaded = True
             self._load_queue.put_nowait(node)
 
-    def reload(self) -> None:
+        return AwaitComplete(self._load_queue.join())
+
+    def reload(self) -> AwaitComplete:
         """Reload the `DirectoryTree` contents."""
         self.reset(str(self.path), DirEntry(self.PATH(self.path)))
         # Orphan the old queue...
@@ -172,7 +182,8 @@ def reload(self) -> None:
         self._loader()
         # We have a fresh queue, we have a fresh loader, get the fresh root
         # loading up.
-        self._add_to_load_queue(self.root)
+        queue_processed = self._add_to_load_queue(self.root)
+        return queue_processed
 
     def clear_node(self, node: TreeNode[DirEntry]) -> Self:
         """Clear all nodes under the given node.
@@ -202,6 +213,7 @@ def reset_node(
         """Clear the subtree and reset the given node.
 
         Args:
+            node: The node to reset.
             label: The label for the node.
             data: Optional data for the node.
 
@@ -213,16 +225,20 @@ def reset_node(
         node.data = data
         return self
 
-    def reload_node(self, node: TreeNode[DirEntry]) -> None:
+    def reload_node(self, node: TreeNode[DirEntry]) -> AwaitComplete:
         """Reload the given node's contents.
 
+        The return value may be awaited to ensure the DirectoryTree has reached
+        a stable state and is no longer performing any node reloading (of this node
+        or any other nodes).
+
         Args:
             node: The node to reload.
         """
         self.reset_node(
             node, str(node.data.path.name), DirEntry(self.PATH(node.data.path))
         )
-        self._add_to_load_queue(node)
+        return self._add_to_load_queue(node)
 
     def validate_path(self, path: str | Path) -> Path:
         """Ensure that the path is of the `Path` type.
@@ -239,13 +255,13 @@ def validate_path(self, path: str | Path) -> Path:
         """
         return self.PATH(path)
 
-    def watch_path(self) -> None:
+    async def watch_path(self) -> None:
         """Watch for changes to the `path` of the directory tree.
 
         If the path is changed the directory tree will be repopulated using
         the new value as the root.
         """
-        self.reload()
+        await self.reload()
 
     def process_label(self, label: TextType) -> Text:
         """Process a str or Text into a label. Maybe overridden in a subclass to modify how labels are rendered.
@@ -421,16 +437,17 @@ async def _loader(self) -> None:
                 # the tree.
                 if content:
                     self._populate_node(node, content)
-            # Mark this iteration as done.
-            self._load_queue.task_done()
+            finally:
+                # Mark this iteration as done.
+                self._load_queue.task_done()
 
-    def _on_tree_node_expanded(self, event: Tree.NodeExpanded) -> None:
+    async def _on_tree_node_expanded(self, event: Tree.NodeExpanded) -> None:
         event.stop()
         dir_entry = event.node.data
         if dir_entry is None:
             return
         if self._safe_is_dir(dir_entry.path):
-            self._add_to_load_queue(event.node)
+            await self._add_to_load_queue(event.node)
         else:
             self.post_message(self.FileSelected(event.node, dir_entry.path))