Add custom hooks specifications for overriding setup_timeout and teardown_timeout methods

[asvetlov]

Using bare pytest-timeout with pytest-asyncio works but timeout's traceback is not perfect.
It points at selector.select() call (epoll.poll() for linux case for example) instead of timed out await f(...) call.

pytest-asyncio has a pull request for providing better timeout support pytest-dev/pytest-asyncio#216 which generates awesome tracebacks but doesn't work well with a debugger.

I don't want to copy-paste all hard work that is done by pytest-timeout already but propose two hooks instead: pytest_timeout_setup(item) and pytest_timeout_teardown(item), both are marked as tryfirst=True.

Having these hooks, pytest-asyncio can provide its own implementation (which should use pluginmanager.get_plugin('timeout').is_debugging(), sure) to override default timeout methods but keep all other functionality untouched.

The pull request deliberately adds the minimum required changes only.
In the future, I can imagine a pluggable timeout_method choice set but it is not required for the bare minimum.

[flub]

Why not, I think this roughly looks good. I have two questions:

• This is kind of bikeshedding, but maybe the internal names of the functions are not good hook names, they describe where they are called rather than what they do. Maybe pytest_timeout_set_timer and pytest_timeout_cancel_timer are better hook names? Or perhaps you have even better ideas?

• Now that the plugin will gain a public API: the hooks and is_debugging. Perhaps you should set __all__ on the module? I'm not really sure how well this interacts with users vs pytest and whether pytest can still find hook implementations if they're not in __all__, you may need to experiment a bit

[asvetlov]

Long story short: I suggest change nothing except documenting added functionality in README (plus the PR itself, sure).

The long story begins.

I'm happy that you want to accept the plugin functionality extending.

Regarding pytest_timeout_set_timer / pytest_timeout_cancel_timer -- new names are probably better. That I really care is pytest_timeout_* prefix. I feel this is crucial.

The public API question is harder.
pytest uses pluggy for hooks. Pluggy is awesome, but it doesn't care about the public API by design. It doesn't care for __all__ also. Not good, not bad -- just a design choice.

The next step is more interesting.
Yuo can call config.pluginmanager.get_plugin('timeout') for getting the pytest-timeout plugin from pluggy. The call returns the whole pytest_timeout module, as it registered by setup.py.
If other pytest plugin registers name = pytest_name.plugin -- the full content of pytest_name.plugin is accessible. The content contains implementation details like many pytest_* function hooks just by design.

I really don't think that we can restrict available names to public API
pytest doesn't it neither even for internal plugins, e.g. capturemanager and _capturelog used by pytest-timeout explicitly. Capturemanager has a list of pytest_* hooks as a part of public API: https://github.com/pytest-dev/pytest/blob/main/src/_pytest/capture.py#L763-L799

I believe, new hook definitions should be described by REAME, that's it. @pytest.mark.tryfirst can handle it perfectly.

Regarding exposing is_debugging(). Surely, you are the decision maker.
I personally doubt if the function is wanted by the plugin casual users. Dependent plugin writers still have full access to all gory details.

import pytest_timeout

def func():
   ...
   pytest_timeout.is_debugging()

seems to my eye less natural than

def func(item):
   ...
   plugin = item.config.pliginmanager.getplugin('timeout')
   plugin.is_debugging()

Please feel free to ask me if the text is not clear. Maybe I've missed some important thing.

Thank you for the long reading :)

[flub]

Thanks for checking out the __all__ part, I was afraid pluggy's interaction with it wasn't going to be great.

On how to get hold of the plugin, I think there's a semantic difference between importing it and using the plugin manager to get a reference. Importing you simply get the module which happens to provide the plugin. Using the plugin manager you get the object instance which is being used by pytest.

So my understanding was that you wanted is_debugging to be public so that you can use it in the custom hook implementations. For this maybe it still makes sense to define an __all__ with is_debugging as public api and encourage people to import pytest_timeout directly if they want to use functions from it. Does this make sense?

So how about still doing this before merging:

• Rename hooks as suggested before (i agree on the pytest_timeout_ prefix btw)
• Define an __all__ for people directly importing pytest_timeout
• Update readme with how to use the hooks and the public API exposed via __all__.

[asvetlov]

@flub
I've fixed your notes.
Also added settings (type is Settings nametuple) argument to pytest_timeout_set_timer hook.
Custom hooks very likely need settings.timeout argument, other things like func_only and method don't make a hurt I believe. If you disagree, I can replace hook the settings hook argument with timeout and method, func_only is handled by plugin internals completely.

[flub]

thanks for the nice quality PR and followup!

[asvetlov]

Welcome and thanks for kind review!

[asvetlov]

Do you have an estimation for new release?

[flub]

On Sun 16 Jan 2022 at 10:59 -0800, Andrew Svetlov wrote: Do you have an estimation for new release?

done. there was some trouble with the readme (TIL about readme_renderer and `twine check`) so I originally pushed bad tags but hopefully everything is fine now, let me know if not.

[asvetlov]

Awesome, thanks!