Implemented the dynamic scope feature. #5776

aklajnert · 2019-08-21T14:45:47Z

This is an attempt to implement the dynamic scope feature described in #1682.

codecov · 2019-08-21T15:10:03Z

Codecov Report

Merging #5776 into features will decrease coverage by 0.07%.
The diff coverage is 93.54%.

@@             Coverage Diff              @@
##           features    #5776      +/-   ##
============================================
- Coverage     96.33%   96.26%   -0.08%     
============================================
  Files           117      117              
  Lines         25995    26064      +69     
  Branches       2498     2506       +8     
============================================
+ Hits          25042    25090      +48     
- Misses          650      669      +19     
- Partials        303      305       +2

Impacted Files	Coverage Δ
src/_pytest/deprecated.py	`100% <100%> (ø)`	⬆️
testing/python/fixtures.py	`98.9% <100%> (-1.1%)`	⬇️
src/_pytest/fixtures.py	`97.1% <90.47%> (-0.41%)`	⬇️
testing/code/test_code.py	`95.76% <0%> (-4.24%)`	⬇️
src/_pytest/pastebin.py	`87.71% <0%> (-3.51%)`	⬇️
src/_pytest/python_api.py	`97.35% <0%> (-0.87%)`	⬇️
src/_pytest/nodes.py	`95.73% <0%> (-0.46%)`	⬇️
testing/test_assertion.py	`97.56% <0%> (-0.02%)`	⬇️
src/_pytest/compat.py	`98.54% <0%> (-0.02%)`	⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5bf9f9a...e3608f0. Read the comment docs.

src/_pytest/fixtures.py

nicoddemus

Hi @aklajnert, thanks a lot for the contribution!

The contribution overall looks great, and besides my comments, I think we are also missing a few things:

linting can be fixed by running tox -e linting or installing pre-commit (see docs).
We need some docs about this feature, possibly in doc/en/fixture.rst.

We need two changelog entries, I suggest:

1682.deprecation.rst:

Passing ``scope`` as a positional parameter to ``@pytest.fixture`` has been deprecated, pass it by keyword instead.

1682.feature.rst:

The ``scope`` parameter of ``@pytest.fixture`` can now be a callable that receives the ``config`` object
as keyword-only parameter. See `the docs <LINK>`__ for more information.

src/_pytest/fixtures.py

nicoddemus

Thanks for the feedback. About the missing things in the PR, I am aware of them, but I just wanted to show my code first, to make sure that my approach is correct.

Great!

For example, if the user wants to control scope via an environment variable, he doesn't need the config at all. The dynamic scope function could be defined as def func(_): ..., which will be better for pylint than unused argument.

The user can ignore keyword arguments using def func(**_): in the same way.

My intent with this is that we might want to pass new arguments in the future, and using keyword arguments would allow us to call the method even if the user doesn't declare all arguments, similar to how pluggy does with hook implementations.

aklajnert · 2019-08-22T17:15:09Z

@nicoddemus I think the code is finished, however, I'm not sure about a few things:

The linting fails on azure pipelines but works fine to me locally. Flake8 complains about files that I didn't modify. Am I missing something?
Am I supposed to add tests also for the exceptions in the _eval_scope_callable?
Not sure if the example in the documentation is good. Any ideas for a better one?

doc/en/fixture.rst

src/_pytest/fixtures.py

src/_pytest/deprecated.py

doc/en/fixture.rst

src/_pytest/fixtures.py

pablochacin · 2019-09-05T10:44:37Z

Hi @aklajnert I have also an implementation of this feature (using hooks, and with some limitations) but I've found that without invocation scoped fixtures it's very hard to make it work reliably unless you mark all the fixtures as dynamically scoped depending on the same configuration option, which is not always convenient.

aklajnert · 2019-09-05T13:14:45Z

Hi @aklajnert I have also an implementation of this feature (using hooks, and with some limitations) but I've found that without invocation scoped fixtures it's very hard to make it work reliably unless you mark all the fixtures as dynamically scoped depending on the same configuration option, which is not always convenient.

To be honest, I don't understand the idea of the invocation scoped fixtures, and I don't see how it affects this feature. Can you explain it to me?

pablochacin · 2019-09-06T10:28:13Z

To be honest, I don't understand the idea of the invocation scoped fixtures, and I don't see how it affects this feature. Can you explain it to me?

The problem I have is that if I define a fixture b with a dynamic scope like this:

@pytest.fixture
def a():

@pytest.fixture(scope=my_dynamic_scope)
def b(a):

I need fixture a to be either defined with the same dynamic scope than b or be defined with a fixed session scope to be sure it will work regardless of the scope of b (a fixture cannot depend of a lesser scoped fixture).

If I understand it correctly, invocation scoped fixtures allows a fixture to have multiple scopes depending on the scope on which is it used. If a session scoped fixture uses it, it becomes session scoped, but if a function scoped fixture or function uses it, a new instance of the fixture with the function scope is created.

In that way a invocation scoped fixture would solve the issue I described before with dynamically scoped fixtures.

aklajnert · 2019-09-06T11:11:25Z

Thanks, @pablochacin I understand now. This indeed seems tricky.
I'm not sure how it works exactly in the internals, but the dynamic scope callable is meant to be evaluated only once during the fixture declaration. After it is resolved, the fixture scope value is a string so you should be able to reach that value.

pablochacin · 2019-09-06T11:28:53Z

Thanks, @pablochacin I understand now. This indeed seems tricky.
I'm not sure how it works exactly in the internals, but the dynamic scope callable is meant to be evaluated only once during the fixture declaration. After it is resolved, the fixture scope value is a string so you should be able to reach that value.

I implemented this same mechanism and started having the problem with mixed scopes. I think in a large project, with many fixtures, it is not viable to implement dynamic scope without invocation scope.

aklajnert · 2019-09-06T11:45:32Z

I think in a large project, with many fixtures, it is not viable to implement dynamic scope without invocation scope.

I don't know pytest internals very well, but in my understanding, if you use the dynamic scope after the fixture is defined, it will be exactly the same as you would define the scope in a traditional way. For example, the callable below when used as dynamic scope is equivalent to statically set session.

def dynamic_scope(**_):
    return "session"

Therefore I can't see why the invocation scoped fixtures are necessary here.

pablochacin · 2019-09-06T14:21:25Z

Therefore I can't see why the invocation scoped fixtures are necessary here.

Because in the example I gave above, if fixture b dynamic scope is set to "session", you get an error unless fixture a has also scope session . Therefore fixture a must also have dynamic scope depending on the same callable than fixture b.

@pytest.fixture
def a(scope=my_dynamic_scope):

@pytest.fixture(scope=my_dynamic_scope)
def b(a):

But consider now that fixture a is used by fixture c which has a function scope, or even a dynamic scope which is different than the one set to fixture b.

@pytest.fixture(scope=my_other_dynamic_scope)
def c(a):

Unless I'm missing something here, this won't work unless my_dynamic_scope and my_other_dynamic_scope both set the scope of fixtures a, b and c to the same value (e.g. session)

aklajnert · 2019-09-06T17:53:21Z

I guess you're right, but I still think it isn't connected with the dynamic scope feature.
From the example below, fixture_a and fixture_b will end up with exactly the same scope, since the dynamic_scope() function will be resolved to the "session" string.

def dynamic_scope(**_):
    return "session"

@pytest.fixture(scope=dynamic_scope)
def fixture_a():
    pass

@pytest.fixture(scope="session")
def fixture_b():
    pass

I have no experience with nested fixtures with different scopes, but I think the dynamic scope feature shouldn't change the current behavior.

pablochacin · 2019-09-09T09:10:52Z

Maybe @nicoddemus or @RonnyPfannschmidt can shed some light on this topic

RonnyPfannschmidt · 2019-09-09T09:15:52Z

from my pov invocation scope and dynamic scope are orthogonal in terms of implementation
however invocation/usage based scopes would make dynamic scope more useful

right now to use dynamic scopes sensible in a larger setup, one needs to use it on all involved fixtures

with better scoping in general one would only need it in key fixtures, and the dependent ones would follow suit as needed (this one needs some trying out and experimentation in the future)

as far as i can tell the implementation as given is reasonable and a enhancement

also its open to future expansion/refinement in a way that seems ok to follow on

pablochacin · 2019-09-09T13:19:23Z

from my pov invocation scope and dynamic scope are orthogonal in terms of implementation
as far as i can tell the implementation as given is reasonable and a enhancement

Absolutly so @RonnyPfannschmidt

It was not my intention to point this proposal as incorrect or not useful. I only wanted to bring attention about the issues users will very likely find (as I did) Sorry @aklajnert if it sounded as a criticism to your proposal.

That said, any chance to make dynamic scope work any time soon, @RonnyPfannschmidt? I was looking at the implementation that was finally reverted and I must confess it is beyond my understanding of pytest's internals at this point.

RonnyPfannschmidt · 2019-09-09T13:36:43Z

That one will take a while, the current internals make invocation scopes and multi scopes very tricky and brittle

We will probably need a sprint and a few refactorings to get it to work

aklajnert · 2019-09-10T04:38:24Z

@RonnyPfannschmidt do you mean that this PR won't be merged anytime soon? I'm a bit confused as it seems to be stuck in limbo, and I don't have any feedback on the changes. I know the tests are failing but they seem to be unrelated.

RonnyPfannschmidt

no it actually should push forward soon

src/_pytest/fixtures.py

nicoddemus

Looks great @aklajnert, thanks a lot for all the hard work and sorry for the delay! 👍

…dynamic-scope

nicoddemus · 2019-09-18T10:51:57Z

I fixed the conflict and took the liberty of changing the tests to plain tests.

Thanks again for the hard work!

RonnyPfannschmidt

lovely add-on detail work

testing/python/fixtures.py

nicoddemus · 2019-09-19T11:22:57Z

Thanks again @aklajnert!

pytest 5.2.0 added support for callable scopes. In order to distinguish those (`@pytest.fixture(scope=fn)`) from decorators (`@pytest.fixture`), it added extra arguments which was not expected by our wrapper. See pytest-dev/pytest#5776 for the change. Fixes the following error: ImportError while loading conftest 'test/conftest.py'. test/conftest.py:42: in <module> from fixtures_ws import * test/fixtures_ws.py:198: in <module> @fixtures.fixture test/fixtures.py:36: in fixture return pytest.fixture(scope, params, autouse, ids, name) E TypeError: 'bool' object is not iterable We do not use non-keyword arguments, so it is safe to use `*` instead of `*args` in the prototype. Change-Id: I96220e0e85249ad58880e5de75f8987a0fdc16ef

pytest 5.2.0 added support for callable scopes. In order to distinguish those (`@pytest.fixture(scope=fn)`) from decorators (`@pytest.fixture`), it added extra arguments which was not expected by our wrapper. See pytest-dev/pytest#5776 for the change. Fixes the following error: ImportError while loading conftest 'test/conftest.py'. test/conftest.py:42: in <module> from fixtures_ws import * test/fixtures_ws.py:198: in <module> @fixtures.fixture test/fixtures.py:36: in fixture return pytest.fixture(scope, params, autouse, ids, name) E TypeError: 'bool' object is not iterable We do not use non-keyword arguments, so it is safe to use `*` instead of `*args` in the prototype. Change-Id: I96220e0e85249ad58880e5de75f8987a0fdc16ef Reviewed-on: https://code.wireshark.org/review/34672 Petri-Dish: Peter Wu <peter@lekensteyn.nl> Tested-by: Petri Dish Buildbot Reviewed-by: Peter Wu <peter@lekensteyn.nl>

pytest 5.2.0 added support for callable scopes. In order to distinguish those (`@pytest.fixture(scope=fn)`) from decorators (`@pytest.fixture`), it added extra arguments which was not expected by our wrapper. See pytest-dev/pytest#5776 for the change. Fixes the following error: ImportError while loading conftest 'test/conftest.py'. test/conftest.py:42: in <module> from fixtures_ws import * test/fixtures_ws.py:198: in <module> @fixtures.fixture test/fixtures.py:36: in fixture return pytest.fixture(scope, params, autouse, ids, name) E TypeError: 'bool' object is not iterable We do not use non-keyword arguments, so it is safe to use `*` instead of `*args` in the prototype. Change-Id: I96220e0e85249ad58880e5de75f8987a0fdc16ef Reviewed-on: https://code.wireshark.org/review/34672 Petri-Dish: Peter Wu <peter@lekensteyn.nl> Tested-by: Petri Dish Buildbot Reviewed-by: Peter Wu <peter@lekensteyn.nl> (cherry picked from commit ba35c23) Reviewed-on: https://code.wireshark.org/review/34685

RonnyPfannschmidt reviewed Aug 21, 2019

View reviewed changes

src/_pytest/fixtures.py Outdated Show resolved Hide resolved

nicoddemus requested changes Aug 21, 2019

View reviewed changes

src/_pytest/fixtures.py Outdated Show resolved Hide resolved

