Skip to content
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

Test flakiness investigation and attempted fixes ❄ #3498

Merged
merged 61 commits into from
Oct 25, 2023
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
61 commits
Select commit Hold shift + click to select a range
6b02725
Modifying two flaky animation tests, hopefully removing flakiness :)
darrenburns Oct 10, 2023
37ff2cf
Make switch_mode return an AwaitMount
darrenburns Oct 10, 2023
f113057
Fix event issue
darrenburns Oct 10, 2023
be865f9
Merge branch 'main' of github.com:Textualize/textual into flaky-tests
darrenburns Oct 12, 2023
120feb0
Add AwaitComplete - a more generalised optionally awaitable object
darrenburns Oct 12, 2023
8525c53
Use AwaitComplete in Tabs.remove_tab() and update tests accordingly.
darrenburns Oct 12, 2023
2539df9
Update TabbedContent to use AwaitComplete instead of AwaitTabbedContent
darrenburns Oct 12, 2023
842bdf8
Simplifying - dont use optional awaitables where not required
darrenburns Oct 12, 2023
ab9f1cd
Update variable name
darrenburns Oct 12, 2023
65d7853
Update a comment
darrenburns Oct 12, 2023
0b4f3a9
Add watcher for cursor blink to ensure when blink is switched off, th…
darrenburns Oct 12, 2023
3741f59
Fix cursor blink reactive and disable cursor blink in the command pal…
darrenburns Oct 12, 2023
b73f69c
More progress
darrenburns Oct 12, 2023
86da626
Reworking AwaitComplete
darrenburns Oct 16, 2023
0faf7b2
Some more work on tabs flakiness/race-conditions
darrenburns Oct 16, 2023
8ecbe20
Merge branch 'main' of github.com:willmcgugan/textual into flaky-tests
darrenburns Oct 16, 2023
fa36cd4
Ensure active tab is set correctly
darrenburns Oct 16, 2023
c0fef47
Simplify next tab assignment
darrenburns Oct 16, 2023
2df0584
Simplify removing tabs logic
darrenburns Oct 16, 2023
5b0d78e
Make button animation duration configurable; Switch off button animat…
darrenburns Oct 16, 2023
cda2319
Remove a flawed test
darrenburns Oct 16, 2023
6c686ff
Add awaits in some tests
darrenburns Oct 16, 2023
68f2d2e
Docstrings
darrenburns Oct 16, 2023
d871c83
Make active_effect_duration an instance attribute
darrenburns Oct 16, 2023
710d6b5
Fix a Tabs crash
darrenburns Oct 16, 2023
1a7feec
Await the tree reload when the path changes in DirectoryTree
darrenburns Oct 16, 2023
b8428dd
Change AwaitComplete _instances class attr to a set from a list
darrenburns Oct 17, 2023
55c0950
Make AwaitComplete generic, AwaitComplete._wait_all is now private, a…
darrenburns Oct 17, 2023
ab28264
Actually make AwaitComplete instances a set, not a list
darrenburns Oct 17, 2023
4d64ca9
Update CHANGELOG.md regarding flaky-test adjacent changes, AwaitCompl…
darrenburns Oct 17, 2023
441882d
Remove whitespace
darrenburns Oct 17, 2023
91a5936
Merge branch 'main' of github.com:willmcgugan/textual into flaky-tests
darrenburns Oct 17, 2023
5238dd9
Use list() instead of useless comprehension, remove unused import
darrenburns Oct 17, 2023
7674b4f
Merge branch 'main' into flaky-tests
darrenburns Oct 17, 2023
0df54ef
Merge branch 'main' of github.com:willmcgugan/textual into flaky-tests
darrenburns Oct 18, 2023
63bcf41
Merge branch 'flaky-tests' of github.com:willmcgugan/textual into fla…
darrenburns Oct 18, 2023
d0f2971
Ensure loading indicator _start_time is initialised correctly
darrenburns Oct 18, 2023
eb15a59
Switch from time.sleep to asyncio.sleep in a notifications test, rewo…
darrenburns Oct 18, 2023
cba8978
Resolve deadlock by awaiting event on the event loop instead of in th…
darrenburns Oct 18, 2023
c837abe
Renaming for clarity
darrenburns Oct 18, 2023
6d06d2a
Debugging for remove_tab test flakiness
darrenburns Oct 19, 2023
1d4c28e
Running all tests
darrenburns Oct 19, 2023
c3766a9
Merge branch 'main' of github.com:willmcgugan/textual into flaky-tests
darrenburns Oct 24, 2023
2e93238
Updating snapshots
darrenburns Oct 24, 2023
5642681
Remove debugging prints
darrenburns Oct 24, 2023
5dd2f81
Fix broken docstring, remove unused import
darrenburns Oct 24, 2023
59572c7
Merge branch 'main' of github.com:willmcgugan/textual into flaky-tests
darrenburns Oct 24, 2023
62ac7e2
Rename variable to make it clearer
darrenburns Oct 24, 2023
a7bb953
Add missing return type annotation
darrenburns Oct 24, 2023
4b9bce3
Update src/textual/widgets/_tabbed_content.py
darrenburns Oct 24, 2023
0582f54
Update src/textual/widgets/_tabbed_content.py
darrenburns Oct 24, 2023
65c97c8
Update src/textual/widgets/_tabs.py
darrenburns Oct 24, 2023
b4de92b
Scroll datatable cursor after refresh
darrenburns Oct 25, 2023
9ec4680
Add comment explaining use of call_after_refresh when scrolling data …
darrenburns Oct 25, 2023
15c308d
Merge branch 'flaky-tests' of github.com:willmcgugan/textual into fla…
darrenburns Oct 25, 2023
547a918
Add repr to AwaitComplete (auto-repr_
darrenburns Oct 25, 2023
6104481
Remove use of generics from AwaitComplete
darrenburns Oct 25, 2023
d9feeda
Update changelog and improve docstring
darrenburns Oct 25, 2023
1238870
Add a missing parameter from DirectoryTree.reset_node docstring.
darrenburns Oct 25, 2023
c5be6af
Improve docstring in DirectoryTree
darrenburns Oct 25, 2023
c238dac
Rename parameter coroutine to coroutines in await_complete.py, since …
darrenburns Oct 25, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/textual/_animator.py
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand All @@ -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__"):
Expand Down
27 changes: 22 additions & 5 deletions src/textual/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -1595,28 +1595,39 @@ 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 indicating whether this mode
darrenburns marked this conversation as resolved.
Show resolved Hide resolved
"""

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.
"""
Expand All @@ -1627,13 +1638,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:
Expand Down
60 changes: 60 additions & 0 deletions src/textual/await_complete.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
from __future__ import annotations

from asyncio import Future, gather, wait
from typing import Coroutine


class AwaitComplete:
darrenburns marked this conversation as resolved.
Show resolved Hide resolved
"""An 'optionally-awaitable' object."""

_instances: list["AwaitComplete"] = []
darrenburns marked this conversation as resolved.
Show resolved Hide resolved
"""Track all active instances of AwaitComplete."""

def __init__(self, *coroutine: Coroutine) -> None:
darrenburns marked this conversation as resolved.
Show resolved Hide resolved
"""Create an AwaitComplete.

Args:
coroutine: One or more coroutines to execute.
"""
self.coroutine = coroutine
AwaitComplete._instances.append(self)
self._future: Future = gather(*[coroutine for coroutine in self.coroutine])
darrenburns marked this conversation as resolved.
Show resolved Hide resolved
self._future.add_done_callback(self._on_done)

async def __call__(self):
await self

def __await__(self):
return self._future.__await__()

def _on_done(self, _: Future) -> None:
"""Stop tracking this instance once it's done."""
AwaitComplete._instances.remove(self)

@property
def is_done(self) -> bool:
"""Returns True if the task has completed."""
return self._future is not None and self._future.done()
darrenburns marked this conversation as resolved.
Show resolved Hide resolved

@property
def exception(self) -> BaseException | None:
"""An exception if it occurred in any of the coroutines."""
if self._future and self._future.done():
return self._future.exception()
return None

@classmethod
async def wait_all(cls):
darrenburns marked this conversation as resolved.
Show resolved Hide resolved
"""Await all instances of AwaitComplete."""
await wait(
[instance._future for instance in cls._instances if instance._future],
timeout=1.0,
)

@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
15 changes: 8 additions & 7 deletions src/textual/widgets/_button.py
Original file line number Diff line number Diff line change
Expand Up @@ -151,9 +151,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."""

Expand Down Expand Up @@ -209,6 +206,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"
Expand Down Expand Up @@ -252,10 +252,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."""
Expand Down
28 changes: 17 additions & 11 deletions src/textual/widgets/_directory_tree.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -152,7 +154,7 @@ 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:
Copy link
Contributor

@rodrigogiraoserrao rodrigogiraoserrao Oct 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docstring is missing the return.
If I'm understanding this, the await complete we get is one that waits for the whole load queue to be processed, right?

I don't know how Queue works, but if you call join and then add more nodes to the load queue, won't the previously called join also wait for those nodes to be loaded?

Copy link
Member Author

@darrenburns darrenburns Oct 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might be on to something - I thought join() returned a Future. I'll investigate this more.
(I misread)

Yes, if you call join and then add more nodes to the queue, the join will wait until the queue is completely empty. An alternative would be to post a "marker" message on to the queue and wait for that to be processed, then stop waiting - maybe that'd be better for since some people might be polling their filesystem for changes faster than the queue is processed.

rodrigogiraoserrao marked this conversation as resolved.
Show resolved Hide resolved
"""Add the given node to the load queue.

Args:
Expand All @@ -163,7 +165,9 @@ def _add_to_load_queue(self, node: TreeNode[DirEntry]) -> None:
node.data.loaded = True
self._load_queue.put_nowait(node)

def reload(self) -> None:
return AwaitComplete(self._load_queue.join())

def reload(self) -> AwaitComplete:
darrenburns marked this conversation as resolved.
Show resolved Hide resolved
"""Reload the `DirectoryTree` contents."""
self.reset(str(self.path), DirEntry(self.PATH(self.path)))
# Orphan the old queue...
Expand All @@ -172,7 +176,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.
Expand Down Expand Up @@ -213,7 +218,7 @@ def reset_node(
node.data = data
return self

def reload_node(self, node: TreeNode[DirEntry]) -> None:
def reload_node(self, node: TreeNode[DirEntry]) -> AwaitComplete:
darrenburns marked this conversation as resolved.
Show resolved Hide resolved
"""Reload the given node's contents.

Args:
Expand All @@ -222,7 +227,7 @@ def reload_node(self, node: TreeNode[DirEntry]) -> None:
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.
Expand All @@ -239,13 +244,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.
Expand Down Expand Up @@ -421,16 +426,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):
darrenburns marked this conversation as resolved.
Show resolved Hide resolved
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))

Expand Down
23 changes: 18 additions & 5 deletions src/textual/widgets/_input.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
from ..message import Message
from ..reactive import reactive
from ..suggester import Suggester, SuggestionReady
from ..timer import Timer
from ..validation import ValidationResult, Validator
from ..widget import Widget

Expand Down Expand Up @@ -157,7 +158,7 @@ class Input(Widget, can_focus=True):
}
"""

cursor_blink = reactive(True)
cursor_blink = reactive(True, init=False)
value = reactive("", layout=True, init=False)
input_scroll_offset = reactive(0)
cursor_position = reactive(0)
Expand Down Expand Up @@ -255,6 +256,9 @@ def __init__(
if value is not None:
self.value = value

self._blink_timer: Timer | None = None
"""Timer controlling the blinking of the cursor, instantiated in `on_mount`."""

self.placeholder = placeholder
self.highlighter = highlighter
self.password = password
Expand Down Expand Up @@ -330,6 +334,15 @@ def _watch_cursor_position(self) -> None:

self.app.cursor_position = self.cursor_screen_offset

def _watch_cursor_blink(self, blink: bool) -> None:
"""Ensure we handle updating the cursor blink at runtime."""
if self._blink_timer is not None:
if blink:
self._blink_timer.resume()
else:
self._cursor_visible = True
self._blink_timer.pause()
Comment on lines +339 to +344
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be independent of whether the timer exists or not, in case the user gets rid of the timer or some weird edge case pops up?
I.e., shouldn't we have

Suggested change
if self._blink_timer is not None:
if blink:
self._blink_timer.resume()
else:
self._cursor_visible = True
self._blink_timer.pause()
if self._blink_timer is not None:
if blink:
self._blink_timer.resume()
else:
self._blink_timer.pause()
if not blink:
self._cursor_visible = True

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only reason the if self._blink_timer is not None guard exists is because this watcher can run before the Input is mounted, and the _blink_timer isn't initialised until on_mount.

I don't think we have to worry about the timer disappearing or anything.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly. So, what if the user sets cursor visibility before mounting the widget?
Then, it'll expect cursor visibility to have been set but it won't have been because we skipped the set altogether.

E.g., in a situation like this:

def compose(self):
    inp = Input()
    inp.cursor_blink = False
    yield inp

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If they set the blink before the mount, then the blink timer will still be initialised in on_mount, but it will start as paused:

    def _on_mount(self, _: Mount) -> None:
        self._blink_timer = self.set_interval(
            0.5,
            self._toggle_cursor,
            pause=not (self.cursor_blink and self.has_focus),
        )


@property
def cursor_screen_offset(self) -> Offset:
"""The offset of the cursor of this input in screen-space. (x, y)/(column, row)"""
Expand Down Expand Up @@ -419,27 +432,27 @@ def _toggle_cursor(self) -> None:
self._cursor_visible = not self._cursor_visible

def _on_mount(self, _: Mount) -> None:
self.blink_timer = self.set_interval(
self._blink_timer = self.set_interval(
0.5,
self._toggle_cursor,
pause=not (self.cursor_blink and self.has_focus),
)

def _on_blur(self, _: Blur) -> None:
self.blink_timer.pause()
self._blink_timer.pause()
if "blur" in self.validate_on:
self.validate(self.value)

def _on_focus(self, _: Focus) -> None:
self.cursor_position = len(self.value)
if self.cursor_blink:
self.blink_timer.resume()
self._blink_timer.resume()
self.app.cursor_position = self.cursor_screen_offset

async def _on_key(self, event: events.Key) -> None:
self._cursor_visible = True
if self.cursor_blink:
self.blink_timer.reset()
self._blink_timer.reset()

if event.is_printable:
event.stop()
Expand Down
Loading
Loading