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

Expose FixtureDef and SubRequest #9510

Closed
wants to merge 2 commits into from
Closed

Expose FixtureDef and SubRequest #9510

wants to merge 2 commits into from

Conversation

asvetlov
Copy link
Contributor

@asvetlov asvetlov commented Jan 14, 2022

Export names required by pytest_fixture_setup and pytest_fixture_post_finalizer hook specs.

@asvetlov asvetlov marked this pull request as draft January 14, 2022 13:10
@asvetlov asvetlov marked this pull request as ready for review January 14, 2022 13:36
@RonnyPfannschmidt
Copy link
Member

@bluetech im wondering, would there be a simple way to type the Sub-request with a FixtureParameter type as well

the idea bein that one coud use a type for the fixture param instead of any

@asvetlov
Copy link
Contributor Author

asvetlov commented Jan 15, 2022

@RonnyPfannschmidt FixtureParameter doesn't exist on the current main branch.
I assume you mean FixtureValue.

Now SubRequest is initialized with fixturedef=FixtureDef[object].
Sure, we can derive SubRequest from Generic[FixtureValue] and change contructor's argument to fixturedef=FixtureDef[FixtureValue].

If you mean something like this -- I can prepare a separate pull request.

The PR for exposing new public names is orthogonal to type system improvement and can be approved/merged independently.

Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

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

Hi @asvetlov,

Regarding SubRequest, please see #8763 (comment). The situation hasn't changed since then unfortunately, it needs work.

Regarding FixtureDef, it needs a bit of an audit before exposing it. From looking at it now:

  • The docstring "A container for a factory definition." is a bit obscure, we should improve it (but that's preexisting, not blocking).

  • Need to decide if we want the constructor to be public or private. If it is public and exposed in pytest, that means pytest is committing to keeping it stable/backward compatible. Also, in this case the ctor parameters need to be documented.

    The conservative approach would be to make it private for now. Our mechanism for doing it is described here. But before doing so we should check if there are many plugins which construct FixtureDef. (I have a large pytest plugins corpus which I use to check for such things, but I'm not on that machine ATM).

  • Should document the non-underscored attributes, if they should be public, or make them private, if not. Need to audit them...

  • Audit the methods:

    • scope -> Good
    • addfinalizer -> Good, should add a docstring
    • finish and execute -> Not sure about these. They also need SubRequest to be public. Need to see if/how the fixtures are meant to be using these.
    • cache_key -> I think it's probably better making it private. Might need a deprecation.

@@ -0,0 +1,9 @@
Fixture types are now exported so the may be used in pytest plugin hooks.
Copy link
Member

Choose a reason for hiding this comment

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

Let's make the changelog refer to issue #7469 instead of the PR.

@bluetech
Copy link
Member

@RonnyPfannschmidt FixtureParameter doesn't exist on the current main branch. I assume you mean FixtureValue.

There are two things:

First, FixtureValue, also present on FixtureRequest, which is the type of the return value of the fixture.

@RonnyPfannschmidt is talking about request.param, which is the value of the parameter in a parametrized fixture. It is present only on SubRequest and is currently not typed. (Additional complication is that it's not always set - only for parametrized fixtures).

@RonnyPfannschmidt
Copy link
Member

Hence the idea of the adding a generic for the case of having a parameter and a normal type when not

@bluetech
Copy link
Member

FixtureDef is exposed now with making the ctor private and minimal exposing of internals (fb55615).

@bluetech
Copy link
Member

I'll close this since just exposing SubRequest as-is is not going to be good.

@bluetech bluetech closed this Mar 17, 2024
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.

None yet

3 participants