Add ClockEventLoop class with fixture and test (close #95)

[derekbrokeit]

No description provided.

[spectras]

Hello.
Any news on merging that? It is a crucial tool to be able to unit test async code error condition number 1: timeouts.

[derekbrokeit]

I am revisiting this, but I am not sure I understand the source of the testing error that occurred. I will try to look into it today (or at least this week) in order to fix that merge conflict. That being said, unfortunately, running pytest did not cause failures on my own machine during the quick development.

Lastly, I may rename the event loop here, because I think the name ClockEventLoop may not be clear enough

[nicoddemus]

FWIW trio also uses "clock" for this: https://trio.readthedocs.io/en/latest/reference-testing.html

[asvetlov]

Should the change be a part of pytest-asyncio plugin?
You can extract the code into a separate project, isn't it?

[nicoddemus]

Should the change be a part of pytest-asyncio plugin?

IMHO yes, this is necessary to test timeouts properly and reliably, the included test is a good example of that.

trio includes a feature like this for testing as well, and even a little more powerful because it is more tied to the internals.

[asvetlov]

Why a separate pytest plugin cannot be used for testing timeouts properly and reliably?

[nicoddemus]

Why a separate pytest plugin cannot be used for testing timeouts properly and reliably?

Well, it is not a generic implementation that can be used by anyone, it is directly tied to asyncio (it's a new event loop), it is really simple (a new fixture) and doesn't change anything else in the plugin.

@asvetlov what are the problems you see with introducing this to pytest-asyncio?

[asvetlov]

Just because it is a simple isolated fixture it can be extracted into a separate library.

If you asking for the PR problems, I can point on.

1. ClockEventLoop doesn't advance wall clock automatically. It means that without explicit loop.advance_time(12.34) all timers don't work. Third-parties can have own timeout and other timer-related activities. Test code should advance the timer explicitly, otherwise a simple await asyncio.sleep(1) will sleep forever.
2. It calls a private _run_once() base method. Maybe not a big deal but a potential source of breaking forward compatibility.
3. ClockEventLoop is inherited from SelectorEventLoop which is not always desirable. UnixEventLoop and ProactorEventLoop are required to work with subprocesses, SelectorEventLoop doesn't support non-socket file descriptors (e.g. pipes) on Windows. It limits ClockEventLoop to very narrow use cases.

I think the PR is not mature enough. Better to make a separate library, battle test it on massive real code usage and after all raise the question about adding a new functionality here.

[nicoddemus]

All good points, thanks for pointing them out. 👍

From your first comment I understood you thought it didn't belong here on philosophy, not on technical merits, that's why I disagreed.

[spectras]

3. I don't think there is a "clean" way to get the correct class. Alas, the event loop policy has no public method that returns the class itself. The best I could reach so far is creating one, getting a ref to its class type and discarding it:

base = asyncio.new_event_loop().__class__
class AdjustableEventLoop(base):
    #...

2. Doing away with the _run_once() call is not only possible, but a good idea. The event loop is not reentrant, and there is a better way: simply yield back to running loop with asyncio.sleep(0).

1. instead of setting time manually, the loop could maintain an offset.

So altering proposed solution:

base = asyncio.new_event_loop().__class__
class ClockEventLoop(base):
    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self._adjustable_offset = 0

    def time(self):
        return super().time() + self._adjustable_offset

    async def advance_time(self, seconds):
        if seconds < 0:
            raise ValueError('cannot go backwards in time')
        await asyncio.sleep(0)    # let event loop cycle complete and scheduled callbacks run
        self._adjustable_offset += seconds
        await asyncio.sleep(0)    # trigger callbacks that expired in the interval

Note that this will jump in time, not "fast forward" time. This should be fine, since event loop only guarantees minimum delay. That's what I do in my own tests anyway.

[derekbrokeit]

You all have made very interesting points. Let me see if I can reconfigure this to address your concerns. If it can be included in pytest-asyncio instead of a separate library, I think it belongs, but the concerns are not without merit.

[derekbrokeit]

I've made some adjustments based on your comments:

1. Instead of using SelectorEventLoop, it uses the default type given by asyncio.new_event_loop().__class__ as suggested by @spectras
2. Using an offset-based event loop to ensure that anomalous hanging is not unexpected. Therefore, the advancement of time ensures that you have advanced past the offset, but this does not gaurantee* exact timing as the previous method did.
3. Using a coroutine method for advance_time as suggested by @spectras in order to not use private method _run_once. The unfortunate side effect of this is that using advance_time requires application in a coroutine, but that should be fine for most cases.
4. Using seconds as the parameter in advance_time because it's meaning is more clear
5. added some better documentation.
6. added asyncio_clock for marking test coroutine functions that must be run in a clock event loop. These functions can either get a clock_event_loop fixture or asyncio.get_event_loop() to get the instance of the loop for advance_time method access.
7. Merged in the latest master
8. all tests in test_simple_35.py should pass, I am not sure if this is right place for my 3 tests. However, some other modules have some unexpected errors on my own machine (and travis).

[derekbrokeit]

In the current arrangement, assuming advance_time is not called, event_loop and clock_event_loop should act the same. Would it be reasonable to suggest that this be the default of testing?

[derekbrokeit]

I made one tweak, I removed the exception raised by advance_time because users may be performing calculations to estimate advancement of time. If the calculation resulted in a negative (already past due) then the ValueError would be unexpected. Instead, I think returning immediately is the better solution.

[spectras]

Shouldn't it be before clock advance (line 219) then?

[derekbrokeit]

Let me take a closer look at these sleep calls. I recall as I was writing it that I had difficulty achieving a predictable number of loop iterations to get the test to react as expected. That's how I settled on sleeping 3 times. It is also the reason I ended up using _run_once in the earlier version and not using a coroutine function because it's functionality was more clear (even though it was a private method call).

[spectras]

One should be enough. The point of having two was to have one before clock advance and one after it.

[spectras]

To make test reliable I tend to await explicitly:

TIMEOUT=1    #define earlier in the file

await asyncio.wait_for(task, TIMEOUT)

(and change short_nap/advance to 10)

I do this because though advance_time marks task's sleep as finished, but the exact number of event loop iterations between this and the actual closing of the task is implementation-dependent.

[asvetlov]

The line picks up the look class at import time.
But the default loop factory can be changed in runtime by installing a custom policy for example.
I recall many projects that change the loop factory in conftest.py or top-level package's __init__.py.
For example, the PR cannot test clock-event-loop against proactor event loop on Windows even if the policy was explicitly changed.

Honestly, I don't know how to solve the problem in unambiguous way but want to point on the problem.

[derekbrokeit]

I understand the problem here, but the solution is not immediately obvious. Let me take a crack at seeing if I can make a function that produces a class to solve this.

[derekbrokeit]

Fixes for given questions posed

• ClockEventLoop is developed in a function to allow for later changes
  to event loop policies
• rework advance_time method to better symbolize the needed loop
  iterations. This is marked by a change to a function that returns
  an awaitable.

As for using wait_for for the testing. That test will also pass, but it actually does not need to be performed. The problem is that after advance_time returns, it is has already passed the TIMEOUT, it does not throw a TimeoutError because it didn't have enough loops to get to that scheduled task yet. In my opinion, it doesn't need to be performed and may make it difficult to understand.

Note: I had to review Task.all_tasks() just to check through and make sure looping was working as expected.

[derekbrokeit]

Additional thoughts I have had regarding user's desires to pick alternative event-loops. I don't see a good way to change after the fixture is called, but there may be an API that could be opened up to users. For example, instead of asyncio.new_event_loop().__class__ it could use a "constant" pytest_asyncio.BASE_EVENT_LOOP_CLASS that could be altered at test-time. Although, users could also change the policy in the test module to change this in a similar (possibly better) way.

I also considered trying to make a mark that uses arguments like @pytest.mark.asyncio_clock.with_args(base_class=ProactorEventLoop), but the way the markers are used in pytest_asyncio made this difficult for me to do without completely refactoring that interface. This could be done, but I would recommend leaving such work to a separate pull request. This may not be necessary since users may be better off changing the event loop policy in their base test module anyway.

Lastly, I am again left wondering, should clock_event_loop be the default? As far as I can tell, the behavior should be identical with the exception of added functionality and a very minor performance reduction in the time() method. Perhaps there is a better (less likely overriden) private variable name other than _offset?

[derekbrokeit]

I found the source the remaining errors to be in pytest_fixture_setup and related setup functions. The main problem is that they are written to be highly specific to the event_loop fixture name and need to be refactored to allow for a variety of event_loop fixture types. I could try to refactor this setup to correct the behavior, but changing default event_loop to clock_event_loop would similarly correct the behavior. Which is the preferred path?

[derekbrokeit]

I am closing this in favor of #113