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

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

Closed
Fak3 opened this issue Mar 13, 2020 · 25 comments
Labels

Comments

@Fak3
Copy link

Fak3 commented Mar 13, 2020

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
Copy link
Member

Can you confirm this happens also on pytest 5.4.1?

If yes, then a minimal reproduction would be great.

@Fak3
Copy link
Author

Fak3 commented Mar 13, 2020

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

@Fak3
Copy link
Author

Fak3 commented Mar 13, 2020

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

@bluetech
Copy link
Member

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 pytest-dev/pytest#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
Copy link
Contributor

blueyed commented Mar 14, 2020

@Fak3
Please try #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 pytest-dev/pytest#6014), until it gets done properly, e.g. via something like blueyed/pytest#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.

@blueyed blueyed changed the title django.db.utils.InterfaceError: connection already closed with pytest 5.4.0+ pytest 5.4 skips Django's teardown (causing e.g. "connection already closed" errors) Mar 14, 2020
@Fak3
Copy link
Author

Fak3 commented Mar 14, 2020

Please try #825.

Yes, that PR fixes this issue

blueyed added a commit to blueyed/pytest-django that referenced this issue Mar 15, 2020
blueyed added a commit to blueyed/pytest-django that referenced this issue Mar 15, 2020
blueyed added a commit to blueyed/pytest-django that referenced this issue Mar 18, 2020
@BenjamenMeyer
Copy link

Is pytest-dev/pytest#6947 related?

blueyed added a commit that referenced this issue Mar 31, 2020
* Work around unittest issue with pytest 5.4.{0,1}

Ref: #824

* tox/travis: pytest53
@blueyed
Copy link
Contributor

blueyed commented Mar 31, 2020

Is pytest-dev/pytest#6947 related?

Yes, caused by it.

Fixed in pytest-django 3.9.0 now.

@blueyed blueyed closed this as completed Mar 31, 2020
@PyB1l
Copy link

PyB1l commented Apr 16, 2020

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

@hugos94
Copy link

hugos94 commented May 4, 2020

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

Same problem here!

@nicoddemus
Copy link
Member

nicoddemus commented May 4, 2020

@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 #824 (comment) is now returning 404).

@PyB1l
Copy link

PyB1l commented May 4, 2020

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
Copy link

hugos94 commented May 4, 2020

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:

  • Django==2.2.7
  • pytest==5.4.1
  • pytest-django==3.9.0

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
Copy link
Member

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
Copy link

n-prat commented May 18, 2021

Hello,

The problem seems to have reappeared:

  • pip install -U pytest-django!=4.3.0 then pytest = 626 passed, 6 skipped, 70 warnings
  • pip install -U pytest-django==4.3.0 then pytest = 8 failed, 618 passed, 6 skipped, 70 warnings

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
Copy link
Contributor

browniebroke commented May 19, 2021

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
Copy link

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

@browniebroke
Copy link
Contributor

browniebroke commented May 21, 2021

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
Copy link

n-prat commented May 21, 2021

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
Copy link
Member

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

@browniebroke
Copy link
Contributor

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

@bluetech
Copy link
Member

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
Copy link

salykin commented Jul 30, 2021

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
Copy link
Contributor

browniebroke commented Feb 23, 2022

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
Copy link

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.

beyondgeeks added a commit to beyondgeeks/django-pytest that referenced this issue Sep 6, 2022
* Work around unittest issue with pytest 5.4.{0,1}

Ref: pytest-dev/pytest-django#824

* tox/travis: pytest53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests