pytest-dev / pytest-django

A Django plugin for pytest.
https://pytest-django.readthedocs.io/
Other
1.39k stars 342 forks source link

pytest 5.4 skips Django's teardown (causing e.g. "connection already closed" errors) #824

Closed Fak3 closed 4 years ago

Fak3 commented 4 years ago

pytest-django==3.5.1 django versions tested: 2.2.11 and 3.0.4

When one test fails, all tests which use database launched after this test will fail with traceback:

test/apiv2/put_inputevent__test.py:34: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
/home/u1/.virtualenvs/npserver/lib/python3.7/site-packages/model_mommy/mommy.py:54: in make
    return mommy.make(_save_kwargs=_save_kwargs, **attrs)
/home/u1/.virtualenvs/npserver/lib/python3.7/site-packages/model_mommy/mommy.py:228: in make
    return self._make(commit=True, commit_related=True, _save_kwargs=_save_kwargs, **attrs)
/home/u1/.virtualenvs/npserver/lib/python3.7/site-packages/model_mommy/mommy.py:265: in _make
    instance = self.instance(self.model_attrs, _commit=commit, _save_kwargs=_save_kwargs)
/home/u1/.virtualenvs/npserver/lib/python3.7/site-packages/model_mommy/mommy.py:289: in instance
    instance.save(**_save_kwargs)
/home/u1/.virtualenvs/npserver/lib/python3.7/site-packages/django/db/models/base.py:741: in save
    force_update=force_update, update_fields=update_fields)
/home/u1/.virtualenvs/npserver/lib/python3.7/site-packages/django/db/models/base.py:779: in save_base
    force_update, using, update_fields,
/home/u1/.virtualenvs/npserver/lib/python3.7/site-packages/django/db/models/base.py:851: in _save_table
    forced_update)
/home/u1/.virtualenvs/npserver/lib/python3.7/site-packages/django/db/models/base.py:900: in _do_update
    return filtered._update(values) > 0
/home/u1/.virtualenvs/npserver/lib/python3.7/site-packages/django/db/models/query.py:760: in _update
    return query.get_compiler(self.db).execute_sql(CURSOR)
/home/u1/.virtualenvs/npserver/lib/python3.7/site-packages/django/db/models/sql/compiler.py:1469: in execute_sql
    cursor = super().execute_sql(result_type)
/home/u1/.virtualenvs/npserver/lib/python3.7/site-packages/django/db/models/sql/compiler.py:1138: in execute_sql
    cursor = self.connection.cursor()
/home/u1/.virtualenvs/npserver/lib/python3.7/site-packages/django/db/backends/base/base.py:256: in cursor
    return self._cursor()
/home/u1/.virtualenvs/npserver/lib/python3.7/site-packages/django/db/backends/base/base.py:235: in _cursor
    return self._prepare_cursor(self.create_cursor(name))
/home/u1/.virtualenvs/npserver/lib/python3.7/site-packages/django/db/utils.py:89: in __exit__
    raise dj_exc_value.with_traceback(traceback) from exc_value
/home/u1/.virtualenvs/npserver/lib/python3.7/site-packages/django/db/backends/base/base.py:235: in _cursor
    return self._prepare_cursor(self.create_cursor(name))
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <django.db.backends.postgresql.base.DatabaseWrapper object at 0x7fe8150ed208>, name = None

    def create_cursor(self, name=None):
        if name:
            # In autocommit mode, the cursor will be used outside of a
            # transaction, hence use a holdable cursor.
            cursor = self.connection.cursor(name, scrollable=False, withhold=self.connection.autocommit)
        else:
>           cursor = self.connection.cursor()
E           django.db.utils.InterfaceError: connection already closed

/home/u1/.virtualenvs/npserver/lib/python3.7/site-packages/django/db/backends/postgresql/base.py:223: InterfaceError

Downgrading to pytest==5.3.5 fixes the issue.

Not yet sure if this is pytest issue or pytest-django.

bluetech commented 4 years ago

Can you confirm this happens also on pytest 5.4.1?

If yes, then a minimal reproduction would be great.

Fak3 commented 4 years ago

Yes, 5.4.1 too. Will try to create minimal case.

Fak3 commented 4 years ago

Created minimal django project: https://github.com/Fak3/testproject-824-git

