-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Fixtures without name matching magic #3834
Comments
Hi @shoyer, If you don't like the way fixtures are automatically injected, you can be more explicit in a way very similar to the example you provided: import pytest
@pytest.fixture
def smtp_connection():
import smtplib
return smtplib.SMTP("smtp.gmail.com", 587, timeout=5)
@pytest.mark.usefixtures("stmp_connection")
def test_ehlo(smtp_connection):
response, msg = connection.ehlo()
assert response == 250
assert 0 # for demo purposes |
@nicoddemus that's explicit about using a fixture, but it's still using a string to refer to a name variable. Is it possible to do something like that while referencing |
Probably you can do something like this: import pytest
def use_fixture(fixfunc):
def inner(f):
return pytest.mark.usefixtures(fixfunc.__name__)(f)
return inner
@pytest.fixture
def fix():
return 1
@use_fixture(fix)
def test(fix):
assert fix == 1 But you still need to use the same name in the argument as the fixture, plus you will need to explicitly import the fixture function into the test module in case it is not defined there, and that is not really recommended because it messes with the internal caching of the fixtures.
What is the problem with that in your opinion? That's very explicit IMHO:
Not sure what is the reason for that, can you explain? |
Indeed, but also using
I want to use Python's normal rules for resolving symbols (i.e., standard variable lookup), and I want to keep introspection to the bare minimum needed to run tests. I know pytest uses magic to rewrite Referencing functions by string name instead of object breaks normal code introspection tools (e.g., lint), which can't recognize misspelled names if there's no variable reference. So if I write (In my opinion, one of the best things about Python is that dependencies are explicit, unless you use the discouraged
As one simple example, if you run this example through
At Google, we actually strictly enforce running pylint on all Python code. If I wanted to disable this message, I would need to write a comment Now, pylint is known for being overly-aggressive in some cases, but I do think that in general it's best not mask variable names from outer scopes. There's basically no reason why this is ever necessary if not using pytest, and if done unintentionally, this can easily lead to bugs. Another example of what doing this explicitly could look like, with keyword arguments: @pytest.mark.usefixtures(conn=smtp_connection)
def test_ehlo(conn):
... Using a fixture imported from another module also becomes very straightforward, e.g., @pytest.mark.usefixtures(conn=other_module.smtp_connection)
def test_ehlo(conn):
... |
Hi @shoyer sorry for taking so long to get back at this. I'm not sure it has a place in the core though, as it would lead to a different way of doing things, and is solving not really an existing problem but a personal preference which is shared by some people but not by the majority of pytest users (at least that's what I think, no hard data here). But all you are proposing is probably possible to write as a plugin. I suggest you give this a try (be sure to post here if you need help) and see how that feels in real code. What do you think? |
personally i have been thinking about such a mechanism as well, i just dont have the time to work on even a base for it |
Let me add to this that the name-matching does lead to real-world problems: I just spent an hour trying to get my pytest-sanic tests to working, not realising that it's |
This would be really nice. I hate dropping The original suggestion by @shoyer looks good enough, imo. Anyway, here are some extra thoughts: # not supported by python(?):
def test_ehlo(@pytest.inject(smtp_connection) connection):
...
# dunno if this would be doable, either:
@pytest.fixture
class SmtpConnection:
def __call__(self):
import smtplib
return smtplib.SMTP("smtp.gmail.com", 587, timeout=5)
@pytest.inject
def test_ehlo(conn: SmtpConnection):
... |
Just to mention that fixtures have a @pytest.fixture(name='smtp_connection')
def smtp_connection_():
...
def test_ehlo(smtp_connection):
... |
Oh, that's a workaround I could use. Good to know. Though the benefit in being able to do something like what I suggested above would be that when you (have to) import fixtures, they are then actually referenced in the tests file, and your import will not get marked as unused (and removed if you auto-organize imports). Sure I could: from foo import my_fixture
PYTEST_IMPORTS = my_fixture But then |
I'll register my support for this issue as well. When users first see tests written in pytest, it is mystifying how the arguments to these functions get populated. At least having the option to be explicit about it in a robust way would be nice. Another possible benefit is the ability to give a different name to the same fixture in different tests. Or maybe more specifically, the ability to give the same name to different fixtures in different tests. Not sure if this is already possible, but with an explicit mechanism like the one @shoyer is talking about, it could look something like this: Let's say we have these fixtures for an application that processes log files (this is a barebones example just to illustrate the point):
With the current behavior, we could write tests like this:
In the proposed behavior, it could look like this:
The explicit approach allows the generic aspects of the test code to remain generic -- in this example, the test only cares about reading a logfile and asserting that it works. The fact that we have selected this file to be an empty one to test an edge case is already captured in the name of the function, and avoiding the need for different variable names seems desirable since the code is the same. In fact, doing it this way gives us yet another advantage -- the possibility of parametrizing over fixtures using
... which covers both tests in a single parameterized test. It looks like there is currently already a way to parametrize fixtures themselves. That's a great feature as well, but I think the ability to parameterize the test rather than the fixture is a different thing which could complement the current functionality and lend additional flexibility. |
FYI there is already a way to do this in a nice-ish way: import pytest
@pytest.fixture
def simple_logfile():
return 'simple_logfile contents'
@pytest.fixture
def empty_logfile():
return 'empty_logfile contents'
@pytest.fixture(name='logfile')
def dynamic_fixture(request):
return request.getfixturevalue(request.param)
@pytest.mark.parametrize('logfile', ['simple_logfile', 'empty_logfile'], indirect=True)
def test_something(logfile):
print()
print('test_something', repr(logfile)) Downside being that you can't do indirect parameterization of I can still see the benefit in this proposal, though: if |
Just wanted to drop my support for this request as well. The current fixtures functionality acts as a global registry of magical variable names and globals are confusing and error prone for the reasons already outlined a few times above. When I'm creating new fixtures I should not need to try and think of a globally unique name for it as long as I've implemented it in my globally unique package name. It's a little disappointing to try and convince folks on the team to write maintainable/testable code and then run into the testing framework itself not following software engineering best practices. We only need to write this code once but we have to read it, understand it, and maintain it for years. We can afford to type a few extra lines of code to make sure it works and is immediately clear to anyone that might run into it in the future. |
Hi @ms-lolo,
I sympathize with that sentiment, but personally I think the issue boils down more to introducing another way to use fixtures, which might lead to even more confusion when the classic way and this new way proposed here are mixed. As was stated before, I believe it is possible to implement a different mechanism as a plugin, but adding this directly to the core without first seeing it being used in the wild might not be the best approach. |
If we agree that an explicit API is better, we might consider deprecating the current global name matching entirely, and eventually only supporting the new way (or at least suggesting it as the preferred option for new code). |
Totally understand wanting to avoid having multiple ways of doing this. And using a plugin to demonstrate a cleaner approach sounds like a great idea that I'm going to try and find some time to investigate, although I have no experience with the pytest internals, so any guidance would be appreciated!
I was mostly leaving a comment to add a data point about this comment :) I disagree with the statement that this is a personal preference but we don't need to go down that rabbit hole without even a proof of concept alternative to compare to. |
@jadkik Nice, though another downside is that, judging by your example, we'd need to write a wrapping dynamic fixture for every set of fixtures we might want to parametrize over. And since we would be creating this abstraction behind yet another global name, this does not escape the namespace issues that @ms-lolo brings up. By separating the fixture definition from its name, we gain the ability to abstract over any set of fixtures behind a lexically-scoped name in each test, without having to pre-designate (via a wrapper function like |
FWIW maybe we could turn to ward for inspiration here. It uses default argument values to make fixtures more explicit (and namespaced) without having much more boilerplate: from ward import fixture, test
@fixture
def user():
return User(id=1, name="sam")
@test("fetch_user_by_id should return the expected User object")
def _(expected_user=user):
fetched_user = fetch_user_by_id(id=expected_user.id)
assert fetched_user == expected_user I don't particularly like the It's still "name matching magic", but at least a bit more explicit and, most importantly, namespaced. Of course the million dollar question is how to implement this with the already quite complex fixture code, especially in a way that's backwards compatible. So far I haven't had the time (or mind-space) to see if it'd even be realistic at all. |
anything in the domain of adoption/modifying the mechanisms in which fixture lookup works is something for after we de-cruft and de-tangle it from past experience its pretty much doomed before we simplify/expliticify it my take is that we can't hope to do better until we make it reasonably sane to choose the dependency injection system and to integrate a external one with pytest fixtures as a base personally i really want to enable better lookup, but that comes after de-tangleing what grew due to the history that started with the |
now that pytest is Python3.5+ only, using an Annotations based DI is possible: https://fastapi.tiangolo.com/tutorial/dependencies/ |
That's an idea, and probably the way to go for certain features, but not for fixture IMO: it would break every pytest suite out there which uses fixtures. This kind of breakage would kill the project IMO.
It depends on the mechanism actually... there are some proposals in this thread, but importing and using the actual names in the test function definition, bypassing the current global lookup, might not be possible through a simple plugin, I'm afraid.
Indeed, but it seems this would take many hours of someone dedicated to this task to tackle.
Indeed, and that's similar to what My take overall:
So this is a very complicated issue. 😓 |
Yes I'm sorry if I confused you. My understanding is that the fixture decorator makes it into a "proper" fixture and all the third party pytest plugins use such fixtures. I was hoping for a workaround for pytests name based matching which I worry will confuse people on my team. Maybe I misunderstood what you were trying to say in that comment. Is there a way to do make a decorator similar to what you wrote possibly with getfixturevalue or something else that works on fixture decorated functions or am I just totally misunderstanding how the fixtures work and that isn't currently possible without a lot of effort? |
This seems to work as a useful workaround def make_test_with_fixtures_from_defaults(fn):
def default_func_names(func):
# HT: https://stackoverflow.com/a/12627202/259130
signature = inspect.signature(func)
return [
v.default.__name__
for k, v in signature.parameters.items()
if v.default is not inspect.Parameter.empty
]
fixture_names = default_func_names(fn)
z = textwrap.dedent(f'''\
def test_func({', '.join(fixture_names)}):
return fn({', '.join(fixture_names)})
''')
scope = dict(fn=fn, fixture_names=fixture_names)
exec(z, scope)
test_func = scope['test_func']
test_func.__name__ = fn.__name__
return test_func seems to work. Assuming you don't call the function by keyword argument. Does that every happen? Or are there any other corner cases or plugins this might have trouble with? from _pytest.tmpdir import tmp_path
#...
@make_test_with_fixtures_from_defaults
def test_create_file(tmp=tmp_path):
d = tmp / "sub"
d.mkdir()
p = d / "hello.txt"
p.write_text('lol')
assert p.read_text() == 'lol'
assert len(list(tmp.iterdir())) == 1 works as expected and passes(so do other examples I tried) I know a lot of people don't like exec but it works. I could probably do something to set the signature with https://smarie.github.io/python-makefun/ instead but I think makefun might also use exec. Maybe https://github.com/dfee/forge? |
Interesting approach. If you turn that into a package, I'm sure others would be interested in trying it out. |
I think this version of the workaround should support both default argument and decorator provided fixtures like ward does def make_test_with_fixtures(**fixture_kwargs):
def inner(fn):
signature = inspect.signature(fn)
keyword_names_to_fixtures = {
k: fixture_kwargs.get(k, None) or v.default
for k, v in signature.parameters.items()
}
assert all(v is not inspect.Parameter.empty
for v in keyword_names_to_fixtures.values()), (
'every parameter should have a matching fixture function '
'provided in either the decorator or default function')
keyword_names_to_fixture_names = {k: f.__name__ for (k, f) in keyword_names_to_fixtures.items()}
fixture_names = keyword_names_to_fixture_names.values()
z = textwrap.dedent(f'''\
def test_func({', '.join(fixture_names)}):
return fn({', '.join(kname+'='+fname for (kname, fname) in keyword_names_to_fixture_names.items())})
''')
scope = dict(fn=fn)
exec(z, scope)
test_func = scope['test_func']
test_func.__name__ = fn.__name__
return test_func
return inner I tested it out and both
test_bar1 and test_bar2 seem to work. I'll try to put it in a package when I have time. Maybe this weekend I want to see it it possible avoid exec first(possibly via the python-forge package). |
Note that is an implementation detail, which you can change at will later. 👍 |
It's pretty barebones but people might find it useful to reference fixtures by reference. https://github.com/rtaycher/pytest-fixture-ref pip install git+https://github.com/rtaycher/pytest-fixture-ref.git from pytest_fixture_ref import make_test_with_fixtures
@make_test_with_fixtures()
def test_bar1(f1=fix_w_yield1, f2=fix_w_yield2, tmp=tmp_path):
print("test_bar")
print(f'{tmp}')
assert tmp.exists()
@make_test_with_fixtures(f1=fix_w_yield1, f2=fix_w_yield2, tmp=tmp_path)
def test_bar2(f1, f2, tmp):
print("test_bar")
print(f'{tmp}')
assert tmp.exists() |
@nicoddemus is there a common way to import pytest plugins? |
There is no standard for that, each plugin decides which API to expose; so in your case Btw, if you have Twitter, feel free to tweet about that project and I'll be glad to retweet (also we can ask the pytest-dev official twitter to retweet too). |
@rtaycher can I suggest splitting up the two versions of fixtures into their own decorators? I'm opposed to the version that inspects the functions' default values and I think it would be useful to have that decision be clearer when we write our tests. A lot of development on large code bases happens by copying nearby code and having them as separate decorators makes it clearer that, in this code base, we chose style A over style B. |
@ms-lolo Could you open an issue with some suggested names? I tend to bikeshed maybe @fixtures_from_default_value and @fixtures_from_ref? Both sound not quite right to me |
@nicoddemus https://twitter.com/rtaycher1987/status/1593199919892156418 sorry it took so long. Figuring out the CI/publish setup for the boilerplate I used took a while(hit a number of odd library issues, plus the autochangelog is really annoying) It's now on pypi so people can just use |
Nice, thanks @rtaycher! I also recommend that suggestions/requests to be made in https://github.com/rtaycher/pytest-fixture-ref/issues instead of in this thread, as that is the appropriate place. 👍 |
A couple of notes:
|
@adriangb your proposal sounded very promising |
I got bogged down in internal implementations of pytest and never got to finishing it. It mostly worked, and worked well, but there was bugs I just couldn't fix. I don't think I'll have time to do it myself. |
can you provide a open draft pr for reeference - it may enable us to put focus on the bugs to enable the rest |
I think this was my WIP branch: https://github.com/adriangb/di/tree/pytest. But given that it was last touched 3 years ago I don't know the state of it. To be clear I'm not saying there were bugs in pytest, more so that I just didn't understand how things were supposed to work internally, especially when it came to the async plugins. |
I have thrown together a quick plugin that allows fixtures to be resolved using e.g.
you can find it here: https://github.com/tristan/pytest-annotated and here: https://pypi.org/project/pytest-annotated/ Unfortunately, I feel it requires the use of too many |
that one is a funky little hack, we ought too figure if we can enable lookup by name, type and annotated type in future |
https://pypi.org/project/pytest-unmagic/ is a pytest plugin that implements fixtures with normal import semantics. Fixtures are applied to tests or other fixtures with a |
@millerdev at first glance pytest-unmagic is a complete deception - using magic globals unrelated to the actual fixtures instead of actual values passed - i strongly discourage anyone from using it - it does the opposite of its name |
@RonnyPfannschmidt Fixtures are global objects. Pytest registers them in an opaque hierarchical namespace, and effectively does a global lookup each time a fixture is requested. The magic that this plugin eliminates is function-scoped local names magically invoking global fixtures. Granted, the active request reference is not the nicest feature. Hopefully we can work together to improve it? The active request reference allows I have a development branch where the global active request reference has been removed, but it has trade-offs. The implementation is more complicated, and it is impossible to use fixtures in the context of For now—in the very spirit of pytest fixtures—practicality beats purity. |
This explanation seems to discard important distinction between concepts like registries, discovery, references,actions at a distance and some more As far as I'm concerned it's much worse than what we have right now I'm happy to work towards more explicit control of the registries as well as sane lookup But injection of dependencies at a distance instead of parameters is absolutely off the table, that's completely unacceptable |
My 2 cents on the plugin -- I agree with @RonnyPfannschmidt overall about the design, however I'm also happy that @millerdev did an experiment to try things out (it is his project after all!). But regardless I wanted to share my opinions on some points (I'm not speaking for all maintainers or for the pytest project at all, but only how I see this topic):
Footnotes
|
@nicoddemus: Clarke's third law states "any sufficiently advanced technology is indistinguishable from magic". That is not at all a bad thing. It just means that as soon as a mechanism is not obvious how it works, it appears as if it were magic. Once you understand how things work, its not magic anymore, but just a great power to wield. And with great power, comes great responsibility. IMHO, what is wrong with the current fixture mechanism is the single global namespace shared among all participants in a given test session. Thus, this particular great power is hard to guide to its intended target. Rather than a surgical laser beam, these fixtures behave like a virus; once you unleash them, they go everywhere, hitting both intended and unintended targets. It appears magic to the many users, because once that virus arrives at your site (intended or not), you don't see where it comes from. This applies both to humans and to tools (type checkers). More importantly, the design makes it impossible to reason about the area of effect within any subscope, because you always need to know the full testing session before you can start figuring out where fixtures may come from or go to. This is very different from the magic methods in dataclasses or the various forms of automatic schema validation and column creation in ORMs. Its true that it can appear equally magic to someone who doesn't understand the mechanism, but once you do understand, a) the rules are simple (as in the current pytest fixtures) and b) the amount of inputs you need to consider to apply them are reasonably bounded (unlike in the current pytest fixtures). |
Thanks @burnpanck for the detailed reasoning, you make good points. Regardless, all my previous points still stand: backward compatibility, more than one way to do the same thing, etc, and those do not change regardless of the discussion about what constitutes a magical behavior or not. But again, this is all from my POV, I don't know if other core maintainers feel otherwise and think it is OK to add an additional way to use fixtures to core pytest. |
Yay! 🎉 This would be wonderful.
This is precisely why I think pytest-unmagic is an improvement on pytest magic fixture parameters. "Unmagic" fixtures are always explicitly referenced, and do not occupy a single shared global namespace. The question "where is the code for that?" can be answered without any knowledge of pytest fixture name resolution. The part that @RonnyPfannschmidt reacted so strongly to (I think) is the active request reference, which is an implementation detail that allows the plugin to bind fixture setup and teardown to the appropriate scope and to retrieve the value from pytest's internal fixture cache. It is easy to reason about the active request within the context of any given test or fixture, so that single global reference does not seem as problematic as a shared global namespace for all fixtures. Maybe "unmagic" is a bit of a misnomer. The plugin moves the "magic" part from fixture name resolution, which everyone using fixtures is confronted with, to an implementation detail where most people don't need to know much if anything about it. You might need to know if you're running tests in parallel within a single Python interpreter, but that is not a problem I tried to solve in v1.0. |
I've been happily using pytest for several projects for the past few years.
There's one part about pytest that I still struggle to get behind: The way that fixtures magically match argument names to fixtures -- and apparently I'm not alone in this feeling. I would much rather declare dependencies explicitly in some way using code. I know this would be more verbose, but that's a tradeoff I'm happy to make.
Is it possible to do this in some way with pytest today? If not, would you be open to adding an optional feature for this?
I was thinking perhaps something like the following, using an example adapted from the docs:
The text was updated successfully, but these errors were encountered: