Fixture "..." called directly

[Alexander-Shukaev]

After upgrading to 3.8.0, I keep getting the

_pytest/fixtures.py:799: RemovedInPytest4Warning: Fixture "..." called directly. Fixtures are not meant to be called directly, are created automatically when test functions request them as parameters. See https://docs.pytest.org/en/latest/fixture.html for more information.

warning message. Is there a way to suppress it?

[RonnyPfannschmidt]

the best way would be fixing your code - so you no longer call an actual fixture directly,

however you also found us a bug, - the error should point to the call location, not to fixtures.py

[Alexander-Shukaev]

Sometimes I want to reuse the code of the fixture and/or parameterize it, e.g.:

@pytest.fixture
def ma_fixture(*args, **kgwargs):
  rd = numpy.random.random(LEN)
  ma = stats.MovingAverage(rd,
                           *args,
                           **kgwargs)
  return rd, ma

@pytest.mark.parametrize('execution_number', range(EXECUTION_NUMBER))
class TestMovingAverage(object):
  @pytest.fixture(autouse=True, scope='class')
  def fixture(self, request):
    request.cls.rd, request.cls.ma = ma_fixture()

  def test_bool(self, execution_number):
    assert self.ma

  def test_len(self, execution_number):
    assert len(self.ma) == len(self.rd)

  def test_iadd(self, ma_fixture, execution_number):
    rd, ma = ma_fixture
    mean = ma.mean
    e = rd[0]
    ma += e
    assert len(ma) == len(rd) + 1
    assert ma.count == len(rd) + 1
    assert ma.mean == mean + (e - mean) / len(ma)

Above, I used it as class-wide fixture (test_bool and test_len) and as test-method fixture (test_iadd).

@pytest.fixture
def me_fixture(index=0, *args, **kgwargs):
  rd = data[index]
  me = stats.MovingExtrema(rd,
                           *args,
                           winlen=WINLEN,
                           check=True,
                           **kgwargs)
  return rd, me

class TestMovingExtrema(object):
  @pytest.fixture(autouse=True, scope='class')
  def fixture(self, request):
    request.cls.rd, request.cls.me = me_fixture()

  def test_lmaxima(self):
    assert self.me.lmaxima == LMAXIMA

  def test_lminima(self):
    assert self.me.lminima == LMINIMA

  def test_def_minabsdiff_default(self):
    minabsdiff = MovingExtrema.def_minabsdiff()
    rd, me = me_fixture(minabsdiff=minabsdiff)
    assert me.lmaxima == LMAXIMA
    assert me.lminima == LMINIMA

Furthermore, I could also pass additional parameters in case of test-method usage (e.g. test_def_minabsdiff_default).

With this new release, you force me to:

