Skip to content

Avoid deprecation warnings on pytest 3.0 #393

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

Merged
merged 6 commits into from
Nov 6, 2016
Merged

Avoid deprecation warnings on pytest 3.0 #393

merged 6 commits into from
Nov 6, 2016

Conversation

mdentremont
Copy link
Contributor

  • Use getfixturevalue instead of getfuncargvalue if it exists

- Use getfixturevalue instead of getfuncargvalue if it exists
@mdentremont
Copy link
Contributor Author

Unfortunately I've addressed the warnings by using duplicated try/except blocks - it might be nice to extract a helper function to avoid the duplication, similar to pytest-bdd https://github.com/pytest-dev/pytest-bdd/blob/30076d478c34afc67c41460a2956b430f15e7f90/pytest_bdd/utils.py#L26

I didn't see any other extracted functions in the codebase, so didn't want to break any patterns.

Let me know if you have any suggestions!

@rpkilby
Copy link
Contributor

rpkilby commented Sep 15, 2016

A compat module already exists - it seems like would be appropriate there.

# compat.py
def getfixturevalue(request, value):
    try:
        return request.getfixturevalue(value)
    except AttributeError:
        return request.getfuncargvalue(value)

# fixtures.py
from compat import getfixturevalue
...

getfixturevalue('transactional_db')

@rpkilby rpkilby mentioned this pull request Sep 16, 2016
@mdentremont
Copy link
Contributor Author

@rpkilby Thanks, done!

Copy link
Contributor

@blueyed blueyed left a comment

Choose a reason for hiding this comment

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

What warning were you seeing?

@@ -0,0 +1 @@
{"last_check":"2016-09-26T19:36:56Z","pypi_version":"8.1.2"}
Copy link
Contributor

Choose a reason for hiding this comment

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

That should not be in here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops - deleted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added it to the .gitignore as it seems to be generated when I try to run the tests

@blueyed
Copy link
Contributor

blueyed commented Sep 26, 2016

What warning were you seeing?

Sorry, apparently I only saw the last commit(s) when reviewing.

@mdentremont
Copy link
Contributor Author

@blueyed Any ideas why the builds are failing on me?

@blueyed
Copy link
Contributor

blueyed commented Sep 27, 2016

@mdentremont
You should not import the .compat module at module level (because that also does Django imports).

 pytest_django/plugin.py | 3 ++-
 tests/test_database.py  | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git i/pytest_django/plugin.py w/pytest_django/plugin.py
index ef0fbcd..d464d7d 100644
--- i/pytest_django/plugin.py
+++ w/pytest_django/plugin.py
@@ -14,7 +14,6 @@
 import py
 import pytest

-from .compat import getfixturevalue
 from .django_compat import is_django_unittest  # noqa
 from .fixtures import django_db_setup  # noqa
 from .fixtures import django_db_use_migrations  # noqa
@@ -369,6 +368,7 @@ def _django_db_marker(request):
     """
     marker = request.keywords.get('django_db', None)
     if marker:
+        from .compat import getfixturevalue
         validate_django_db(marker)
         if marker.transaction:
             getfixturevalue(request, 'transactional_db')
@@ -380,6 +380,7 @@ def _django_db_marker(request):
 def _django_setup_unittest(request, django_db_blocker):
     """Setup a django unittest, internal to pytest-django."""
     if django_settings_is_configured() and is_django_unittest(request):
+        from .compat import getfixturevalue
         getfixturevalue(request, 'django_test_environment')
         getfixturevalue(request, 'django_db_setup')

diff --git i/tests/test_database.py w/tests/test_database.py
index 06ba392..cb009d0 100644
--- i/tests/test_database.py
+++ w/tests/test_database.py
@@ -4,7 +4,6 @@
 from django.db import connection
 from django.test.testcases import connections_support_transactions

-from pytest_django.compat import getfixturevalue
 from pytest_django_test.app.models import Item


@@ -33,6 +32,7 @@ class TestDatabaseFixtures:

     @pytest.fixture(params=['db', 'transactional_db'])
     def both_dbs(self, request):
+        from pytest_django.compat import getfixturevalue
         if request.param == 'transactional_db':
             return getfixturevalue(request, 'transactional_db')
         elif request.param == 'db':

try:
return request.getfixturevalue(value)
except AttributeError:
return request.getfuncargvalue(value)
Copy link
Contributor

@blueyed blueyed Sep 27, 2016

Choose a reason for hiding this comment

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

Oh, there might hide some AttributeError in getting the fixture.

Better:

try:
    f = request.getfixturevalue
except AttributeError:
    return request.getfuncargvalue(value)
return f(value)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we're going down this route - might as well just use a hasattr?

Copy link
Contributor

@blueyed blueyed left a comment

Choose a reason for hiding this comment

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

See my comment above.

@mdentremont
Copy link
Contributor Author

mdentremont commented Sep 28, 2016

@blueyed - Oh man, finally realizing why this isn't working: we're importing from compat in some cases, before we try pulling in the DB fixture. This is problematic as I think the DB fixtures are loaded in before Django has gotten a chance to setup.

- This allows importing the function without pulling Django piece
  before Django has gotten a chance to setup
@mdentremont
Copy link
Contributor Author

@blueyed It took awhile - but I finally got this thing to build 😛

@pelme pelme merged commit c1d15cb into pytest-dev:master Nov 6, 2016
@pelme
Copy link
Member

pelme commented Nov 6, 2016

Thanks for the fix, reviews and the persistence to get this working! :)

@mdentremont mdentremont deleted the topic/fix-pytest-deprecations branch January 19, 2017 14:49
mfa pushed a commit to aexeagmbh/pytest-django that referenced this pull request May 17, 2017
timb07 pushed a commit to timb07/pytest-django that referenced this pull request May 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants