Skip to content
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

Generate an error when a mark is applied to a fixture #3664

Open
nicoddemus opened this issue Jul 7, 2018 · 21 comments · May be fixed by #8428
Open

Generate an error when a mark is applied to a fixture #3664

nicoddemus opened this issue Jul 7, 2018 · 21 comments · May be fixed by #8428

Comments

@nicoddemus
Copy link
Member

@nicoddemus nicoddemus commented Jul 7, 2018

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.

@pytestbot
Copy link
Contributor

@pytestbot pytestbot commented Jul 7, 2018

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

@avirlrma
Copy link
Member

@avirlrma avirlrma commented Sep 27, 2018

@nicoddemus I am starting working on this.

@nicoddemus
Copy link
Member Author

@nicoddemus nicoddemus commented Sep 29, 2018

Great, thanks @avirlrma!

@avirlrma avirlrma self-assigned this Oct 4, 2018
@avirlrma
Copy link
Member

@avirlrma avirlrma commented Oct 4, 2018

@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
@nicoddemus
Copy link
Member Author

@nicoddemus nicoddemus commented Oct 4, 2018

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)

@RonnyPfannschmidt
Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt commented Oct 4, 2018

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

@avirlrma
Copy link
Member

@avirlrma avirlrma commented Oct 4, 2018

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

@nicoddemus
Copy link
Member Author

@nicoddemus nicoddemus commented Oct 4, 2018

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?

@avirlrma
Copy link
Member

@avirlrma avirlrma commented Oct 5, 2018

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?

@RonnyPfannschmidt
Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt commented Oct 5, 2018

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')
@avirlrma
Copy link
Member

@avirlrma avirlrma commented Oct 5, 2018

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.

@nicoddemus
Copy link
Member Author

@nicoddemus nicoddemus commented Oct 5, 2018

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)
@avirlrma
Copy link
Member

@avirlrma avirlrma commented Oct 6, 2018

got it! I'm working on the PR

@youtux
Copy link

@youtux youtux commented Aug 24, 2019

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?"

@nicoddemus
Copy link
Member Author

@nicoddemus nicoddemus commented Aug 24, 2019

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

@youtux
Copy link

@youtux youtux commented Aug 24, 2019

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

@nicoddemus
Copy link
Member Author

@nicoddemus nicoddemus commented Aug 24, 2019

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.

@youtux
Copy link

@youtux youtux commented Aug 24, 2019

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?

@nicoddemus
Copy link
Member Author

@nicoddemus nicoddemus commented Aug 24, 2019

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.

@youtux
Copy link

@youtux youtux commented Aug 24, 2019

Ok, I'll give it a look then.

@nicoddemus
Copy link
Member Author

@nicoddemus nicoddemus commented Aug 24, 2019

Thanks!

RalfBarkow pushed a commit to RalfBarkow/Plinth that referenced this issue Dec 17, 2019
Fixtures cannot be currently included into other fixtures by using
@pytest.mark.fixtures('fixture_name')
They have to be included as parameters instead.
See bug: pytest-dev/pytest#3664

Also increase the scope of needs_root to the highest, i.e. session, so that it
can be used by any kind of fixture.

Signed-off-by: Joseph Nuthalapati <njoseph@thoughtworks.com>
graingert added a commit to graingert/pytest that referenced this issue Mar 10, 2021
graingert added a commit to graingert/pytest that referenced this issue Mar 10, 2021
graingert added a commit to graingert/pytest that referenced this issue Mar 10, 2021
@graingert graingert linked a pull request that will close this issue Mar 10, 2021
graingert added a commit to graingert/pytest that referenced this issue Mar 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants