Mock time during integration tests #507

olivierphi · 2022-05-13T11:58:28Z

Finally, it seems to work! (even on Windows, even though it was harder there because of asyncio 😔 )

Why I used classic mocking rather than dependencies injection

In the end I didn't opt for a passing of a get_time function in our class constructors, because in order to be able to simulate the time we also have to mock await asyncio.wait([duration]), so that would be one more function to pass to all these constructors which would probably make the code less nice to work with.

That's why I opted for good old mocks instead, which limits the impact of this PR on existing code - the only changes I had to make were to factorise a bit more our calls to time.monotonic().

No use of `time_machine` for the moment

I could have used time_machine; however, we're clearly not in the case of a classic stateless Web backend, where the state is entirely in databases rather than in the code.
In our own case, on top of calls to time.monotonic() we also use await asyncio.wait([duration]) to control the time, which to me makes tools such a time_machine less relevant?

closes #499
closes #500

olivierphi · 2022-05-17T10:05:36Z

src/textual/_animator.py

 from typing import Any, Callable, TypeVar

 from dataclasses import dataclass

-from . import log


Don't mind me, removing some unused imports while I'm there... 😄

olivierphi · 2022-05-17T10:06:11Z

src/textual/_animator.py

@@ -3,14 +3,11 @@
 from abc import ABC, abstractmethod
 import asyncio
 import sys
-from time import monotonic


removing occurrences of this as much as possible in the code, so we have less stuff to mock during our integration tests

olivierphi · 2022-05-17T10:15:56Z

src/textual/_animator.py

@@ -186,10 +179,13 @@ def animate(
            raise AttributeError(
                f"Can't animate attribute {attribute!r} on {obj!r}; attribute does not exist"
            )
+        assert not all(


just an additional check while I was there

Both values could be 0. This check should probably be explicit when checking for None. You probably don't want to use all for something like this, an expressions is probably more efficient.

olivierphi · 2022-05-17T10:16:27Z

src/textual/_animator.py

@@ -139,10 +136,6 @@ def __init__(self, target: MessageTarget, frames_per_second: int = 60) -> None:
            pause=True,
        )

-    def get_time(self) -> float:


Unless I'm missing something, I don't think we have a reason to maintain this as a public method?

olivierphi · 2022-05-17T10:23:14Z

src/textual/_context.py

@@ -5,4 +5,9 @@
 if TYPE_CHECKING:
    from .app import App

+
+class NoActiveAppError(RuntimeError):


We're catching plain Python LookupError in some of our code in order to not interrupt what we're doing when there is no "active App" registered in the current asyncio context - this is the raw error that is raised by ContextVar.get() under the hood.

It works of course, but as I was running in cases where this was happening in the integration tests I noticed that it was not super clear for me as to why we do this in the code that does this errors interception, like there:

try: self.app.stylesheet.update(self.app, animate=True) self.refresh() except LookupError: # why would a LookupError pop here? Not crystal clear (to me at least 😅) pass

That's why I changed that to to the following:

try: self.app.stylesheet.update(self.app, animate=True) self.refresh() except NoActiveAppError: # more explicit. hopefully? pass

It shouldn't be necessary to catch this case, except internally. But it could leak out, and I like that it's explicit

olivierphi · 2022-05-17T10:52:12Z

tests/test_integration_scrolling.py

-        [SCREEN_SIZE, 10, "placeholder_7", False, 0.01, (3, 4, 5, 6, 7), "others"],
-        [SCREEN_SIZE, 10, "placeholder_9", False, 0.01, (5, 6, 7, 8, 9), "others"],
+        [SCREEN_SIZE, 10, None, None, 0.01, (0, 1, 2, 3, 4)],
+        [SCREEN_SIZE, 10, "placeholder_3", False, 0.01, (0, 1, 2, 3, 4)],


now that this test gives deterministic results, thanks to our mocking of the time, we can remove this last_screen_expected_out_of_viewport_placeholder_ids parameter form the test params, and simply check that all the elements that are not in the "supposed to be visible" list are well and truly not visible :-)

tests/test_integration_scrolling.py

olivierphi · 2022-05-17T10:53:25Z

tests/utilities/test_app.py

+                run_task = asyncio.create_task(run_app())
+
+                # We have to do this because `run_app()` is running in its own async task, and our test is going to
+                # run in this one - so the app must also be the active App in our current context:


took me a while to figure this one out! 😅

olivierphi · 2022-05-17T10:59:06Z

tests/utilities/test_app.py

+# > The resolution of the monotonic clock on Windows is usually around 15.6 msec.
+# > The best resolution is 0.5 msec.
+# @link https://docs.python.org/3/library/asyncio-platforms.html:
+ASYNCIO_EVENTS_PROCESSING_REQUIRED_PERIOD = 0.025 if WINDOWS else 0.002


Not very elegant, but it definitely seem that the granularity of asyncio is not the same for Windows than for Unix-y OSes - as specific by the Python documentation.
So after we triggered some Timers in our "mocked time" machinery it seems that we have to await asyncio.wait() for a much longer period in Windows if we want asyncio pending events to be processed during this time.

And uvloop is not even available for Windows, so we cannot rely on it to make the integration tests faster on Windows 😔
(which is a bit ironic as uvloop is a Python wrapper of libuv, which was especially created to make Node.js work on Windows, back in the days)

olivierphi · 2022-05-17T11:01:07Z

tests/utilities/test_app.py

+
+            ticks_count = ceil(seconds * ticks_granularity_fps)
+            activated_timers_count_total = 0
+            for tick_counter in range(ticks_count):


naive implementation for the moment: for each simulated second we loop 60 times, and for each tick we check if we have some mocked calls to asyncio.wait([duration]) to trigger

olivierphi · 2022-05-17T11:12:01Z

tests/utilities/test_app.py

+
+            return activated_timers_count
+
+        with mock.patch("textual._timer._TIMERS_CAN_SKIP", new=False), mock.patch(


we end up with 3 things to mock in our tests to simulate time in Textual; not too bad! 🙂

willmcgugan

After a discussion, I think what we need is a Clock class with two methods; get_time and sleep which by default would return the monotonic time and call asyncio.sleep. This would essentially replicate the current behaviour.

The get_time method should probably be a coroutine.

When testing we can replace this with a idealize clock which can be mocked. An update_time method would advance the time, and the sleep method would wake coroutines according to the test time.

This should help us avoid some ackward logic in tests.

willmcgugan · 2022-05-17T13:40:57Z

src/textual/_animator.py

@@ -186,10 +179,13 @@ def animate(
            raise AttributeError(
                f"Can't animate attribute {attribute!r} on {obj!r}; attribute does not exist"
            )
+        assert not all(


Both values could be 0. This check should probably be explicit when checking for None. You probably don't want to use all for something like this, an expressions is probably more efficient.

willmcgugan · 2022-05-17T13:41:30Z

src/textual/_animator.py

            animation_keys = list(self._animations.keys())
            for animation_key in animation_keys:
                animation = self._animations[animation_key]
                if animation(animation_time):
                    del self._animations[animation_key]
+
+    def _get_time(self) -> float:


Makes sense.

willmcgugan · 2022-05-17T13:43:18Z

src/textual/_context.py

@@ -5,4 +5,9 @@
 if TYPE_CHECKING:
    from .app import App

+
+class NoActiveAppError(RuntimeError):


It shouldn't be necessary to catch this case, except internally. But it could leak out, and I like that it's explicit

olivierphi · 2022-05-17T16:18:53Z

@willmcgugan As discussed, I added a new _clock module, exposing 3 functions that should now be the single source of truth for anything related to time in Textual: f5969fb 🙂

My initial implementation was made of a Clock class in this module, and a singleton of it exposed as _clock.clock .
However, the issue with this is that mocking the singleton was not effective, because other Python modules were using from ._clock import clock, according to the existing code style where we use direct imports from modules.
Because of this it was too late for the patch, as a direct reference to this non-patched singleton were already created in all these modules here and there - that would not be impacted by the mocking of the singleton later on.

To solve this I opted for a slightly different approach, where I expose only 3 plain functions in this _clock module, which are simple proxys for calls to such a _clock singleton - the difference being that the singleton and its underlying class are hidden APIs, so we can mock the singleton and be sure that other Python modules didn't have any direct reference to it before the patch.
What do you think?

willmcgugan

Some requests...

willmcgugan · 2022-05-18T15:15:27Z

src/textual/_clock.py

+    return _clock.get_time()
+
+
+async def aget_time() -> float:


Not keen on the name. I have a policy of no abbreviations.

I'm not a fan at all either to be honest 😅 - I just opted for that a prefix because it seems to become the idiomatic way to make things "asyncio-related" in Python?

i.e. object.__enter__ and object.__exit__ become object.__aenter__ and object.__aexit__ for async contexts in the Python data model.

And we find the same pattern in Django now: aget_or_create, acount, aaggregate, etc .

django/django#14843

You’re absolutely right, but I think we can avoid it in Textual.

Perhaps get_time for the coroutine and get_time_no_wait for the sync function? It’s consistent with some other Textual methods.

Consider using the "await me maybe" pattern 👀

https://simonwillison.net/2020/Sep/2/await-me-maybe/

Consider using the "await me maybe" pattern eyes

https://simonwillison.net/2020/Sep/2/await-me-maybe/

This pattern is good, and we use it already in Textual (for event listeners for example) 🙂 , but the problem with it is that the caller must be in an async context itself to use it - which is not the case for some calls we need to make to these clock-related functions, as they can be called in some classes' constructors for example

willmcgugan · 2022-05-18T15:17:31Z

tests/utilities/test_app.py

+        # (which can only be "unlocked" by calls to `move_clock_forward()`)
+        await event.wait()
+
+    async def move_clock_forward(self, *, seconds: float) -> tuple[float, int]:


Suggest "advance_clock"

Does it need to be a keyword only argument? We generally exclusively use seconds for time.

willmcgugan · 2022-05-18T15:22:45Z

tests/utilities/test_app.py

+
+        # let's make the time advance _very_ slightly between 2 consecutive calls of this function,
+        # within the same order of magnitude than 2 consecutive calls to ` timer.monotonic()`:
+        self._current_time += 1.1e-06


Not keen on this. I think it may create spin loops and it may create non-deterministic output.

The get_time should bean idealized version of time that advances in predictable increments.

willmcgugan · 2022-05-18T15:30:26Z

tests/utilities/test_app.py

+            return  # the app may not have a screen yet
+
+        # We artificially tell the Compositor that the whole area should be refreshed
+        screen._compositor._dirty_regions = {


I think I might expose this on the Screen object to avoid digging in to the internals.

willmcgugan · 2022-05-18T15:31:57Z

tests/utilities/test_app.py

+        self._pending_sleep_events.append((target_event_monotonic_time, event))
+        # Ok, let's wait for this Event
+        # (which can only be "unlocked" by calls to `move_clock_forward()`)
+        await event.wait()


olivierphi · 2022-05-19T09:06:00Z

@willmcgugan here you go, feedback hopefully addressed! 🙂 ca6d7be

willmcgugan · 2022-05-20T10:46:28Z

tests/utilities/test_app.py

+            event.set()
+            activated_timers_count += 1
+            # ...and remove it from our pending sleep events list:
+            del self._pending_sleep_events[i]


This is mutating a list being iterated over, which may cause unexpected side-effects. Deleting indexes from a list can be problematic. Suggest a different structure, dict or set...

I converted the list into a dict, which is also a much more elegant design 👌 0ee9c57

Oh and while I was there I also (arbitrarily) increased the non-Windows duration of the "atomic waiting time to let asyncio do its stuff after we advanced the clock", hopefully this will make the test suite pass on your local own environment! 🤞

I had to add a flag in the `_timer` module that allows us to completely disable the "skip" feature of Timers, though - but it shouldn't cause too much trouble 🤞

…time This makes time quite easier to mock during integration tests :-)

…entralised Clock

…entralised Clock, episode II

olivierphi · 2022-05-20T14:54:39Z

@willmcgugan could that be the last commit of this PR before its merging? Time will tell! 😄 631f7a5

willmcgugan · 2022-05-23T16:09:53Z

Thanks for sticking with it!

olivierphi force-pushed the try-mocking-time-during-integration-tests branch 6 times, most recently from d54cc11 to a194c29 Compare May 17, 2022 10:39

olivierphi commented May 17, 2022

View reviewed changes

olivierphi force-pushed the try-mocking-time-during-integration-tests branch from a194c29 to 6669216 Compare May 17, 2022 11:09

olivierphi commented May 17, 2022

View reviewed changes

olivierphi marked this pull request as ready for review May 17, 2022 11:31

olivierphi requested review from willmcgugan and darrenburns May 17, 2022 11:31

willmcgugan requested changes May 17, 2022

View reviewed changes

olivierphi force-pushed the try-mocking-time-during-integration-tests branch from 303f964 to f5969fb Compare May 17, 2022 16:15

olivierphi requested a review from willmcgugan May 17, 2022 16:19

willmcgugan requested changes May 18, 2022

View reviewed changes

willmcgugan requested changes May 20, 2022

View reviewed changes

Olivier Philippon added 6 commits May 20, 2022 14:49

[e2e] Add a way to accelerate time in our integration tests

74ad6f7

[App] Finally, time mocking in tests seems to be working! 😅

15df759

I had to add a flag in the `_timer` module that allows us to completely disable the "skip" feature of Timers, though - but it shouldn't cause too much trouble 🤞

[App] Integration tests now work on Windows too

5789816

[clock] Add a centralised Clock, responsible for anything related to …

4549f2d

…time This makes time quite easier to mock during integration tests :-)

[clock] Address PR feedback after the initial implementation of the c…

aed2528

…entralised Clock

[clock] Address PR feedback after the initial implementation of the c…

0ee9c57

…entralised Clock, episode II

olivierphi force-pushed the try-mocking-time-during-integration-tests branch from ca6d7be to 0ee9c57 Compare May 20, 2022 13:51

olivierphi requested a review from willmcgugan May 20, 2022 13:57

[clock] Misc bug fixes 🐛

631f7a5

willmcgugan approved these changes May 23, 2022

View reviewed changes

willmcgugan merged commit c212fd5 into css May 23, 2022

willmcgugan deleted the try-mocking-time-during-integration-tests branch May 23, 2022 16:09

olivierphi mentioned this pull request May 24, 2022

Text input improvements #528

Merged

olivierphi mentioned this pull request Jun 24, 2022

Use time_machine for integration tests #500

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mock time during integration tests #507

Mock time during integration tests #507

olivierphi commented May 13, 2022 •

edited

Loading

olivierphi May 17, 2022

olivierphi May 17, 2022

olivierphi May 17, 2022

willmcgugan May 17, 2022

olivierphi May 17, 2022

olivierphi May 17, 2022

willmcgugan May 17, 2022

olivierphi May 17, 2022

olivierphi May 17, 2022

olivierphi May 17, 2022

olivierphi May 17, 2022

olivierphi May 17, 2022

willmcgugan left a comment

willmcgugan May 17, 2022

willmcgugan May 17, 2022

willmcgugan May 17, 2022

olivierphi commented May 17, 2022

willmcgugan left a comment

willmcgugan May 18, 2022

olivierphi May 18, 2022

willmcgugan May 18, 2022

tusharsadhwani May 18, 2022

olivierphi May 19, 2022

willmcgugan May 18, 2022

willmcgugan May 18, 2022

willmcgugan May 18, 2022

willmcgugan May 18, 2022

olivierphi commented May 19, 2022

willmcgugan May 20, 2022

olivierphi May 20, 2022

olivierphi commented May 20, 2022

willmcgugan commented May 23, 2022


		return activated_timers_count

		with mock.patch("textual._timer._TIMERS_CAN_SKIP", new=False), mock.patch(

Mock time during integration tests #507

Mock time during integration tests #507

Conversation

olivierphi commented May 13, 2022 • edited Loading

Why I used classic mocking rather than dependencies injection

No use of time_machine for the moment

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

Choose a reason for hiding this comment

willmcgugan left a 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

olivierphi commented May 17, 2022

willmcgugan left a 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

olivierphi commented May 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olivierphi commented May 20, 2022

willmcgugan commented May 23, 2022

olivierphi commented May 13, 2022 •

edited

Loading

No use of `time_machine` for the moment