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

provide error fixture applied to the same func #2849

Merged

Conversation

ApaDoctor
Copy link
Contributor

Ref #2334

provide error fixture applied to the same func

provide error fixture applied to the same func
@RonnyPfannschmidt
Copy link
Member

i am fairly certain that unless we raise a deprecation warning for one feature iteration at least its going to break people quite a bit unexpectedly

@nicoddemus i'd like your opinion on that

@RonnyPfannschmidt
Copy link
Member

oh, i just cross-checked the prior pr in #2814 - since @nicoddemus suggested the error it may be fine

@RonnyPfannschmidt
Copy link
Member

for completeness please add a unittest

something like


def test_fixture_disallow_twice():
   with pytest.raises(ValueError):
       @pytest.fixture
       @pytest.fixture
       def foo():
          pass

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ApaDoctor! Agree that we should add a test for it.

I updated the changelog a bit, hope you don't mind (I gotta say that I just love being able to quickly edit files directly while reviewing a PR 😄 )

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 92.773% when pulling 132fb61 on ApaDoctor:disable-repeated-fixture into 3318e53 on pytest-dev:features.

@RonnyPfannschmidt
Copy link
Member

@nicoddemus thanks for finishing it up

@RonnyPfannschmidt RonnyPfannschmidt merged commit 6e62fc9 into pytest-dev:features Apr 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants