pytest icon indicating copy to clipboard operation
pytest copied to clipboard

Generate an error when a mark is applied to a fixture

Open nicoddemus opened this issue 7 years ago • 24 comments

Follow up from #1014.

We should generate an error if a @pytest.mark is applied to a fixture.

There is a warning in doc/en/fixture.rst about this problem which should be updated once this issue is dealt with.

nicoddemus avatar Jul 07 '18 12:07 nicoddemus

GitMate.io thinks possibly related issues are https://github.com/pytest-dev/pytest/issues/3346 (Please error when fixtures conflict), https://github.com/pytest-dev/pytest/issues/2872 (mark fixtures ), https://github.com/pytest-dev/pytest/issues/2399 (marks should propogate through fixtures), https://github.com/pytest-dev/pytest/issues/2424 (dynamically generated fixtures), and https://github.com/pytest-dev/pytest/issues/3351 (Is there a way to provide mark with fixture params).

pytestbot avatar Jul 07 '18 12:07 pytestbot

@nicoddemus I am starting working on this.

avirlrma avatar Sep 27 '18 06:09 avirlrma

Great, thanks @avirlrma!

nicoddemus avatar Sep 29 '18 13:09 nicoddemus

@nicoddemus I'm having trouble with code navigation, need some help with it. My initial guess was mark/evaluate.py, but it is called even when there are no marked tests. I am using a test like below and and setting up breakpoints to find where the @pytest.mark.usefixtures('client') takes me but I'm having no luck with the same.

import pytest

@pytest.fixture
def client():
    print('fixture.client')


@pytest.mark.usefixtures('client')
def test_tom():
    print('user jessie')
    assert 0

avirlrma avatar Oct 04 '18 16:10 avirlrma

Hi @avirlrma,

Actually I believe you need to look at where marks are applied to functions; at that point we need to identify if the function where the mark will be applied is already a fixture (possibly by checking one of the attributes which are attached to the function by the fixture decorator).