bluetech commented 4 years ago

Thanks for the reproduction!

It looked to me like a problem in pytest's unittest support, so I looked at changes made to that in pytest in 5.4.x. Reverting https://github.com/pytest-dev/pytest/pull/5996 "unittest: do not use TestCase.debug() with --pdb" (and other commits in that PR) fixes the problem.

cc @blueyed @nicoddemus who worked on that PR and probably have a better idea than me.

blueyed commented 4 years ago

@Fak3 Please try https://github.com/pytest-dev/pytest-django/pull/825. The problem is that Django's (rather custom) teardown gets not called (because this is throwing an exception to get out of unittest's __call__, so that --pdb can be used before any teardown - although you could also just add a pdb.set_tracce() and be done).

(It's the next round of the long-standing issue from 2016, and I still think it should have been reverted first at least instead (ref https://github.com/pytest-dev/pytest/pull/6014), until it gets done properly, e.g. via something like https://github.com/blueyed/pytest/pull/107))

pytest-django could intercept Django's _post_teardown (https://github.com/django/django/blob/1a09708dcb2f60ad5ca0a75b8de9619356f74ff6/django/test/testcases.py#L274), and do it after the call, but that would still require to first patch pytest to not use the current approach.

Fak3 commented 4 years ago

Please try #825.

Yes, that PR fixes this issue

BenjamenMeyer commented 4 years ago

Is https://github.com/pytest-dev/pytest/issues/6947 related?

blueyed commented 4 years ago

Is pytest-dev/pytest#6947 related?

Yes, caused by it.

Fixed in pytest-django 3.9.0 now.

PyB1l commented 4 years ago

I think that the issue persist in pytest-django 3.9.0 #835

hugos94 commented 4 years ago

I think that the issue persist in pytest-django 3.9.0 #835

Same problem here!

nicoddemus commented 4 years ago

@PyB1l or @hugos94, any chance of testing this with pytest's latest master? If the problem still persists, would appreciate a minimal project to reproduce the problem (the one provided in https://github.com/pytest-dev/pytest-django/issues/824#issuecomment-598857918 is now returning 404).

PyB1l commented 4 years ago

For my tests i managed to isolate the issue in Django==3.0.2. For Django===3.0.5 and all the lastet version of pytest, pytest-django, i have no issues. @hugos94 could you check your versions and let us know?

hugos94 commented 4 years ago

For my tests i managed to isolate the issue in Django==3.0.2. For Django===3.0.5 and all the lastet version of pytest, pytest-django, i have no issues. @hugos94 could you check your versions and let us know?

I am using the following versions:

Apparently the problem only happens after making requests in views of the application. In tests that i use graphql, the error doesn't occur.

nicoddemus commented 4 years ago

pytest 5.4.2 is out with the fix I mentioned earlier. If people can try that version and see if this is still a regression, would appreciate it. 👍

n-prat commented 3 years ago

Hello,

The problem seems to have reappeared:

Only the 2 tests below have an issue, which is weird considering the high number of tests still working fine:

@pytest.mark.parametrize('email', (
        'test@aaa.fr',
        'test+aaa@test.com',
        'test+aaa@test.reallylongtld',
        'test@abc.def.ghi.test.reallylongtldaaaaaaaaaaaaaaa',
        'test@tld-minus.com',
        't!#$%&es!#$%&t@tld-minus.com',
        # Those are NOT valid, but we accept them to avoid an overly
        # complicated regex. In any case, the true validation SHOULD be to send
        # a confirmation email.
        'test---@tld-minus.com',
    ))
    @pytest.mark.django_db
    def test_email_valid(self, db_setup_company: Company, email: str) -> None:
        company = db_setup_company

        user = User.objects.create(email=email, company=company)

        assert list(user.building_ids) == []

    @pytest.mark.parametrize('email', (
        'aaaa@bbbb@tld-minus.com',
        '@@@test@@@@tld-minus.com',
    ))
    @pytest.mark.django_db
    def test_email_invalid(self, db_setup_company: Company, email: str) -> None:
        company = db_setup_company

        with pytest.raises(IntegrityError) as exc:
            User.objects.create(email=email, company=company)

        assert 'violates check constraint "email_valid_chk"' in str(exc)

freeze.txt

browniebroke commented 3 years ago

I noticed this problem as well. Do you prefer to re-open this bug or create a new issue? FWIW, here is the list of my other dependencies.

django==3.0.14
pytest-cov==2.12.0
pytest-env==0.6.2
pytest-forked==1.3.0
pytest-freezegun==0.4.2
pytest-instafail==0.4.2
pytest-mock==3.6.1
pytest-socket==0.4.0
pytest-subtests==0.4.0
pytest-vcr==1.0.2
pytest-xdist==2.2.1
pytest==6.2.4

My Django version is different from @nathanprat (3.0 vs 3.2) so it doesn't seem relevant. I see 3 common points with the above freeze.txt:

pytest==6.2.4
pytest-django==4.3.0
pytest-mock==3.6.1

UPDATE:

after further investigation, I've created a minimal project with out issue, you can see the failure in the action executions.

I've tracked down the problem to a custom fixture in our project. Doesn't sounds like a regression of this bug after all.

BenjamenMeyer commented 3 years ago

@browniebroke if you would, could you explain why that particular fixture is causing the problem? Might help others with finding similar issues.

browniebroke commented 3 years ago

I don't know why (yet) unfortunately, I've just managed to isolate this as the root cause, but I didn't persist further with the upgrade due to lack of time.

In case it helps, this fixture is supposed to persist model data across test cases within a class, a bit like setTestData does for Django's TestCase class. It's working for simple use cases, but at times, I've seen some side effects across tests, which are not present with setTestData.

n-prat commented 3 years ago

Thanks for the investigation.

The bug was indeed caused by a class-level db fixture.

I don't have time to investigate deeper right now, but the code is below if anyone is interested:

@pytest.fixture(name='class_db', scope='class')
def fixture_class_db(request: 'Any',
                     django_db_setup: 'Any',  # pylint: disable=unused-argument
                     django_db_blocker: 'Any') -> None:
    """
    To avoid the error with 'fixture_db_setup(db)':

    "ScopeMismatch: You tried to access the 'function' scoped fixture 'db'
    with a 'module' scoped request object, involved factories"

    We reimplement a 'db'-like fixture class-scoped.
    """
    # Copy-pasted from pytest_django.fixtures
    if "django_db_reset_sequences" in request.fixturenames:
        request.getfixturevalue("django_db_reset_sequences")
    if ("transactional_db" in request.fixturenames
            or "live_server" in request.fixturenames):
        request.getfixturevalue("transactional_db")
    else:
        _django_db_fixture_helper(
            request,
            django_db_blocker,
            transactional=False)
bluetech commented 3 years ago

@browniebroke If you change your code from django_db_blocker.unblock() to with django_db_blocker.unblock(): ... does it help any?

browniebroke commented 3 years ago

Thanks for the suggestion @bluetech. I tried your suggestion (I think) but no luck yet... Was it the solution you had in mind?

bluetech commented 3 years ago

Yep. Although it was actually also correct before, my eye just glanced over the addfinalizer calls. Since you're now using a context manager you should be able to remove the request.addfinalizer(django_db_blocker.restore). You can also convert the request.addfinalizer(functools.partial(TestCase._rollback_atomics, atomics)) to a regular call after the yield.

(I understand this doesn't actually fix your problem. What I think you are trying to achieve -- safe sharing of DB data beyond a single test -- is currently a missing feature in pytest-django)

salykin commented 3 years ago

Just for note.

In some cases it happens when you override TestCase::setUpClass() and TestCase::tearDownClass() methods but forget to call super-method within its. Please check you code these methods right.

I had made this mistake. After inserting super().tearDownClass() all errors gone. This thread helped me to note my fault: https://code.djangoproject.com/ticket/31065.

browniebroke commented 2 years ago

Just a quick update on my earlier issue is now resolved in pytest-django 4.5.0 and above 😄

Not sure what nor who solved it, but I just wanted to say thanks to the maintainer!

thomasgubler commented 2 years ago

In some cases it happens when you override TestCase::setUpClass() and TestCase::tearDownClass() methods but forget to call super-method within its. Please check you code these methods right.

as an additional note: for me it happened when calling super().setUpClass() from a overriden TestCase::setUp() by mistake. When I removed the erroneous call the overall issue was gone.