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

add advance_time fixture and test (closes #83, #95, #96 #110) #113

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

derekbrokeit
Copy link

This is reworking of the advance_time method from #96 and #95 into an event-loop independent fixture. This removes the need to change the event-loop class or policy on the fly. Compared to the fixture discussed in #83, this technique does not require any private method calls to the event-loop, which potentially volatile depending on the loop implementation. One thing to keep in mind, this technique requires patching the time method of the event loop, but the original base time method is stored to reduce the assumptions required about the timing methodology, which is left up to loop creators. This should be a fairly safe implementation.

* master:
  Simplified fixture setup by relying on the fact that get_event_loop returns the loop provided by the event_loop fixture of pytest-asyncio.
  Removed a layer of indirection when checking for markers. There is only one marker and one fixture provided by pytest-asyncio. Retrieving these from a dictionary seems unnecessary complicated.
  Add test for hypothesis event_loop reuse.
  Sync wrapper for hypothesis tests uses the event loop provided by pytest-asyncio instead of creating a new loop.
  0.11 open for business.
  Actually bump version.
  Changelog.
  Make transfer_markers a noop when importing fails
  Revert "Remove transfer_markers"
  Remove transfer_markers
  More specific Hypothesis detection
  Support async tests which use Hypothesis
  Move pytest warning config to setup.cfg
  Fix: Avoid warning on latest Pytest versions
* advance_time-fixture:
  add more tests to verify the advance_time fixture operations
  simplify EventLoopClockAdvancer and clarify __call__ method
  Simplified fixture setup by relying on the fact that get_event_loop returns the loop provided by the event_loop fixture of pytest-asyncio.
  Removed a layer of indirection when checking for markers. There is only one marker and one fixture provided by pytest-asyncio. Retrieving these from a dictionary seems unnecessary complicated.
  Add test for hypothesis event_loop reuse.
  Sync wrapper for hypothesis tests uses the event loop provided by pytest-asyncio instead of creating a new loop.
  foo
@derekbrokeit
Copy link
Author

derekbrokeit commented May 14, 2019

I went through and added some additional tests. Also, I think the __call__ method is more clear now.

@bryanforbes
Copy link

I just tried this in a Python 3.7 project I'm working on with the following code:

async def race(
    futures: Iterable[FutureT[T]],
    *,
    timeout: Optional[float] = None,
    loop: Optional[asyncio.AbstractEventLoop] = None,
) -> T:
    if loop is None:
        loop = asyncio.get_running_loop()

    tasks = {future for future in futures}

    done, pending = await asyncio.wait(
        tasks, timeout=timeout, return_when=asyncio.FIRST_COMPLETED, loop=loop
    )

    try:
        if len(pending) == len(tasks):
            raise asyncio.TimeoutError()

        return done.pop().result()
    finally:
        for future in pending:
            future.cancel()

My test for this function looks like this:

@pytest.mark.asyncio
async def test_race(event_loop, advance_time):
    async def one():
        await asyncio.sleep(100, loop=event_loop)
        return 1

    async def two():
        await asyncio.sleep(50, loop=event_loop)
        return 2

    async def three():
        await asyncio.sleep(25, loop=event_loop)
        return 3

    task = event_loop.create_task(race([one(), two(), three()], loop=event_loop))
    await advance_time(35)
    await advance_time(60)
    await advance_time(110)
    assert task.result() == 3

I copied the implementation in this PR into my conftest.py, but it didn't work until I changed the asyncio.sleep() calls in EventLoopClockAdvancer.__call__ to something greater than 0:

    async def __call__(self, seconds):
        """
        Advance time by a given offset in seconds. Returns an awaitable
        that will complete after all tasks scheduled for after advancement
        of time are proceeding.
        """
        # sleep so that the loop does everything currently waiting
        await asyncio.sleep(0.0001)

        if seconds > 0:
            # advance the clock by the given offset
            self.offset += seconds

            # Once the clock is adjusted, new tasks may have just been
            # scheduled for running in the next pass through the event loop
            await asyncio.sleep(0.0001)

@derekbrokeit
Copy link
Author

derekbrokeit commented May 23, 2019

Thanks for testing, @bryanforbes . What OS and loop policy are you loop using? I recently found that using uvloop causes this to fail for some reason, and I suspect the ProactorEventLoop (on windows only) may also fail. Thanks in advance for any info you can provide.

@derekbrokeit
Copy link
Author

derekbrokeit commented May 23, 2019

I should mention for future reference, there is nothing (to my knowledge) we can do to fix this on uvloop. The problem with uvloop is that it does not use the public time() method for scheduling, it uses a private method Loop._time() as seen here. Unfortunately, the private method is a c-level api that cannot be modified by pytest-asyncio python code, and it is best to leave it alone.

@derekbrokeit
Copy link
Author

derekbrokeit commented May 23, 2019

@bryanforbes , you motivated me to add something I had been considering. I set the default to 1e-6 because it has shown to be most effective on more systems that I tested.

- for more simplicity the advnace-time-sleep has simply been changed to
    1e-6 so that it works on most systems.
- remove debug code
@bryanforbes
Copy link

What OS and loop policy are you loop using?

I'm on macOS 10.14.1 using the standard event loop (as far as I'm aware). I updated the code in my conftest.py with 1e-6 and got failures in other tests. Everything worked fine with 1e-4.

@RemiCardona
Copy link

Hi folks, I'm thrilled to report that this fixture works as intended with my work codebase. Until a new release is cut out with this PR, I've vendored the fixture. This will allow us to progressively migrate our large body of tests written using asynctest (especially code using asynctest.ClockedTestCase), which has served us well, with neater pytest-style tests.

It's worth mentioning here (for the record) that ClockedTestCase works a little bit differently: time is set in stone and never "progresses". Whereas advance_time does not stop the time from progressing, it just allows doing controlled skips. It's too early to tell whether this subtle change will be a hindrance to our porting effort, but it is a significant difference between the two.

In any case, let me know if there's anything I can do to help making sure this PR lands. Cheers.

@RemiCardona
Copy link

Hi folks, is there anything I can do to help get the ball rolling on this PR? We've been using it successfully for a few months now, it's rock solid and works as advertised. Thanks

@RemiCardona
Copy link

Hi folks, I've been revisiting old TODOs at work and this issue popped up. I was thinking, to help merging this PR, maybe an "experimental" disclaimer/tag in the doc could be added to this fixture so that neither its API nor its behavior is set in stone. Could this help pave the way forward?

Maybe I should create a pytest-asyncio-advance-time package containing this fixture? (that is, until it is eventually merged, I really don't want to get into the business of maintaining yet-another tiny package) I'd really like to avoid that as pytest-asyncio really seems like the natural place for this fixture.

Any input from the maintainers would be greatly appreciated, thanks!

@asvetlov
Copy link
Contributor

asvetlov commented Jan 3, 2022

BTW, I have a loop proxy implementation that allows the wallclock time tweaking without monkey-patching of the original event loop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants