Skip to content

Add factory fixture example to documentation. #3522

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

Merged

Conversation

NiklasMM
Copy link
Contributor

Close #3461

Not sure if this is the best place to put this, but it seemed somewhat reasonable.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 92.753% when pulling 08de3da on NiklasMM:fix/3461_factory-fixture-doc into 5748c5c on pytest-dev:master.

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.

This looks great @NiklasMM, thanks a lot!

@nicoddemus nicoddemus merged commit cc793a8 into pytest-dev:master Jun 5, 2018
@andreyfedoseev
Copy link

@NiklasMM @nicoddemus

What are the benefits of using a fixture (as described in the docs) instead of a simple factory function?

This:

@pytest.fixture
def make_customer_record():

    def _make_customer_record(name):
        return {
            "name": name,
            "orders": []
        }

    return _make_customer_record

can be replaced with:

def make_customer_record(name):
    return {
        "name": name,
        "orders": []
    }

@nicoddemus
Copy link
Member

@andreyfedoseev I suggest opening a separate discussion with your question, rather than post your question to a merged PR -- it is OK to to refer to the PR in your post if the context helps.

@RonnyPfannschmidt
Copy link
Member

Perhaps the example should be extended with some context,the minimal example does not include interaction with storage and cleanup after test

This lack of extra context may mislead people into thinking the problem can be reduced to just the function

@RonnyPfannschmidt
Copy link
Member

Scrap that, the second example literally expands on that in a minimal fashion

@andreyfedoseev
Copy link

I ask specifically about the first trivial example because my fellow developers are seemingly replicating it, simply because it is a "pattern" described in pytest documentation.

The existing documentation says:

The "factory as fixture" pattern can help in situations where the result of a fixture is needed multiple times in a single test.

This is somewhat misleading, since a simple function would work just fine in such case, making the fixture unnecessary.

@nicoddemus
Copy link
Member

I agree in general that very simple fixtures, without arguments, are probably better started off as functions instead.

However fixtures shine when you need to introduce dependencies later, for example, if make_customer_record later in development needs an external resource, say a customers collection. If that's a fixture, you introduce the new parameter and the callers will not need to be updated because pytest will inject it automatically:

@pytest.fixture
def make_customer_record(customers):

    def _make_customer_record(name):
        customers.check_unused(name)
        return {
            "name": name,
            "orders": []
        }

    return _make_customer_record

Meanwhile if you used a simple function, the function itself needs to be updated, as well as the callers:

def make_customer_record(name, customers):
    customers.check_unused(name)
    return {
        "name": name,
        "orders": []
    }

That just to mention one of the advantages of fixtures over functions, but I would still recommend for simple fixtures (with minimal parameters) to be still start as functions, migrating to actual fixtures as needed.


As I mentioned, this should have been a separate discussion, so others could more easily find it later -- now this (interesting) discussion is kind of buried in a closed PR, which is a shame.

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.

5 participants