Add the TestCase.with_timeout decorator.

[Kentzo]

Like #43 but less generic.

This is a backport from @GreatFruitOmsk's internal async_unittest written by @amezin.

[Kentzo]

@Martiusweb What do you think about the change?

[Kentzo]

@Martiusweb What's the min version of Python you'd like to support in asynctest?

[Martiusweb]

Hi, currently the target is python 3.4 until its EOL (2019-03-16), but this may change if the support of this version becomes a problem with future changes in the language.

[Kentzo]

@Martiusweb Perhaps the decorator should also support class as it's target (i.e. setting timeout for all coroutines).

The implementation might be changed to set an attribute on a function / class instead and then letting AsyncTestCase to runt with timeout.

[Kentzo]

@Martiusweb Any thoughts regarding that improvement?

[Martiusweb]

I'm not 100% convinced about a timeout feature.

A general-purpose timeout decorator around a coroutine is out of the scope of asynctest (imho), and if we want to tie this to TestCase, there are a couple of concerns:

Do we want a decorator for the whole class?
probably yes, as other decorators (@patch) do support this, but, some tests are plain functions...

A test function is not always a coroutine, should we support timeouts in this case?
probably not, as it's brittle to implement (using SIGALARM or something similar), and probably also out of the scope of asynctest. Should we raise an exception if used on a function? But we should not if it decorates a class... maybe emit a warning in this case?

Do we want a default timeout value?
While convenient for the author of the tests, I'm not convinced it's a good idea to provide this because it can be confusing. Instead I think something like this makes more sense:

• @TestCase.with_timeout(timeout_seconds) requires a value
• if a class/test is not decorated, by default there's no timeout for the tests
• the environment variable ASYNCTEST_TIMEOUT would allow to set a timeout value for all tests executed by the runner (being decorated or not). I believe this is a feature that can be interesting for CIs willing to ensure that tests won't run forever (a global timeout set at the CI level don't allow to retrieve which test(s) timed out)
• we can possibly add ASYNCTEST_MAX_TIMEOUT which caps the timeout value set by the decorator, but I'm not sure this is useful.

Implementation
rather than wrapping the coroutine in the decorator, I think this would be a better approach (and it allows to implement ASYNCTEST_TIMEOUT with the semantics proposed above):

@classmethod
def with_timeout(cls, timeout):
   def decorator(func):
       if not asyncio.iscoroutinefunction(func):
           warning("func is not a coroutine, timeout might be ignored")

        if timeout is None:
            cls._timeouts.pop(func.__name__, None)
        else:
            cls._timeouts[func.__name__] = timeout

        # do not wrap the test function, return it instead
        return func

    return decorator

the timeout would then be set when TestCase runs the test.

What do you think?

[Kentzo]

I agree with your comment regarding the general purpose timeout decorator. There is at least async_timeout (by aiohttp authors) for async code and timeout-decorator for sync code.

We can introduce an optional feature via setuptools's extra with necessary dependencies.

Do we want a decorator for the whole class?

Makes sense.

A test function is not always a coroutine, should we support timeouts in this case?

Not necessarily but I expect developers who use asyncio to find it convenient to restrict both async and sync functions (where latter are often implemented as blocking async functions internally).

Do we want a default timeout value?

I think that the most common usage of this feature will be to detect deadlocks within async code. Therefore I think it's convenient to have a sensible default value as well as to support the env variable to override that default.

Implementation that I propose:

1. Define an extra named timeout with deps async_timeout and timeout-decorator
2. Provide the @with_timeout decorator for methods that will have default value which can be configured via an env variable
3. The @with_timeout decorator should be able to detect whether decorated function is async and use the appropriate [1] implementation

[Martiusweb]

I really want to avoid dependencies if possible, especially when the library represents ~100-200 LOC. I think it's reasonable to only support asyncio timeouts and assume it's not our job to interrupt a non async test (this is more the job of the test runner).
We can try to forcibly stop the loop if the test is not a coroutine (with loop.call_later(timeout_value, loop.stop)) though.

I think that the most common usage of this feature will be to detect deadlocks within async code. Therefore I think it's convenient to have a sensible default value as well as to support the env variable to override that default.

I think my proposal is somewhat more convenient for this case: I'm not sure what would be a "sensible" default, and I don't think that an environment variable is the best way to override that default value: I prefer to use environment variable as a "global switch" that applies to everything that is run. That's why I proposed that the ASYNCTEST_TIMEOUT variable would apply to all (coroutine) tests, even if they're not decorated.

[Kentzo]

Perhaps it makes sense to skip env vars initially and wait until someone else will request them.