pytest-dev / pytest

The pytest framework makes it easy to write small tests, yet scales to support complex functional testing
https://pytest.org
MIT License
11.85k stars 2.64k forks source link

Fixtures without name matching magic #3834

Open shoyer opened 6 years ago

shoyer commented 6 years ago

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:

import pytest

@pytest.fixture
def smtp_connection():
    import smtplib
    return smtplib.SMTP("smtp.gmail.com", 587, timeout=5)

@pytest.use_fixture(smtp_connection)
def test_ehlo(connection):
    response, msg = connection.ehlo()
    assert response == 250
    assert 0 # for demo purposes
nicoddemus commented 6 years ago

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
shoyer commented 6 years ago

@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 smtp_connection as a Python object -- preferably with a different, non-conflicting argument name?

nicoddemus commented 6 years ago

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.

that's explicit about using a fixture, but it's still using a string to refer to a name variable.

What is the problem with that in your opinion? That's very explicit IMHO: @pytest.mark.usefixtures("fix") explicitly declares that the test function uses a fixture "fix", and that is passed as argument name.

preferably with a different, non-conflicting argument name?

Not sure what is the reason for that, can you explain?

shoyer commented 6 years ago

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.

Indeed, but also using fixfunc.__name__ for lookup is a good example of exactly what I was trying to avoid here.

What is the problem with that in your opinion? That's very explicit IMHO: @pytest.mark.usefixtures("fix") explicitly declares that the test function uses a fixture "fix", and that is passed as argument name.

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 assert and decide which tests to run, but in general I like my Python code (even tests) to follow the normal rules of Python code.

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 stmp_connection instead of smtp_connection, nothing catches that mistake until I actually run my tests.

(In my opinion, one of the best things about Python is that dependencies are explicit, unless you use the discouraged import *.)

preferably with a different, non-conflicting argument name?

Not sure what is the reason for that, can you explain?

As one simple example, if you run this example through pylint, you get the following warning:

W0621: Redefining name 'smtp_connection' from outer scope (line 7) (redefined-outer-name)

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 # pylint: disable=redefined-outer-name.

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):
    ...
nicoddemus commented 6 years ago

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?

RonnyPfannschmidt commented 6 years ago

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

burnpanck commented 5 years ago

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 test_client fixture clashes with the test_client fixture of pytest-aiohttp.

tuukkamustonen commented 5 years ago

This would be really nice. I hate dropping # pylint: disable=redefined-outer-name lines to my test files because pylint will nag about every test with a fixture otherwise.

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):
    ...
nicoddemus commented 5 years ago

This would be really nice. I hate dropping # pylint: disable=redefined-outer-name lines to my test files because pylint will nag about every test with a fixture otherwise.

Just to mention that fixtures have a name parameter to avoid this problem:

@pytest.fixture(name='smtp_connection')
def smtp_connection_():
    ...

def test_ehlo(smtp_connection):
    ...
tuukkamustonen commented 5 years ago

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 PYTEST_IMPORTS will be marked as unused :P (or would importing even work together with name=?)

countvajhula commented 5 years ago

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):

@pytest.fixture
def simple_logfile():
    return ("2019-08-16 10:35:05 connection established\n"
             "2019-08-16 11:07:16 connection dropped\n")

@pytest.fixture
def empty_logfile():
    return ""

With the current behavior, we could write tests like this:

def test_read_logfile(simple_logfile):
    result = read_logfile(simple_logfile)
    assert result == True

def test_read_empty_logfile(empty_logfile):
    result = read_logfile(empty_logfile)
    assert result == True

In the proposed behavior, it could look like this:

@pytest.mark.usefixtures(logfile=simple_logfile)
def test_read_logfile(logfile):
    result = read_logfile(logfile)
    assert result == True

@pytest.mark.usefixtures(logfile=empty_logfile)
def test_read_empty_logfile(logfile):
    result = read_logfile(logfile)
    assert result == True

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 pytest.mark.parametrize. For instance, the proposed behavior could now look like this:

@pytest.mark.parametrize(
    "logfile", (simple_logfile, empty_logfile),
)
def test_read_logfile(logfile):
    result = read_logfile(logfile)
    assert result == True

... 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.

jadkik commented 4 years ago

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 simple_logfile from test_something, and it doesn't show up when you run pytest with --setup-plan (it does when you do --setup-only though.

