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

Suggestion: merge .fixture and .yield_fixture #1461

Closed
csaftoiu opened this issue Mar 18, 2016 · 8 comments
Closed

Suggestion: merge .fixture and .yield_fixture #1461

csaftoiu opened this issue Mar 18, 2016 · 8 comments
Labels
type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature

Comments

@csaftoiu
Copy link

The yield_fixture page says:

lastly yield introduces more than one way to write fixture functions, so what’s the obvious way to a newcomer?

My suggestion is to merge pytest.fixture and pytest.yield_fixture.

Before:

@pytest.fixture
def foo():
    return ['bar']

@pytest.yield_fixture
def boof():
    with some_stuff as more_stuff:
        yield even_more_stuff(more_stuff)

After:

@pytest.fixture
def foo():
    return ['bar']

@pytest.fixture
def boof():
    with some_stuff as more_stuff:
        yield even_more_stuff(more_stuff)

This should be possible - if calling the fixture function returns a generator, then do what yield_fixture currently does, otherwise (i.e. if it returns a value), then do what fixture currently does.

The reason? There will be one less choice for a newcomer to make - which decorator to call.


Further, the documentation section "Fixture finalization / executing teardown code" can now read something like this:

pytest supports execution of fixture specific finalization code when the fixture goes out of scope. By yielding the fixture object instead of returning it, you can perform finalization code after the test has finished executing:

# content of conftest.py

import smtplib
import pytest

@pytest.fixture(scope="module")
def smtp(request):
    smtp = smtplib.SMTP("smtp.gmail.com")
    yield smtp  # provide the fixture value
    print("teardown smtp")
    smtp.close()

The code after the yieldwill execute when the last test using the fixture in the module has finished execution.

This is a straightforward change, syntactically (return -> yield), even if the internals are quite different, and is intuitive enough to me, and I suspect anyone who gets how generators work.

I suggest deprecating the old callback-teardown method since the yield mechanism is superior in every way. Really it should be the only way, but it may be confusing to someone who expects to be able to simply return a value, and so returning a value will just work as well.

@nicoddemus
Copy link
Member

Particularly I agree that yield_fixtures are superior and would like to see pytest.fixture support both styles.

I don't think there are backward compatibility issues, because we can use inspect.isgeneratorfunction(f) to decide if a fixture is using yield-style or return-style.

But I would like to hear what others have to say on this.

@The-Compiler
Copy link
Member

I've always thought the same, but I thought there was some obscure reason it was done the way it is rather than combining them since the beginning - seems like I'm wrong? 😉

I've also talked with @hpk42 about yield-fixtures vs. normal fixtures, in the context of the pytest trainings we're giving. He finds it more difficult to teach the yield_fixture approach, as people might not know yield or generators yet.

I personally think it's still easier to teach than request.addfinalizer, as people are also likely to not know the difference between calling a function and passing a function object (and of course it gets even more complex when parameters are involved), as I also saw in this stackoverflow question today.

If I were to decide, I'd integrate yield_fixture into fixture, promote that to the official/recommended way to do teardown from fixtures, and in the long run maybe deprecate request.addfinalizer and yield_fixture.

@nicoddemus
Copy link
Member

If I were to decide, I'd integrate yield_fixture into fixture, promote that to the official/recommended way to do teardown from fixtures

👍

in the long run maybe deprecate request.addfinalizer and yield_fixture.

Depends on what you mean exactly by deprecate, but I would prefer just discourage or note that request.addfinalizer is the old approach and just recommend using yield. 😉 (My main point here is that we wouldn't remove support for it in any future version because certainly there's a ton of code depending on it).

@RonnyPfannschmidt
Copy link
Member

we need a proper deprecation warning system

@nicoddemus
Copy link
Member

we need a proper deprecation warning system

I agree, but do you feel the need for a better warning system blocks deprecating yield_fixture as suggested here? To me they are separate concerns.

@nicoddemus nicoddemus added the type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature label Mar 29, 2016
@chrish42
Copy link

chrish42 commented Apr 7, 2016

As an experienced Python developer learning pytest, having the two merged together would have reduced what I had to learn with pytest.

Also, I'm pretty sure I'll forget (and need to lookup in the doc) the specific name request.addfinalizer, but I'll remember how to do cleanup by writing a generator (since it's similar to contextlib.contextmanager, etc. and feels like idiomatic Python to me).

@nicoddemus
Copy link
Member

Resolved in #1586

@csaftoiu
Copy link
Author

I do enjoy open source software :).

On Thursday, June 16, 2016, Bruno Oliveira [email protected] wrote:

Closed #1461 #1461.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#1461 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AACoaU59Mp9hg4UOywbFLsAPVKq2_T-Oks5qMSzMgaJpZM4H0EOU
.

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

No branches or pull requests

5 participants