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

Hook wrapping without _Result #389

Merged
merged 1 commit into from
Jun 17, 2023
Merged

Conversation

bluetech
Copy link
Member

@bluetech bluetech commented Jun 11, 2023

This implements the idea from #260. I think it makes pluggy more elegant, and also provides a backward compatible fix for #244 (replaces #257).

@bluetech
Copy link
Member Author

In #260, @maxnikulin argued against using a "plain" generator function for this, preferring a new @hookimpl(wrapper=True) instead, to make it explicit.

My opinion is that implicit is fine, because that's how python functions themselves work, and how e.g. pytest.fixture works, and I think they're mostly fine. But I'm willing to be outvoted on this.

Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks nicer than what we have
It Will make async a little Harder, but reasonably so

@bluetech
Copy link
Member Author

It Will make async a little Harder, but reasonably so

Seems like async generators have the necessary asend/athrow, the only thing they're missing is return support. I wonder why they don't support that.

@bluetech
Copy link
Member Author

bluetech commented Jun 12, 2023

Thanks for the reviews @RonnyPfannschmidt.

I plan to:

  • Extract the force_exception change to its own PR with own changelog.
  • Try to port pytest to detect any issues and measure performance in a realistic setting.
  • Wait a week or two to allow for discussion.

@bluetech
Copy link
Member Author

Here's a branch converting all of pytest to new-style wrappers: https://github.com/bluetech/pytest/commits/new-style-wrappers

All of the tests pass, you can use it to get an impression of usage. I haven't compared performance yet.

@bluetech
Copy link
Member Author

I've made the following performance measurements:

  1. Pluggy benchmark - old pluggy/new pluggy - neutral
  2. Pytest benchmarks - old pytetst+old pluggy/new pytest+new pluggy - neutral
  3. Pytest benchmarks - old pytetst+old pluggy/old pytest+new pluggy - neutral

I also made an experiment where I removed hookwrapper=True support entirely from _multicall (removing _Result etc.) and this gets a ~15% improvement in pluggy benchmark. But we don't have to drop old-style wrappers to get this improvement; it is easy to add a fast path which specializes for only new-style case - see here (better to view ignoring whitespace). Will maybe submit this as a separate PR if this one gets merged.

@RonnyPfannschmidt
Copy link
Member

How expensive would it be to have legacy hook wrappers be implemented in terms off The new wrappers thus making them slower but allowing the new wrappers also Bugfix the exception escape

@bluetech
Copy link
Member Author

I'm not sure, but I generally think it'd be better to keep the existing hookwrapper=True semantics as-is, to avoid breaking things (https://xkcd.com/1172/ situation). Switching to the new style would also be opting in to the new semantics.

@nicoddemus
Copy link
Member

nicoddemus commented Jun 13, 2023

Great work @bluetech!

The idea seems great, I will take a look at the code in the next few days! 👍

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks @bluetech!

"""Wrap calls to ``setup_project()`` implementations which
should return json encoded config options.
"""
# get initial default config
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency, the legacy example in Old-style wrappers could also have the exact same beginning as here: defaults = config.tojson(), and then printing the debug info via if config.debug.

(I would suggest that directly below, but I cannot leave a suggestion outside the diff)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops I forgot to handle this -- fixing now.

Fix pytest-dev#260.

Co-authored-by: Maxim Nikulin <manikulin@gmail.com>
@bluetech
Copy link
Member Author

Thanks for the reviews!

After taking another look at #257, I've made a few changes:

  • Added the teardown.close() bit -- it's good to have. Added it only for the new style so as not to touch old style.
  • Ported the tests from there.
  • Added @maxnikulin as co-author

@bluetech bluetech merged commit 40fbab1 into pytest-dev:main Jun 17, 2023
@bluetech bluetech deleted the un-result branch June 17, 2023 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants