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

fixtures: refactor FixtureRequest/SubRequest a bit #11218

Closed
bluetech opened this issue Jul 16, 2023 · 2 comments · Fixed by #11219
Closed

fixtures: refactor FixtureRequest/SubRequest a bit #11218

bluetech opened this issue Jul 16, 2023 · 2 comments · Fixed by #11219
Labels
topic: fixtures anything involving fixtures directly or indirectly type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature

Comments

@bluetech
Copy link
Member

The FixtureRequest/SubRequest situation is currently quite confusing.

  • FixtureRequest (on its own, not its subclass SubRequest) is used for the request fixture in a test function itself. One is created even if the request fixture is not explicitly requested. The name is a bit confusing -- it is not a request for a fixture. I think it wanted to be named RequestFixture i.e. the request fixture, but not 100% sure.

    A test function itself cannot be parametrized using request.param, only a fixture can.

    For FixtureRequest the request.scope is always function and the request.node is the item.

    In the pytest internals, it is used to drive getting the fixture values of the fixtures needed by the item (_fillfixtures).

  • SubRequest is used for request inside a fixture. The SubRequest holds a reference to the FixtureDef it handles.

    For parametrized fixtures it holds the parameter value (request.param).

    In the pytest internals, it is used to drive the execution of a specific "fixture request" .

    While executing an item, the SubRequests and the top FixtureRequest form a chain/stack. For example, if we have test_it requesting fix1 requesting fix2, then there is SubRequest(fix2) which points to SubRequest(fix1) which points to FixtureRequest(test_it). This is only used in practice for printing a "fixture stack" in some errors, but is useful to understand conceptually.

SubRequest inherits from FixtureRequest, but in a pretty hard to understand way, e.g. its __init__ doesn't call the super.

As an initial way to clarify things a bit, I propose making FixtureRequest itself abstract, and add a new TopRequest subclass for the test-function request.

The name TopRequest would not be my TopChoice, but renaming FixtureRequest or SubRequest at this point would be a disruptive breaking change, so better to choose a name which makes sense in relation to SubRequest.

This will break plugins which instantiate FixtureRequest directly. It's private so not an official breaking change. I found pytest-alembic, pytest-yield, pytest-wdl, pytest-play. I can notify them about this change if this proposal is accepted.

@bluetech bluetech added type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature topic: fixtures anything involving fixtures directly or indirectly labels Jul 16, 2023
bluetech added a commit to bluetech/pytest that referenced this issue Jul 16, 2023
@RonnyPfannschmidt
Copy link
Member

Id like to propose introducing new names and leaving deprecated aliases for fixture request/sub request

Then we can use names to express the detail

@nicoddemus
Copy link
Member

nicoddemus commented Jul 17, 2023

Thanks for the detailed proposal @bluetech.

I agree the whole FixtureRequest/SubRequest situation is really confusing, and renaming them is a step in the right direction.

As per @RonnyPfannschmidt's suggestion, if we keep around some aliases, there's no harm in renaming either of them. I like TopRequest, it is easy to explain its meaning when in conjunction with SubRequest.

As an initial way to clarify things a bit, I propose making FixtureRequest itself abstract, and add a new TopRequest subclass for the test-function request.

And SubRequest would no longer subclass FixtureRequest/TopRequest, just implement the abstract class?

EDIT: Just saw you opened a PR already, will move the discussion over there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: fixtures anything involving fixtures directly or indirectly type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants