Make normal fixtures work with "yield" #1586

nicoddemus · 2016-06-03T00:09:28Z

Did a quick experiment to see how hard would it be to implement #1461 to suggest working on it during the sprint... astonished to find out it was way too easy. 😲

Just ignore the yieldctx parameter and decide on the spot if we are a "yield fixture" based on whether or not the function is a generator does the trick nicely.

Almost all of the changes are related only to change the tests of yield_fixture to also test with plain fixture to prove this is working.

Missing:

Clean up the code (remove the need to pass yieldctx around);
Update docs and CHANGELOG;

Opening the PR to discuss if we should move this forward or not. @RonnyPfannschmidt, @hpk42, @The-Compiler what you guys think?

coveralls · 2016-06-03T00:16:47Z

Coverage decreased (-0.02%) to 92.407% when pulling a0bbad6 on nicoddemus:issue-1461-merge-yield-fixture into b5bd4d9 on pytest-dev:features.

nicoddemus · 2016-06-03T00:42:52Z

(quick note: AppVeyor is failing because of the "doctesting" env, which is already fixed in master)

RonnyPfannschmidt · 2016-06-03T06:22:12Z

Looks neat, surprisingly simple, just wondering if this is to be considered a breaking api change art semver

The-Compiler · 2016-06-03T06:52:17Z

_pytest/python.py

@@ -2326,6 +2326,7 @@ def fail_fixturefunc(fixturefunc, msg):
                pytrace=False)

 def call_fixture_func(fixturefunc, request, kwargs, yieldctx):
+    yieldctx = is_generator(fixturefunc)
    if yieldctx:
        if not is_generator(fixturefunc):
            fail_fixturefunc(fixturefunc,


So I know this is a WIP, just adding this so we don't accidentally forget: Those lines aren't needed anymore now 😉

EDIT: Or should be changed to work again, as I pointed out in #1586 (comment)

The-Compiler · 2016-06-03T06:56:34Z

Awesome! 👍

@RonnyPfannschmidt What would be the breaking change? I can only imagine two things:

A yield_fixture without a yield would now work instead of error out. We can change that to the old behaviour.
A fixture with a yield would now work instead of erroring the test. I don't think that's a backwards-incompatible change, as hopefully nobody would keep tests around which fail in that way 😉

RonnyPfannschmidt · 2016-06-03T07:15:59Z

@The-Compiler true

nicoddemus · 2016-06-03T09:54:38Z

Nice, I will continue to work on the PR then (clean it up and docs, mostly).

About the documentation, I plan to consider @fixture + yield as the recommended way of writing teardown fixture code and update all examples to use @fixture and yield where appropriate (those which don't have tear down code will be kept using return). So yield_fixture will be a footnote in the docs mentioned only as a historical fact. How do you guys feel about that?

A yield_fixture without a yield would now work instead of error out. We can change that to the old behaviour.

I see. We just have to consider that to keep this behavior we need to keep yieldctx around... I was hoping to remove that as this will cleanup the code quite a bit. Do you feel we really need this behavior?

The-Compiler · 2016-06-03T11:18:43Z

About the documentation, I plan to consider @fixture + yield as the recommended way of writing teardown fixture code and update all examples to use @fixture and yield where appropriate (those which don't have tear down code will be kept using return). So yield_fixture will be a footnote in the docs mentioned only as a historical fact. How do you guys feel about that?

Sounds great to me!

I see. We just have to consider that to keep this behavior we need to keep yieldctx around... I was hoping to remove that as this will cleanup the code quite a bit. Do you feel we really need this behavior?

Hmm, that's a good point. On a second thought I guess we can do without it, as hopefully people will get away from using yield_fixture anyways.

nicoddemus · 2016-06-08T00:28:43Z

Cleaned up the code and updated docs! 😁

coveralls · 2016-06-08T00:35:22Z

Coverage decreased (-0.02%) to 92.405% when pulling a91d035 on nicoddemus:issue-1461-merge-yield-fixture into b5bd4d9 on pytest-dev:features.

nicoddemus · 2016-06-08T00:44:54Z

CONTRIBUTING.rst

@@ -211,7 +211,7 @@ but here is a simple overview:
   You need to have Python 2.7 and 3.5 available in your system.  Now
   running tests is as simple as issuing this command::

-    $ python runtox.py -e linting,py27,py35
+    $ python3 runtox.py -e linting,py27,py35


This change is already on master, cherry-picked here to see green builds. 😁

coveralls · 2016-06-08T00:51:13Z

Coverage increased (+0.004%) to 92.433% when pulling e2040f4 on nicoddemus:issue-1461-merge-yield-fixture into b5bd4d9 on pytest-dev:features.

coveralls · 2016-06-09T00:27:15Z

Coverage increased (+0.004%) to 92.433% when pulling bdc2996 on nicoddemus:issue-1461-merge-yield-fixture into 9232389 on pytest-dev:features.

nicoddemus · 2016-06-14T00:33:09Z

Guys, would you like to merge this now or would you rather wait until the sprint?

The-Compiler · 2016-06-16T07:04:28Z

I'm a bit late to the party, but LGTM, and thanks for the great work! I think this definitely simplifies things a lot. 👍

In pytest-dev/pytest#1586 the "yieldctx" argument to FixtureDef was removed. This uses utils.get_args to check if it's needed or not so pytest-bdd works on pytest versions before and after that PR.

nicoddemus changed the title ~~Experiment to make normal fixtures work with "yield"~~ [WIP] Experiment to make normal fixtures work with "yield" Jun 3, 2016

The-Compiler reviewed Jun 3, 2016
View reviewed changes

nicoddemus mentioned this pull request Jun 7, 2016

Use multiple yield statements as an alternative for parametrization #1595

Closed

nicoddemus force-pushed the issue-1461-merge-yield-fixture branch from a0bbad6 to a91d035 Compare June 8, 2016 00:21

nicoddemus changed the title ~~[WIP] Experiment to make normal fixtures work with "yield"~~ Make normal fixtures work with "yield" Jun 8, 2016

nicoddemus reviewed Jun 8, 2016
View reviewed changes

nicoddemus added 7 commits June 8, 2016 21:07

Make normal fixtures work with "yield"

366879d

Fix custom name for yield_fixtures

d712428

Remove yieldctx variable being passed around

98acda4

Update docs in prol of using yield statements

fe4f23c

Update CHANGELOG (yield statements in normal fixtures)

371fbe4

Remove most content from yieldfixture as it is now deprecated

ed69424

Remove dead code and simplify code in call_fixture_func

bdc2996

nicoddemus force-pushed the issue-1461-merge-yield-fixture branch from e2040f4 to bdc2996 Compare June 9, 2016 00:08

RonnyPfannschmidt merged commit feeee28 into pytest-dev:features Jun 14, 2016

nicoddemus deleted the issue-1461-merge-yield-fixture branch June 16, 2016 10:58

nicoddemus mentioned this pull request Jun 16, 2016

Suggestion: merge .fixture and .yield_fixture #1461

Closed

The-Compiler mentioned this pull request Jun 17, 2016

Fix FixtureDef signature for newer pytest versions pytest-dev/pytest-bdd#183

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make normal fixtures work with "yield" #1586

Make normal fixtures work with "yield" #1586

nicoddemus commented Jun 3, 2016

coveralls commented Jun 3, 2016

nicoddemus commented Jun 3, 2016

RonnyPfannschmidt commented Jun 3, 2016

The-Compiler Jun 3, 2016 •

edited

Loading

The-Compiler commented Jun 3, 2016

RonnyPfannschmidt commented Jun 3, 2016

nicoddemus commented Jun 3, 2016

The-Compiler commented Jun 3, 2016 •

edited

Loading

nicoddemus commented Jun 8, 2016

coveralls commented Jun 8, 2016

nicoddemus Jun 8, 2016

coveralls commented Jun 8, 2016

coveralls commented Jun 9, 2016

nicoddemus commented Jun 14, 2016

The-Compiler commented Jun 16, 2016

Make normal fixtures work with "yield" #1586

Make normal fixtures work with "yield" #1586

Conversation

nicoddemus commented Jun 3, 2016

coveralls commented Jun 3, 2016

nicoddemus commented Jun 3, 2016

RonnyPfannschmidt commented Jun 3, 2016

The-Compiler Jun 3, 2016 • edited Loading

Choose a reason for hiding this comment

The-Compiler commented Jun 3, 2016

RonnyPfannschmidt commented Jun 3, 2016

nicoddemus commented Jun 3, 2016

The-Compiler commented Jun 3, 2016 • edited Loading

nicoddemus commented Jun 8, 2016

coveralls commented Jun 8, 2016

nicoddemus Jun 8, 2016

Choose a reason for hiding this comment

coveralls commented Jun 8, 2016

coveralls commented Jun 9, 2016

nicoddemus commented Jun 14, 2016

The-Compiler commented Jun 16, 2016

The-Compiler Jun 3, 2016 •

edited

Loading

The-Compiler commented Jun 3, 2016 •

edited

Loading