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

Comments

Projects
None yet
5 participants
@csaftoiu

csaftoiu commented Mar 18, 2016

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

This comment has been minimized.

Member

nicoddemus commented Mar 18, 2016

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

This comment has been minimized.

Member

The-Compiler commented Mar 18, 2016

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

This comment has been minimized.

Member

nicoddemus commented Mar 18, 2016

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

This comment has been minimized.

Member

RonnyPfannschmidt commented Mar 22, 2016

we need a proper deprecation warning system

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Mar 22, 2016

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.

@chrish42

This comment has been minimized.

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

This comment has been minimized.

Member

nicoddemus commented Jun 16, 2016

Resolved in #1586

@nicoddemus nicoddemus closed this Jun 16, 2016

@csaftoiu

This comment has been minimized.

csaftoiu commented Jun 16, 2016

I do enjoy open source software :).

On Thursday, June 16, 2016, Bruno Oliveira notifications@github.com 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