nicoddemus reviewed Aug 22, 2019

View reviewed changes

bluetech reviewed Aug 22, 2019

View reviewed changes

nicoddemus mentioned this pull request Aug 23, 2019

Docker containers scope pytest-docker-compose/pytest-docker-compose#3

Closed

nicoddemus reviewed Aug 23, 2019

View reviewed changes

doc/en/fixture.rst Outdated Show resolved Hide resolved

src/_pytest/fixtures.py Outdated Show resolved Hide resolved

Implemented the dynamic scope feature.

10bf6aa

aklajnert changed the title ~~WIP: Implemented the dynamic scope feature.~~ Implemented the dynamic scope feature. Aug 31, 2019

aklajnert requested a review from nicoddemus August 31, 2019 16:44

RonnyPfannschmidt approved these changes Sep 10, 2019

View reviewed changes

aklajnert commented Sep 10, 2019

View reviewed changes

src/_pytest/fixtures.py Outdated Show resolved Hide resolved

Fixed the fixture function signature.

f2f3ced

nicoddemus approved these changes Sep 18, 2019

View reviewed changes

nicoddemus added 2 commits September 18, 2019 07:44

Merge remote-tracking branch 'upstream/features' into aklajnert/1682-…

6918d07

…dynamic-scope

Change fixture argument handling tests to unit-tests

df46afc

RonnyPfannschmidt approved these changes Sep 18, 2019

View reviewed changes

aklajnert commented Sep 18, 2019

View reviewed changes

testing/python/fixtures.py Outdated Show resolved Hide resolved

Minor cleanup in tests.

e2382e9

nicoddemus merged commit 9669413 into pytest-dev:features Sep 19, 2019

aklajnert deleted the 1682-dynamic-scope branch September 19, 2019 11:27

francesco-ballarin mentioned this pull request Dec 17, 2019

Pass fixture scope as a keyword argument vtitor/pytest-gc#2

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented the dynamic scope feature. #5776

Implemented the dynamic scope feature. #5776

aklajnert commented Aug 21, 2019

codecov bot commented Aug 21, 2019 •

edited

nicoddemus left a comment •

edited

nicoddemus left a comment •

edited

aklajnert commented Aug 22, 2019

pablochacin commented Sep 5, 2019

aklajnert commented Sep 5, 2019

pablochacin commented Sep 6, 2019 •

edited

aklajnert commented Sep 6, 2019

pablochacin commented Sep 6, 2019

aklajnert commented Sep 6, 2019

pablochacin commented Sep 6, 2019

aklajnert commented Sep 6, 2019

pablochacin commented Sep 9, 2019

RonnyPfannschmidt commented Sep 9, 2019

pablochacin commented Sep 9, 2019

RonnyPfannschmidt commented Sep 9, 2019

aklajnert commented Sep 10, 2019

RonnyPfannschmidt left a comment

nicoddemus left a comment

nicoddemus commented Sep 18, 2019

RonnyPfannschmidt left a comment

nicoddemus commented Sep 19, 2019

Implemented the dynamic scope feature. #5776

Implemented the dynamic scope feature. #5776

Conversation

aklajnert commented Aug 21, 2019

codecov bot commented Aug 21, 2019 • edited

Codecov Report

nicoddemus left a comment • edited

Choose a reason for hiding this comment

nicoddemus left a comment • edited

Choose a reason for hiding this comment

aklajnert commented Aug 22, 2019

pablochacin commented Sep 5, 2019

aklajnert commented Sep 5, 2019

pablochacin commented Sep 6, 2019 • edited

aklajnert commented Sep 6, 2019

pablochacin commented Sep 6, 2019

aklajnert commented Sep 6, 2019

pablochacin commented Sep 6, 2019

aklajnert commented Sep 6, 2019

pablochacin commented Sep 9, 2019

RonnyPfannschmidt commented Sep 9, 2019

pablochacin commented Sep 9, 2019

RonnyPfannschmidt commented Sep 9, 2019

aklajnert commented Sep 10, 2019

RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

nicoddemus left a comment

Choose a reason for hiding this comment

nicoddemus commented Sep 18, 2019

RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

nicoddemus commented Sep 19, 2019

codecov bot commented Aug 21, 2019 •

edited

nicoddemus left a comment •

edited

nicoddemus left a comment •

edited

pablochacin commented Sep 6, 2019 •

edited