I can still see the benefit in this proposal, though: if usefixtures accepts keyword arguments with names or actual fixture functions, it could be a change that doesn't break the current behaviour.

ms-lolo commented 4 years ago

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.

nicoddemus commented 4 years ago

Hi @ms-lolo,

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.

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.

shoyer commented 4 years ago

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.

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).

ms-lolo commented 4 years ago

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'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).

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.

countvajhula commented 4 years ago

@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 dynamic_fixture) which ones those are.

The-Compiler commented 4 years ago

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 @test decorator/naming thingy myself, but having namespaced fixtures which can be imported and used by specifying them as argument value seems pretty nice to me.

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.

RonnyPfannschmidt commented 4 years ago

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 pytest_funcarg__somename hooks that wouldn't cache

graingert commented 4 years ago

now that pytest is Python3.5+ only, using an Annotations based DI is possible: https://fastapi.tiangolo.com/tutorial/dependencies/

nicoddemus commented 4 years ago

@shoyer

If we agree that an explicit API is better, we might consider deprecating the current global name matching entirely

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.

@ms-lolo

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!

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.

@RonnyPfannschmidt

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

Indeed, but it seems this would take many hours of someone dedicated to this task to tackle.

now that pytest is Python3.5+ only, using an Annotations based DI is possible: fastapi.tiangolo.com/tutorial/dependencies

Indeed, and that's similar to what ward does, as @The-Compiler commented.


My take overall:

So this is a very complicated issue. 😓

graingert commented 4 years ago

Backward compatibility is a hard requirement

is backwards and forwards compatibility a hard requirement? Can tests using sans-name-magic based fixtures be wholly incompatible with name-magic based fixtures, specifically a test requiring Annotation based fixtures would not be able to consume name-based fixtures (except autouse?)?

nicoddemus commented 4 years ago

Can tests using sans-name-magic based fixtures be wholly incompatible with name-magic based fixtures, specifically a test requiring Annotation based fixtures would not be able to consume name-based fixtures (except autouse?)?

That's a good question. I suppose both systems need to co-exist and be interchangeable, so a test/fixture can request fixtures using name matching and/or explicit annotations, otherwise it wouldn't be very useful.

techdragon commented 4 years ago

@nicoddemus "co-existence" in that they are both functional options you can use as a developer, in the same version of pytest, sounds like a good thing to have for compatibility reasons...

But "interchangeable" actually sounds like a bad thing to me... it sounds like it would make building this more complicated than I feel it needs to be. I would be happy switching over to explicit fixtures if I had to use some special compatibility shim in my own codebase as a wrapper around magic fixtures. I kind of already have to, using things similar to the examples by yourself https://github.com/pytest-dev/pytest/issues/3834#issuecomment-428923623 and @jadkik https://github.com/pytest-dev/pytest/issues/3834#issuecomment-581358040 if I want to keep the level of magic in my tests to a minimum.

RonnyPfannschmidt commented 4 years ago

i believe we could have

@fixture(name=None, typed=True)
def something(request) -> MyClass
   return MyClass(name=request.fixturename)

def test_has_some(alice: MyClass, bob: MyClass):
   assert alice.name == "alice"
  assert bob.name == "bob"

in future, but the timeline is absolutely unclear

ms-lolo commented 4 years ago

i believe we could have

@fixture(name=None, typed=True)
def something(request) -> MyClass
   return MyClass(name=request.fixturename)

def test_has_some(alice: MyClass, bob: MyClass):
   assert alice.name == "alice"
  assert bob.name == "bob"

in future, but the timeline is absolutely unclear

I'm not quite following the example. How id test_has_some() defining where the two parameters come from?

graingert commented 4 years ago

@RonnyPfannschmidt I'm against looking at the args/kwarg names completely, annotated fixtures should use typing.Annotated pep 593 which have been reserved for runtime introspection:

class MyClass(pytest.Fixture):
    def __init__(self, name: str):
        self.name = name

def test_has_some(a: pytest.Param[MyClass, "alice"], b: pytest.Param[MyClass, "bob"]):
    assert a.name == "alice"
    assert b.name == "bob"

def test_query_some(v: MysqlDB | PostgresDB) -> None:
    assert v.execute(select(1)).scalar_one() == 1
RonnyPfannschmidt commented 4 years ago

@graingert i can understand the sentiment, but the ux looks painful at first glance

max-sixty commented 4 years ago

Could I make a suggestion that's very close to the original one by @shoyer ?

Two modes — explicit & implicit — default is for both to work. A pytest config is available to enforce explicit only (like how --strict-markers works now).

import pytest

@pytest.fixture
def connection():
    import smtplib
    return smtplib.SMTP("smtp.gmail.com", 587, timeout=5)

@pytest.use_fixture(connection=connection)  # <- not required in implicit mode, would achieve same result without it
def test_ehlo(connection):
    response, msg = connection.ehlo()
    assert response == 250
    assert 0 # for demo purposes

Note the annotated line can be elided in implicit mode because the name connection is used as both the fixture name and the parameter name.

In explicit mode, if we want to reduce the number of times connection is overloaded, we can use a different name for the function name and parameter name.

import pytest

@pytest.fixture
def smtp_connection():
    import smtplib
    return smtplib.SMTP("smtp.gmail.com", 587, timeout=5)

@pytest.use_fixture(connection=smtp_connection)  # <- required, would not work in implicit mode given the name difference
def test_ehlo(connection):
    response, msg = connection.ehlo()
    assert response == 250
    assert 0 # for demo purposes
ms-lolo commented 4 years ago

@max-sixty that's pretty much what I was hoping for. It's extremely clear and obvious and the function itself is not coupled to the implementation details of the testing framework. In theory, test_ehlo can be defined without the decorator and it would be a completely normal testing function that can be wrapped in pytest.use_fixture() later.

nicoddemus commented 4 years ago

@max-sixty

Implicit — the existing design Explicit — something like:

Not sure a config file which enables one behavior but disables the other is feasible. For example, some internal pytest features and many plugins depend on the implicit behavior. Aside from the internals (which could be work around), enabling "explicit" means that you can't use any of the existing plugins?

@graingert

@RonnyPfannschmidt I'm against looking at the args/kwarg names completely, annotated fixtures should use typing.Annotated pep 593 which have been reserved for runtime introspection

Not sure I follow what you mean here, but I read @RonnyPfannschmidt's example:

@fixture(name=None, typed=True)
def something(request) -> MyClass
    return MyClass(name=request.fixturename)

def test_has_some(alice: MyClass, bob: MyClass):
    assert alice.name == "alice"
    assert bob.name == "bob"

As meaning that for each type-annotated parameter, pytest should look at the type, and see in the current module namespace if any @pytest.fixture-decorated function (either defined locally or imported) has the return value annotated with that type; in that case, call the function and inject the value.

At first glance seems like it could work, but after some thought:

  1. This means you can have multiple values for the fixture in the same scope/call. Right now each fixture name has a single value at that scope (for example, you only have a tmpdir instance, regardless if the test function and or multiple fixtures request it), but this now is not so clear:

    @fixture(name=None, typed=True)
    def something(request) -> MyClass
       ...
    
    @fixture(name=None, typed=True)
    def other_fixture(alice: MyClass) -> Foo:
       ...  
    
    def test_has_some(alice: MyClass, other: Foo):
       ...

    In that case, how many instances of MyClass are created? The short answer would be 2 (one for other_fixture, another instance to test_has_some), but that's different on how normal fixtures work, which is confusing. Making they share the instance is also tricky, how to you define what ties them together? The attribute name? Seems brittle.

  2. Right now people are already using this notation for type-annotations (see pytest-mock which recently exported MockerFixture for this purpose, and also how we plan to proceed with builtin fixtures in pytest). Hijacking it at this point seems problematic to me.

  3. Using typing.Annotated seems the standard way to accomplish this, but I agree it feels clunky.


IMHO a proposal needs to think on the big picture, and how the new way to specify fixtures will interact with the classical way. I think it would be a mistake to make code bases choose one while disabling the other.

altendky commented 4 years ago

A type-based lookup doesn't seem great. We only get one int typed fixture? We have to create not-quite-int types to differentiate them while still hinting int to mypy and editors? I thought the solution to the 'magic' was to just use Python to identify the fixture as in mypackage.somemodule.the_fixture.

Then we go and add in multiple simultaneous instances of a fixture and... we're back to some magic for 'which instance do you want?' :]

shoyer commented 4 years ago

A type-based lookup doesn't seem great. We only get one int typed fixture? We have to create not-quite-int types to differentiate them while still hinting int to mypy and editors? I thought the solution to the 'magic' was to just use Python to identify the fixture as in mypackage.somemodule.the_fixture.