(Sorry for the brevity as I'm short on time)

nicoddemus avatar Oct 04 '18 16:10 nicoddemus

the pytest fixture parser should raise an error if either the fuction or the wrapped function has markers applied

RonnyPfannschmidt avatar Oct 04 '18 17:10 RonnyPfannschmidt

@RonnyPfannschmidt @nicoddemus Where should we catch the error? I mean when fixture is parsed or when the marks are applied to function?

Also,

@pytest.fixture
@pytest.mark.usefixtures('client')
def user_tom():
    print('user jessie')

As far as I understand decorators, mark will applied first, but since then the function is not a fixture, this should work, but it doesn't. Please help me with this as well.

avirlrma avatar Oct 04 '18 17:10 avirlrma

Where should we catch the error?

You mean raise the error? We don't need to catch the error, only raise it to warn the user.

I mean when fixture is parsed or when the marks are applied to function?

Probably at both places, because as you correctly point out, marks and fixtures can be applied in different order.

Btw, perhaps we should issue a warning instead of an error right away?

nicoddemus avatar Oct 04 '18 21:10 nicoddemus

Ah yes, I meant raise the error. I am working on finding the code for fixtures are parsed and marks are applied. May need some help on that later.

we should issue a warning instead of an error right away?

How do we do that?

avirlrma avatar Oct 05 '18 04:10 avirlrma

we should check both, fixture, and at fixture parsing time, as we currently still need to catch stuff like


@pytest.mark.usefixtures('client')
@pytest.fixture
def user_tom():
    print('user jessie')

RonnyPfannschmidt avatar Oct 05 '18 08:10 RonnyPfannschmidt

ok, so we have to check both. For now I'm starting with the mark first and then fixture case i.e.:

@pytest.fixture
@pytest.mark.usefixtures('client')

This can be raised when the fixture is parsed, so the necessary changes will be done in fixtures.py . Let me know If you find something wrong with the approach. Just need explanation on the warning thing @nicoddemus talked about above.

avirlrma avatar Oct 05 '18 08:10 avirlrma

Let me know If you find something wrong with the approach.

Sounds good, we can discuss over a PR.

Just need explanation on the warning thing @nicoddemus talked about above.

I mean to issue a warning instead of an error, something like:

warnings.warn(pytest.PytestWarning('marks cannot...'), stacklevel=2)

nicoddemus avatar Oct 05 '18 11:10 nicoddemus

got it! I'm working on the PR

avirlrma avatar Oct 06 '18 09:10 avirlrma

I don't understand why applying the usefixtures marker to a fixture should result in an error. Fixtures can already use other fixtures by declaring them in the signature:

@pytest.fixture
def setup_for_bar():
    # setup something....
    pass

@pytest.fixture
def bar(setup_for_bar):
    return 43

What is the reason of not supporting the equivalent way with usefixtures?

@pytest.fixture
def setup_for_bar():
    # setup something....
    pass

@pytest.mark.usefixtures('setup_for_bar')
@pytest.fixture
def bar():
    return 43

The reason I am asking this is that in pytest-factoryboy we have to call exec in order to generate a fixture that requires all the relevant fixtures. The use of exec could be easily avoided if it was possible to mark a fixture with the usefixtures marker.

EDIT: I made a typo that changed the polarity of the sentence "What is the reason of not supporting the equivalent way with usefixtures?"

youtux avatar Aug 24 '19 15:08 youtux

Hi @youtux,

I don't understand why applying the usefixtures marker to a fixture should result in an error.

Mostly because it doesn't do anything currently.

What is the reason of supporting the equivalent way with usefixtures?

This issue is about raising an error if a mark is aplied to a fixture, not to support @pytest.mark.usefixtures in fixtures. 😁

nicoddemus avatar Aug 24 '19 15:08 nicoddemus

Sorry, I meant to say "What is the reason of not supporting the equivalent way with usefixtures?". So I am all about supporting it, not the other way around 😅.

youtux avatar Aug 24 '19 15:08 youtux

Oh OK! 😁

I think it makes sense actually; this task is more of a stop gap in order for people to stop using it and it doing nothing I think. It is easier to remove this when we eventually do support marks in fixtures.

nicoddemus avatar Aug 24 '19 15:08 nicoddemus

So if I understand correctly this is just about making it resulting into an error, but the maintainers are not against this feature per-se, correct?

youtux avatar Aug 24 '19 15:08 youtux

I don't think so; previously it was a problem technically because of how marks worked internally, but since we have refactor and improved that aspect I think it should be fine to do this now.

nicoddemus avatar Aug 24 '19 15:08 nicoddemus

Ok, I'll give it a look then.

youtux avatar Aug 24 '19 15:08 youtux

Thanks!

nicoddemus avatar Aug 24 '19 15:08 nicoddemus

If anyone is still interested, I actually figured out a workaround for this. You can find it here, in this function that dynamically generates fixture functions: https://github.com/pytest-dev/pytest-factoryboy/blob/62474a25a80de3400d862d68d929c0a42a650f7c/pytest_factoryboy/fixturegen.py

Basically I replace the function signature with one that requires the fixtures I need, and just discard them.

youtux avatar May 27 '22 21:05 youtux

Just to mention that I stubbed my toe on this very problem today. My tests miraculously were failing when applying a fixture using mark but when used directly it was fine. It took a while to debug the cause and find this thread. Think it would be great to have at least the warning merged a.s.a.p. and even better would be to actually support this behavior. I would have helped out with a PR but haven't developed pytest yet, maybe in the future though.

sphuber avatar Jun 09 '22 13:06 sphuber

what's the progress on this?

1Mark avatar Jul 08 '22 11:07 1Mark

Any progress on this?

artemonsh avatar Feb 21 '23 16:02 artemonsh

Here is a way you can warn about marks being applied to fixtures (and of course you could get more specific what exactly you would like to warn about & how). I created this proof of concept using pytest==7.2.0:

conftest.py

def pytest_fixture_setup(fixturedef, request):
    marks = getattr(fixturedef.func, 'pytestmark', [])
    assert (
        not marks
    ), f"{fixturedef} has marks, which is probably an error: {marks}"

test_demo.py

import pytest


@pytest.fixture
@pytest.mark.parametrize("x", (1, 2))  # this will trigger the assertion error
def has_mark():
    return "has mark"


@pytest.fixture
def no_mark():
    return "no mark"


def test_demo(has_mark, no_mark):
    pass

zsoldosp avatar Feb 22 '23 08:02 zsoldosp

When did this ship?

Redoubts avatar Dec 01 '23 15:12 Redoubts

@Redoubts not released yet.

nicoddemus avatar Dec 01 '23 16:12 nicoddemus