Pytest 4.2.0 seems to break django's @override_settings

[diox]

Upgrading to Pytest 4.2.0 broke our tests: mozilla/addons-server#10556

The issue looked strange - it seemed like django's @override_settings persisted from one class to the next instead of being reset, triggering those weird failures. I've bisected it down to 0f918b1 - before that commit, everything works fine.

Our pip list is huge, I'm working on a reduced testcase (edit: see #4704 (comment)), but this is with:

pytest                         4.2.0
Django                         1.11.18 
pytest-django                  3.4.5

Python 2.7.15 and 3.6.8
Debian stretch

I don't think the version of pytest-django matters here, and since I've bisected it down to a pytest commit I'm filing it here.

[diox]

Note: I've seen #4700 but we don't have skipped classes. I've tried applying #4703 but it didn't have any impact, the issue persisted.

[nicoddemus]

Hi @diox,

Thanks for the detailed report, we really appreciate it.

I'm happy to investigate this later, meanwhile could you please post a minimal reproducible example if you can?

I've zero experience with pytest-django, so would appreciate if @pytest-dev/pytest-django-developers and @pytest-dev/pytest-django-admin could take a look as well.

[blueyed]

@diox
Are you using @override_settings (from Django), or the settings wrapper from pytest-django? (https://github.com/pytest-dev/pytest-django/blob/fe6ddb8c7086accdb9d82746908538a7f805ad70/pytest_django/fixtures.py#L322-L329)

[diox]

I'm using @override_settings. Here is a repo with a reduced testcase: https://github.com/diox/pytest_override_issue_bug_testcase

Install in a virtualenv with pip install -r requirements.txt, and run make. It fails for me. Run the command make runs with -k TestclassWithoutOverrideSettingsUrlConf, or run with pytest==4.1.1, or just comment out the @override_settings call, and it will pass.

[blueyed]

pytest-django has a failing test with pytest 4.2 also, caused by the same commit (0f918b1).

https://github.com/pytest-dev/pytest-django/blob/fe6ddb8c7086accdb9d82746908538a7f805ad70/tests/test_unittest.py#L260-L325:

============================= test session starts ==============================
platform linux -- Python 3.7.2, pytest-4.1.2.dev29+gba452dbc, py-1.7.0, pluggy-0.8.0
Django settings: pytest_django_test.settings_sqlite_file (from ini file)
rootdir: …/Vcs/pytest-django, inifile: setup.cfg
plugins: django-3.4.6.dev6+gfe6ddb8, xdist-1.24.1, forked-0.2
collected 1 item                                                               

tests/test_unittest.py F                                                 [100%]

=================================== FAILURES ===================================
____________ TestUnittestMethods.test_multi_inheritance_setUpClass _____________

self = <test_unittest.TestUnittestMethods object at 0x7fe3bae678d0>
django_testdir = <Testdir local('/tmp/pytest-of-user/pytest-56/test_multi_inheritance_setUpClass0')>

    def test_multi_inheritance_setUpClass(self, django_testdir):
        django_testdir.create_test_module(
            """
            from django.test import TestCase
            from .app.models import Item
    
            # Using a mixin is a regression test, see #280 for more details:
            # https://github.com/pytest-dev/pytest-django/issues/280
    
            class SomeMixin(object):
                pass
    
            class TestA(SomeMixin, TestCase):
                expected_state = ['A']
                state = []
    
                @classmethod
                def setUpClass(cls):
                    super(TestA, cls).setUpClass()
                    cls.state.append('A')
    
                @classmethod
                def tearDownClass(cls):
                    assert cls.state.pop() == 'A'
                    super(TestA, cls).tearDownClass()
    
                def test_a(self):
                    assert self.state == self.expected_state
    
            class TestB(TestA):
                expected_state = ['A', 'B']
    
                @classmethod
                def setUpClass(cls):
                    super(TestB, cls).setUpClass()
                    cls.state.append('B')
    
                @classmethod
                def tearDownClass(cls):
                    assert cls.state.pop() == 'B'
                    super(TestB, cls).tearDownClass()
    
                def test_b(self):
                    assert self.state == self.expected_state
    
            class TestC(TestB):
                expected_state = ['A', 'B', 'C']
    
                @classmethod
                def setUpClass(cls):
                    super(TestC, cls).setUpClass()
                    cls.state.append('C')
    
                @classmethod
                def tearDownClass(cls):
                    assert cls.state.pop() == 'C'
                    super(TestC, cls).tearDownClass()
    
                def test_c(self):
                    assert self.state == self.expected_state
        """
        )
    
        result = django_testdir.runpytest_subprocess("-vvvv", "-s")
>       assert result.parseoutcomes()["passed"] == 6
E       KeyError: 'passed'

…/Vcs/pytest-django/tests/test_unittest.py:324: KeyError
----------------------------- Captured stdout call -----------------------------
running: …/Vcs/pytest-django/.venv/bin/python -mpytest --basetemp=/tmp/pytest-of-user/pytest-56/test_multi_inheritance_setUpClass0/runpytest-0 -vvvv -s
     in: /tmp/pytest-of-user/pytest-56/test_multi_inheritance_setUpClass0
============================= test session starts ==============================
platform linux -- Python 3.7.2, pytest-4.1.2.dev29+gba452dbc, py-1.7.0, pluggy-0.8.0 -- …/Vcs/pytest-django/.venv/bin/python
cachedir: .pytest_cache
Django settings: tpkg.the_settings (from environment variable)
rootdir: /tmp/pytest-of-user/pytest-56/test_multi_inheritance_setUpClass0, inifile: tox.ini
plugins: xdist-1.24.1, forked-0.2, django-3.4.6.dev6+gfe6ddb8
collecting ... collected 6 items

tpkg/test_the_test.py::TestA::test_a Creating test database for alias 'default' ('/tmp/test_oef41m0d_inner')...
Destroying old test database for alias 'default' ('/tmp/test_oef41m0d_inner')...
Operations to perform:
  Apply all migrations: app, auth, contenttypes
Running pre-migrate handlers for application auth
Running pre-migrate handlers for application contenttypes
Running pre-migrate handlers for application app
Running migrations:
  Applying app.0001_initial... OK (0.002s)
  Applying contenttypes.0001_initial... OK (0.003s)
  Applying contenttypes.0002_remove_content_type_name... OK (0.007s)
  Applying auth.0001_initial... OK (0.012s)
  Applying auth.0002_alter_permission_name_max_length... OK (0.008s)
  Applying auth.0003_alter_user_email_max_length... OK (0.007s)
  Applying auth.0004_alter_user_username_opts... OK (0.007s)
  Applying auth.0005_alter_user_last_login_null... OK (0.007s)
  Applying auth.0006_require_contenttypes_0002... OK (0.001s)
  Applying auth.0007_alter_validators_add_error_messages... OK (0.007s)
  Applying auth.0008_alter_user_username_max_length... OK (0.007s)
  Applying auth.0009_alter_user_last_name_max_length... OK (0.007s)
Running post-migrate handlers for application auth
Adding permission 'Permission object (None)'
Adding permission 'Permission object (None)'
Adding permission 'Permission object (None)'
Adding permission 'Permission object (None)'
Adding permission 'Permission object (None)'
Adding permission 'Permission object (None)'
Adding permission 'Permission object (None)'
Adding permission 'Permission object (None)'
Adding permission 'Permission object (None)'
Adding permission 'Permission object (None)'
Adding permission 'Permission object (None)'
Adding permission 'Permission object (None)'
Running post-migrate handlers for application contenttypes
Adding permission 'Permission object (None)'
Adding permission 'Permission object (None)'
Adding permission 'Permission object (None)'
Adding permission 'Permission object (None)'
Running post-migrate handlers for application app
Adding permission 'Permission object (None)'
Adding permission 'Permission object (None)'
Adding permission 'Permission object (None)'
Adding permission 'Permission object (None)'
FAILED
tpkg/test_the_test.py::TestB::test_a FAILED
tpkg/test_the_test.py::TestB::test_b FAILED
tpkg/test_the_test.py::TestC::test_a FAILED
tpkg/test_the_test.py::TestC::test_b FAILED
tpkg/test_the_test.py::TestC::test_c FAILEDDestroying test database for alias 'default' ('/tmp/test_oef41m0d_inner')...


=================================== FAILURES ===================================
_________________________________ TestA.test_a _________________________________

self = <tpkg.test_the_test.TestA testMethod=test_a>

    def test_a(self):
>       assert self.state == self.expected_state
E       AssertionError: assert ['A', 'A'] == ['A']
E         Left contains more items, first extra item: 'A'
E         Full diff:
E         - ['A', 'A']
E         + ['A']

tpkg/test_the_test.py:26: AssertionError
_________________________________ TestB.test_a _________________________________

self = <tpkg.test_the_test.TestB testMethod=test_a>

    def test_a(self):
>       assert self.state == self.expected_state
E       AssertionError: assert ['A', 'B', 'A', 'B'] == ['A', 'B']
E         Left contains more items, first extra item: 'A'
E         Full diff:
E         - ['A', 'B', 'A', 'B']
E         + ['A', 'B']

tpkg/test_the_test.py:26: AssertionError
_________________________________ TestB.test_b _________________________________

self = <tpkg.test_the_test.TestB testMethod=test_b>

    def test_b(self):
>       assert self.state == self.expected_state
E       AssertionError: assert ['A', 'B', 'A', 'B'] == ['A', 'B']
E         Left contains more items, first extra item: 'A'
E         Full diff:
E         - ['A', 'B', 'A', 'B']
E         + ['A', 'B']

tpkg/test_the_test.py:42: AssertionError
_________________________________ TestC.test_a _________________________________

self = <tpkg.test_the_test.TestC testMethod=test_a>

    def test_a(self):
>       assert self.state == self.expected_state
E       AssertionError: assert ['A', 'B', 'C', 'A', 'B', 'C'] == ['A', 'B', 'C']
E         Left contains more items, first extra item: 'A'
E         Full diff:
E         - ['A', 'B', 'C', 'A', 'B', 'C']
E         + ['A', 'B', 'C']

tpkg/test_the_test.py:26: AssertionError
_________________________________ TestC.test_b _________________________________

self = <tpkg.test_the_test.TestC testMethod=test_b>

    def test_b(self):
>       assert self.state == self.expected_state
E       AssertionError: assert ['A', 'B', 'C', 'A', 'B', 'C'] == ['A', 'B', 'C']
E         Left contains more items, first extra item: 'A'
E         Full diff:
E         - ['A', 'B', 'C', 'A', 'B', 'C']
E         + ['A', 'B', 'C']

tpkg/test_the_test.py:42: AssertionError
_________________________________ TestC.test_c _________________________________

self = <tpkg.test_the_test.TestC testMethod=test_c>

    def test_c(self):
>       assert self.state == self.expected_state
E       AssertionError: assert ['A', 'B', 'C', 'A', 'B', 'C'] == ['A', 'B', 'C']
E         Left contains more items, first extra item: 'A'
E         Full diff:
E         - ['A', 'B', 'C', 'A', 'B', 'C']
E         + ['A', 'B', 'C']

tpkg/test_the_test.py:58: AssertionError
=========================== 6 failed in 0.21 seconds ===========================
=========================== short test summary info ============================
FAILED tests/test_unittest.py::TestUnittestMethods::test_multi_inheritance_setUpClass
=========================== 1 failed in 0.77 seconds ===========================

[blueyed]

Maybe pytest-django's method for handling Django unittests needs to be adjusted for the new behavior/code in pytest?
It was introduced initially in pytest-dev/pytest-django@96f5fb1. I have no time to investigate currently though.

[blueyed]

@diox
Can you try to reproduce it without pytest-django?

[diox]

Interesting, it seems to be working without pytest-django. With the following patch applied to my testcase repo (need to pip uninstall pytest-django first if installed already), tests are passing:

diff --git a/conftest.py b/conftest.py
new file mode 100644
index 0000000..075048d
--- /dev/null
+++ b/conftest.py
@@ -0,0 +1,6 @@
+import pytest
+
+@pytest.mark.trylast
+def pytest_configure():
+    import django
+    django.setup()
diff --git a/requirements.txt b/requirements.txt
index efa3723..b2adbb8 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -8,7 +8,6 @@ pkg-resources==0.0.0
 pluggy==0.8.1
 py==1.7.0
 pytest==4.2.0
-pytest-django==3.4.5
 pytz==2018.9
 scandir==1.9.0
 six==1.12.0

Edit: rolled the patch into a separate branch too.

[blueyed]

Good to know.. so likely a bug/issue caused by pytest-django then.

I am releasing pytest-django 3.4.6, which disallows pytest 4.2.0 (I've hoped this could be fixed in pytest 4.2.1, which is still the case maybe, but it might have been better to use <4.2 instead).

[RonnyPfannschmidt]

based on the debugging i'm inclined to close this one as pytest-django issue

[nicoddemus]

Let's close this then; if further debugging reveals we need to change pytest, we can re-open or create a new issue. 👍

[blueyed]

It turns out that pytest and pytest-django do a similar thing here, ending in the methods being called twice.
Skipping the processing in pytest-django for pytest.__version__ <= '4.2' fixes it there (pytest-dev/pytest-django#700).

1. Is the version check sane? (i.e. comparing it to a string; IIRC there was something in this regard lately)
2. I do not fully grasp currently what pytest-django is doing, but maybe pytest's method could be made compatible to it? (likely not though)

[nicoddemus]

Cool finding @blueyed.

1. Comparing versions with strings is not a good idea, I suggest using from pkg_resources import parse_version instead:

   parse_version(pytest.__version__) < parse_version('4.2')

2. Probably not; if it is trying to "fix" the same problem pytest fixes, then just skipping it on based on pytest version seems better, because you can rip that code out when dropping support for pytest < 4.2 in a few years.