Agreed, type-based lookup as described in https://github.com/pytest-dev/pytest/issues/3834#issuecomment-681794705 feels like a solution to a different problem, unless the type annotation includes a Python reference to the fixture function.

ms-lolo commented 4 years ago

I agree. Anything trying to look at the test function arguments (names or types) sounds like a step in the wrong direction and just another version of implicit fixtures. It becomes especially confusing when mixed with things like the standard unittest.mock libraries which works by decorating functions. What @max-sixty suggested seems the least magical and the ability to enable/disable the two types of fixtures seems unrelated. A type annotation on a variable does not tell me what the intent is. A function called use_fixture() can only mean one thing.

altendky commented 4 years ago

I like click but I hesitate on the decorator route for new development at this point. What are the benefits over leveraging annotations?

max-sixty commented 4 years ago

Not sure a config file which enables one behavior but disables the other is feasible. For example, some internal pytest features and many plugins depend on the implicit behavior. Aside from the internals (which could be work around), enabling "explicit" means that you can't use any of the existing plugins?

If we want: a) Backward compatibility b) Ability to enforce these explicit / non-magical fixtures in a project ...then I think we need some sort of config

The config wouldn't need to apply to plugins — e.g. maybe it's only on the fixtures in that project.

markers --strict is similar IIUC?

nicoddemus commented 4 years ago

b) Ability to enforce these explicit / non-magical fixtures in a project

Oh I didn't really realized this was a requirement. My understanding was to allow explicit fixture, not enforce them on a given project.

However I don't think we need to get stuck on this as a requirement, the important thing to focus on is to provide the capability of using explicit fixtures, enforcing them seems secondary (it can even be enforced by a linter, for example).

The config wouldn't need to apply to plugins — e.g. maybe it's only on the fixtures in that project.

Should be possible, but not how things work internally.

markers --strict is similar IIUC?

Not really, it applies to all marks, so it will complain even by markers used by plugins but are not declared explicitly.

graingert commented 4 years ago

but I agree it feels clunky.

Annotated supports arbitrary nesting and aliasing, users wouldn't actually see typing.Annotated in their code

A type-based lookup doesn't seem great. We only get one int typed fixture? We have to create not-quite-int types to differentiate them while still hinting int to mypy and editors?

Again typing.Annoated would work here to runtime annotate an int factory. I'd probably go for a class though:

@pytest.Fixture
@attrs.define
class EphemeralPortReserve:
   v: int
def test_webserver(p: EphemeralPortReserve):
    django.ruinserver(port=p.v)

Here it is with annotated:

def _epr() -> int:
    return 2

EphemeralPortReserve = pytest.DI[int, factory=_epr]
def test_ws(v: EphemeralPortReserve):
    django.ruinserver(port=v)
techdragon commented 4 years ago

@nicoddemus the ability to enforce them was kind of where I was going with the idea that interoperability was potentially an undesirable feature. Even if it meant plugins didn’t work for a while... As long as there’s a simple enough way to make fixtures work both ways (explicit and old style implicit), there’s a lot of people like myself who will step up and start submitting fixes to the libraries that provide fixtures.

@graingert i get It, but that took quite a lot longer to parse in my head than the simple example from @max-sixty. It also requires runtime type introspection which is still (at least last time I looked) a bit of a risky area as far as performance goes. As a hypothesis user, who is imagining this inspection overhead multiplied by a hundred or a thousand times for every test I write, I’m very concerned about anything that could make that slower.

Also @graingert in my head I can’t read they type hint example without simplifying it down to some kind of an @inject decorator that accepts *args and **kwargs of callable objects. The type hinting just doesn’t seem to me, to add enough other than looking slightly more modern since I also have started to associate type hints with more modern looking python code. Do you have larger/non-trivial examples that might be more compelling?

RonnyPfannschmidt commented 4 years ago

By now im strongly opposed to use type hints for parameter configuration, it's just terrifying complex to read

I'd rather use a decorator to pass over the di configuration

Until we enable to actually do it, i don't see value in hypothesis on how it will look, as implementation and hitting issues will reshape the system.

nicoddemus commented 4 years ago

@nicoddemus the ability to enforce them was kind of where I was going with the idea that interoperability was potentially an undesirable feature. Even if it meant plugins didn’t work for a while... As long as there’s a simple enough way to make fixtures work both ways (explicit and old style implicit), there’s a lot of people like myself who will step up and start submitting fixes to the libraries that provide fixtures.