1. Duplicate fixture code between class-wide fixtures and test-method fixtures when they are supposed to be the same or create a fixture-like global function (which is not marked as @pytest.fixture and call that function in both class-wide fixtures and test-method fixtures when they are supposed to be the same, what looks like an overkill since pytest fixtures are already functions anyways.
2. Write tedious code for fixtures that may be parameterized by inventing new names for each of the parameterized cases just so that I would not call a fixture directly inside a test method with desired parameters but rather list a parameterless version of it (with a tedious name to avoid name clashes with other test cases) in a test-method argument list.

If there are no maintainable alternatives to my requirements, then I propose @pytest.fixture(callable=True) to suppress those warnings.

[RonnyPfannschmidt]

the correct and dry way is to extract the fixture insides

aka something like the untested :

@pytest.fixture
def ma_fixture():
  return make_ma()

def make_ma(*k, **kw)
  rd = numpy.random.random(LEN)
  ma = stats.MovingAverage(rd, *k, **kw)
  return rd, ma

@pytest.mark.parametrize('execution_number', range(EXECUTION_NUMBER))
class TestMovingAverage(object):
  @pytest.fixture(autouse=True, scope='class')
  def fixture(self, request):
    request.cls.rd, request.cls.ma = make_ma()

  def test_bool(self, execution_number):
    assert self.ma

  def test_len(self, execution_number):
    assert len(self.ma) == len(self.rd)

  def test_iadd(self, ma_fixture, execution_number):
    rd, ma = ma_fixture
    mean = ma.mean
    e = rd[0]
    ma += e
    assert len(ma) == len(rd) + 1
    assert ma.count == len(rd) + 1
    assert ma.mean == mean + (e - mean) / len(ma)

[Alexander-Shukaev]

That's what I mentioned in the first item. I can of course redo everything this way but it's an overkill since fixtures are functions already anyways. Hence, I was surprised and perplexed about the purpose of this warning. Are there any side effects introduced by fixture annotation if called normally?

[RonnyPfannschmidt]

@Alexander-Shukaev the main problem is that fixture declaration is a massive point of unintended mess ups - there is simply so much going wrong with leaving it be a normal function that we decided to deprecate having the fixture definition be a function - people just use it wrong again and again and again

[Alexander-Shukaev]

I see, then my last question would be: does it make sense to turn this warning into an error with the next major bump?

[RonnyPfannschmidt]

it was introduced to turn it into an error at the next major bump

[blueyed]

I propose @pytest.fixture(callable=True) to suppress those warnings.

people just use it wrong again and again and again

If we would allow for callable=True still, then it would indicate that the user knows what they are doing.
Given that messing up is really the only reason to remove it.

[RonnyPfannschmidt]

@blueyed im strictly opposed - thats as complex as literally having just the function extracted completely,
but it creates an easy misuse point for team members that are no as well versed with the finer details

just extract all if the function, there should be a helper to create a new fixture definition from a function perhaps - but there shouldn't be a callable=True

[RonnyPfannschmidt]

aka this is also to prevent the "i know what I'm doing" people from handing a loaded safety off foot-gun to junior members of a dev team (i observed on multiple occasions that this kind of mistake even slip good python developers in review - since its just a function, so the code they see isn't "wrong" yet it is - a good api makes really bad behavior impossible - and as such fixture definitions should never be normal function - as people will use it as such, and then face the "surprise" of unexpectedly fragile tests as the aftermath

[blueyed]

Yes, good points. Let's not add more complexity / hold on to the plan to remove it.
I am using the pattern of a helper function / context manager etc myself anyway.

[tim-schilling]

What about the case of inheritance?

class TestClassA:
    @pytest.fixture
    def something(self):
        return SomeClass()

class TestClassB:
    @pytest.fixture
    def something(self):
        something = super().something()
        something.prop = 'else'
        return something

Will this pattern be explicitly not allowed?

[RonnyPfannschmidt]

@tim-schilling it will be disallowed, because while for really simple things it does work, for more complex fixtures it is generally structurally broken

[aparamon]

I was also greatly surprised that direct fixture calling is deprecated. To me, that is one of power features!

Please consider how useful and readable it is:

@pytest.fixture
def client(request):
    server = request.config.getoption('--server')
    if server == 'app':
        yield from app_client()
    elif server == 'local':
        yield from local_client()
    elif server == 'docker':
        yield from docker_client(request)
    else:
        yield from running_client(request)

where app_client, local_client, docker_client, running_client are fixtures too!
In tests, I can use "automatic" client fixture, but also specific app_client/local_client etc fixtures if required.

It is disturbing that useful feature is deprecated just because some hmmm... under-qualified engineers use it incorrectly.

[RonnyPfannschmidt]

@aparamon just a fyi - your code is full on broken and the moment someone uses 2 of those fixtures at the same time they are at best lucky if nothing blows up

the correct api to select different named fixtures is request.getfixturevalue("name")

just let it sink in for a while that while you complain about "under-qualified" people using it incorrect, you did as well

the problem here is that well-qualified people use it incorrectly since the underlying behavior contracts are unexpectedly complex - its basically backstabbing you as is

[RonnyPfannschmidt]

also note, if you make the server option a choice between the valid client names and set the default to running

your fixture will turn into

@pytest.fixture
def client(request):
    server = request.config.getoption('--server')
    return request.getfixturevalue(server + '_client')

[aparamon]

@RonnyPfannschmidt Thank you for your comments and feedback!

I cannot imagine a use-case for both client and *_client fixtures in the same test, so we are safe to assume that for particular test only one of the fixtures is in play. Under this use-case my code is not broken, which is confirmed by months of test runs. Or I'm missing something important and there is another practical problem?

Is that correct that my code can be rewritten as

@pytest.fixture
def client(request):
    server = request.config.getoption('--server')
    if server == 'app':
        return request.getfixturevalue('app_client')
    elif server == 'local':
        return request.getfixturevalue('local_client')
    elif server == 'docker':
        return request.getfixturevalue('docker_client')
    else:
        return request.getfixturevalue('running_client')

to avoid any subtle possibility of unexpected behavior, and thus warnings? It becomes less straight, but if it's fine I believe I can live with it.

Shouldn't deprecation warning refer to request.getfixturevalue() to make refactoring path more discoverable?

[nicoddemus]

@aparamon, your new version is the recommended one IMO. 👍

Shouldn't deprecation warning refer to request.getfixturevalue() to make refactoring path more discoverable?

It really depends on how the fixture is being used, always mentioning getfixturevalue() might make things more confusing: the sample code in the deprecation docs is an example where mentioning getfixturevalue() would only add to the confusion:

@pytest.fixture
def cell():
    return ...

@pytest.fixture
def full_cell():
    cell = cell()
    cell.make_full()
    return cell

[aparamon]

@nicoddemus Thanks for the pointer!
Might it be that "dispatching fixture" pattern I'm using is common enough to be also covered in deprecation docs? And maybe some other patterns mentioned in this thread?

[RonnyPfannschmidt]

@Alexander-Shukaev good point, addressing it there could be a great help for others 👍

[nicoddemus]

Might it be that "dispatching fixture" pattern I'm using is common enough to be also covered in deprecation docs?

Good idea, would you like to submit a PR?

[aparamon]

@nicoddemus Not immediately, so if someone wishes to do it please go ahead!

[butla]

I'm trying to contribute to one pytest plugin, but I see that its own tests are calling the fixtures directly and raising the warning. Should getfixturevalue be used then, or should the fixture tests be handled only like shown here?

[nicoddemus]

@butla It is a matter of preference, both will work.

Another alternative is to create an indirection:

def fix_value():
    return FixtureValue()

@pytest.fixture
def fix():
   return fix_value()

def test_foo():
    value = fix_value()

[chriskessel]

FWIW, I find the original (I guess incorrect) usage in the example above more readable. No hard coded string and you can click through to the function in your IDE.

    yield from app_client()
vs
    return request.getfixturevalue('app_client')

This change broke hundreds of tests when a build pulled in a more recent version of pytest. And the author is gone, so now I get to go through all this to try and "fix" it. I guess I understand the desire to encourage a better usage, but breaking changes really suck.

[nicoddemus]

breaking changes really suck.

We always introduce warnings before turning things into actual errors to minimize this pain. But we feel you, it does really sucks when things that are working break, even if we intend to improve the overall experience.

[aparamon]

I have now looked into https://github.com/pytest-dev/pytest/blob/master/src/_pytest/fixtures.py, and the warning is produced in wrap_function_to_warning_if_called_directly().

But what is the actual reason not to call the implied request.getfixturevalue() there, if that's "the right thing to do"? I assume we can get current request object in the internal code even if it was not declared as the fixture argument/dependency explicitly?

It might be surprising that function call re-uses cached return-value, but that's not different from e.g. functools.lru_cache: you do expect some "black magic" from certain decorators.

[RonnyPfannschmidt]

@Alexander-Shukaev request.getfixturevalue() is native to the pytest fixture system and ensures pytest is well aware of the objects and their lifetimes, the random call to the function directly is just that - and a common source of actual error

[excitoon]

the correct and dry way is to extract the fixture insides

aka something like the untested :

@pytest.fixture
def ma_fixture():
  return make_ma()

def make_ma(*k, **kw)
  rd = numpy.random.random(LEN)
  ma = stats.MovingAverage(rd, *k, **kw)
  return rd, ma

Not dry at all

[excitoon]

@Alexander-Shukaev the main problem is that fixture declaration is a massive point of unintended mess ups - there is simply so much going wrong with leaving it be a normal function that we decided to deprecate having the fixture definition be a function - people just use it wrong again and again and again

You also dropped possibility of using it from lambdas with this change.

[excitoon]

https://github.com/theY4Kman/pytest-lambda

[excitoon]

Best solution I came up with is, if you want some fixture to be called both as fixture and directly, you remove @pytest.fixture attribute (fortunately in my case I have this possibility) and fill it up manually (immediately or at some point):

def fixture():
    ...

def use(fixture=fixture()):
    ...

or (if you still want to use pytest functionality):

fixture.__wrapped__()