I see, but I don't think we need to enforce this, I suppose it is possible to make the architecture flexible enough to allow for the styles to be mixed or enforced if one wants to (via a configuration option for example).

By now im strongly opposed to use type hints for parameter configuration, it's just terrifying complex to read

I'm leaning towards this as well.

I however find using default values for that (like ward does) to be quite readable:

@pytest.fixture
def random_sample() -> List[int]:
    ...

def test_foo(x: List[int] = random_sample):
    ...

Or:

def random_sample() -> List[int]:
    ...

def test_foo(x: List[int] = Fixture(random_sample)):
    ...

The second form has the advantage of not requiring to decorate the fixture function at all.

Also it seems you can make it work with mypy:

from typing import List, TypeVar, Callable

T = TypeVar("T")

def Fixture(func: Callable[..., T]) -> T:
    ...

def random_sample() -> List[int]:
    return [1, 2]

def test_foo(x: List[int] = Fixture(random_sample)):
    x.append(10)

Until we enable to actually do it, i don't see value in hypothesis on how it will look, as implementation and hitting issues will reshape the system.

Yeah I agree. First order of business would be to change the internals to allow this type of system to be provided by a plugin.

shoyer commented 4 years ago
def random_sample() -> List[int]:
    ...

def test_foo(x: List[int] = Fixture[random_sample]):
    ...

It's a little weird to see type annotation syntax with [] used for default values. Did you mean to write something like test_foo(x: List[int] = Fixture(random_sample)) like what you wrote below?

nicoddemus commented 4 years ago

I did, thanks (corrected original post).

graingert commented 4 years ago

it doesn't seem that bad using typing.Annotated

def random_sample() -> List[int]:
    ...

def test_foo(x: pytest.Fixture[List[int], random_sample]):
    ...

it also supports aliasing:

def random_sample() -> List[int]:
    ...

RandomSample = pytest.Fixture[List[int], random_sample]

def test_foo(x: RandomSample):
    ...
graingert commented 4 years ago

and class based fixtures work nicely too

@attr.frozen
class RandomSample:
    v: List[int]

    @pytest.fixture
    @classmethod
    def fixture(cls) -> RandomSample:
        return cls([1, 2, 3])

def test_foo(y: RandomSample):
    x = y.v
shoyer commented 4 years ago

I would still lean towards a decorator for tests rather than type annotations or default values. Type annotations and default values would be more compact, but the decorator removes all the magic.

In principle, the core test collection part of pytest wouldn't need to know about fixtures at all anymore -- the use_fixture decorator could just be a normal decorator that returns a new test function. For example, if we use a decorator we wouldn't need to pre-define a "scope" for fixtures, and could just rely on Python's normal variable model, e.g.,

def smtp_connection():
    import smtplib
    return smtplib.SMTP("smtp.gmail.com", 587, timeout=5)

@pytest.use_fixture(smtp_connection)  # local-scope
def test_ehlo(connection):
    ...

cached_smtp_connection = functools.lru_cache(smtp_connection)

@pytest.use_fixture(cached_smtp_connection)  # module-scope
def test_ehlo(connection):
    ...
graingert commented 4 years ago

core test collection part of pytest wouldn't need to know about fixtures at all anymore

This seems like something that could be developed on pypi for use with even old versions of pytest

ms-lolo commented 4 years ago

In principle, the core test collection part of pytest wouldn't need to know about fixtures at all anymore

this is what we should be striving for. eliminating any kind of magic and asking test writers to type a few extra characters will eliminate tight coupling between the pytest components and give us a much more flexible and maintainable architecture.

Edit: I started poking around last week at trying to get something basic working but got a bit lost switching between tests that extended unittest.TestCase and plain ones.

Could someone help me understand a little more about what current limitations live in pytest that prevent me from wrapping a normal test function in a decorator? I'd love to help poke at things a bit more and learn more about the pytest internals in the process.

graingert commented 4 years ago

asking test writers to type a few extra characters

test writers could also use pytestmark and a collect_item hook to add this decorator, similar to pytest-asyncio

graingert commented 4 years ago

Could someone help me understand a little more about what current limitations live in pytest that prevent me from wrapping a normal test function in a decorator? I'd love to help poke at things a bit more and learn more about the pytest internals in the process.

@altendky maintains some workarounds for Twisted decorators, and there's workarounds for @mock.patch in pytest core