Tests with MigratorTestCase cause failures in other tests

maiksprenger commented 4 years ago

Hi, I'm working on a medium-sized Django codebase and recently introduced django-test-migrations to test our migrations. It's a great project and solves a need that many serious Django apps have. We'd like to see it improved by fixing a blocking issue we're seeing.

Unfortunately, when our test suite isn't ordered in the usual fashion, I'm seeing failures. This is true for random ordering, and predictable but non-standard ordering (see below).

I'm careful to draw conclusions, but to me it seems likely that the issue is with our use of django-test-migrations. It doesn't always appear, but it only appears when those tests are run.

I'm a bit at loss about what might be causing the issue, and don't know how to debug further. I do have some work-time available to help debug this issue. I'd be interested to know if you've heard of this issue before. And please let me know how I can assist.

Things I did

Ran test suite without new tests in random order multiple times: no failures
Ran test suite with new tests in random order: failures almost every time so far
Ran test suite with new tests in standard order: no failures
Ran test suite with new tests in parallel on CircleCI: failures
Ran test suite with new tests and clear ContentType cache before and after each test: failures. I did this because it was documented as possible solution here. Also see #10827.

Offending tests

The two tests that seem to be causing the unrelated failures are here: https://gist.github.com/maikhoepfel/0c4c52c86f047c79b85d8b4e98af194f I don't think they do anything special.

The tests I added aren't the first tests for migrations. We've been employing something very similar to the blog post where it all started. Only when we added django-test-migrations did we see the issue. This is surprising to me, because the actual code executed is similar.

The failures

django.db.utils.IntegrityError: insert or update on table "core_stock" violates foreign key constraint "core_stock_content_type_id_d8f48c0c_fk_django_content_type_id" DETAIL:  Key (content_type_id)=(1) is not present in table "django_content_type".
self = <django.db.backends.utils.CursorWrapper object at 0x7f4b5f881dd8>
sql = 'SET CONSTRAINTS ALL IMMEDIATE', params = None
ignored_wrapper_args = (False, {'connection': <django.db.backends.postgresql.base.DatabaseWrapper object at 0x7f4b675bd4a8>, 'cursor': <django.db.backends.utils.CursorWrapper object at 0x7f4b5f881dd8>})

    def _execute(self, sql, params, *ignored_wrapper_args):
        self.db.validate_no_broken_transaction()
        with self.db.wrap_database_errors:
            if params is None:
>               return self.cursor.execute(sql)
E               psycopg2.errors.ForeignKeyViolation: insert or update on table "core_stock" violates foreign key constraint "core_stock_content_type_id_d8f48c0c_fk_django_content_type_id"
E               DETAIL:  Key (content_type_id)=(1) is not present in table "django_content_type".

venv/lib/python3.6/site-packages/django/db/backends/utils.py:82: ForeignKeyViolation

The above exception was the direct cause of the following exception:

self = <tests.ondemand.views.test_views.StoreCartTestCase testMethod=test_checkout_cart_pricing_rule_limit_add_occurences>
result = <TestCaseFunction test_checkout_cart_pricing_rule_limit_add_occurences>

    def __call__(self, result=None):
        """
        Wrapper around default __call__ method to perform common Django test
        set up. This means that user-defined Test Cases aren't required to
        include a call to super().setUp().
        """
        testMethod = getattr(self, self._testMethodName)
        skipped = (
            getattr(self.__class__, "__unittest_skip__", False) or
            getattr(testMethod, "__unittest_skip__", False)
        )

        if not skipped:
            try:
                self._pre_setup()
            except Exception:
                result.addError(self, sys.exc_info())
                return
        super().__call__(result)
        if not skipped:
            try:
>               self._post_teardown()

venv/lib/python3.6/site-packages/django/test/testcases.py:274: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
venv/lib/python3.6/site-packages/django/test/testcases.py:1009: in _post_teardown
    self._fixture_teardown()
venv/lib/python3.6/site-packages/django/test/testcases.py:1177: in _fixture_teardown
    connections[db_name].check_constraints()
venv/lib/python3.6/site-packages/django/db/backends/postgresql/base.py:246: in check_constraints
    self.cursor().execute('SET CONSTRAINTS ALL IMMEDIATE')
venv/lib/python3.6/site-packages/sentry_sdk/integrations/django/__init__.py:487: in execute
    return real_execute(self, sql, params)
venv/lib/python3.6/site-packages/django/db/backends/utils.py:67: in execute
    return self._execute_with_wrappers(sql, params, many=False, executor=self._execute)
venv/lib/python3.6/site-packages/django/db/backends/utils.py:76: in _execute_with_wrappers
    return executor(sql, params, many, context)
venv/lib/python3.6/site-packages/django/db/backends/utils.py:82: in _execute
    return self.cursor.execute(sql)
venv/lib/python3.6/site-packages/django/db/utils.py:89: in __exit__
    raise dj_exc_value.with_traceback(traceback) from exc_value
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <django.db.backends.utils.CursorWrapper object at 0x7f4b5f881dd8>
sql = 'SET CONSTRAINTS ALL IMMEDIATE', params = None
ignored_wrapper_args = (False, {'connection': <django.db.backends.postgresql.base.DatabaseWrapper object at 0x7f4b675bd4a8>, 'cursor': <django.db.backends.utils.CursorWrapper object at 0x7f4b5f881dd8>})

    def _execute(self, sql, params, *ignored_wrapper_args):
        self.db.validate_no_broken_transaction()
        with self.db.wrap_database_errors:
            if params is None:
>               return self.cursor.execute(sql)
E               django.db.utils.IntegrityError: insert or update on table "core_stock" violates foreign key constraint "core_stock_content_type_id_d8f48c0c_fk_django_content_type_id"
E               DETAIL:  Key (content_type_id)=(1) is not present in table "django_content_type".

venv/lib/python3.6/site-packages/django/db/backends/utils.py:82: IntegrityError

sobolevn commented 4 years ago

Looks like this is due to the content-type issue. We can possibly leave a database in a broken state after testing migrations.

@skarzi what do you think?

maiksprenger commented 4 years ago

I just compared https://github.com/wemake-services/django-test-migrations/blob/master/django_test_migrations/contrib/unittest_case.py and https://github.com/skarzi/django-test-migrations/blob/master/test_migrations/mixins.py. A striking difference is that django-test-migrations disconnects signals. The Django issue makes me think the contenttypes are created in a receiver that listens to migrations. A theory I have is that the because that signal isn't emitted, the contenttypes aren't created after a MigratorTestCase tore down. To verify this theory, I removed the _pre_setup and _post_teardown methods from MigratorTestCase. Two test runs with random order have passed so far. Maybe you can shed some light on why the signals are disconnected? Neither @skarzi nor the original blog post are doing that.

sobolevn commented 4 years ago

Original issue about signals #68 and #11

skarzi commented 4 years ago

Probably we should disable signals only for Migrator.apply_initial_migration and Migrator.apply_tested_migration, but not for Migrator.reset to apply all migrations and emit post_migrate signal to setup one content types and all permissions for each model like django test case do, however, it's just a guess and I will validate it later on this week.

sobolevn commented 4 years ago

Probably we should disable signals only for Migrator.apply_initial_migration and Migrator.apply_tested_migration, but not for Migrator.reset to apply all migrations and emit post_migrate signal to setup one content types and all permissions for each model like django test case do

Sounds reasonble!

maiksprenger commented 4 years ago

@skarzi I think you're onto something there. I just ran our test suite with a modified MigratorTestCase, and the issues went away. I also confirmed that the issues are still present without the modifications.

I modified tearDown to restore signals before calling reset. For that, I also removed the _post_teardown method. The complete class is here: https://gist.github.com/maikhoepfel/433fef487b0a8f0a4ba0e72b76dc945c#file-unittest_case-py-L33

I'm happy to provide a PR, but I'm unsure how to fix the same issue for the pytest fixture. My feeling is that handling signals is a core aspect of this library, and should move into the Migrator class. I think one could update apply_[initial|tested]_migration to silence signals before they start, and restore them before they return. That means a slight change in behavior, but should still fix the issue. It would also mean signals are handled correctly for people using Migrator directly.

Please let me know if you think that the signals handling code should move into Migrator. I'm happy to make a PR for it either way.

maiksprenger commented 4 years ago

I've made https://github.com/wemake-services/django-test-migrations/pull/133 as a preview PR. Two tests fail because I slightly changed how signals behave. Happy to fix them if you think that's a good direction.

sobolevn commented 4 years ago

@maikhoepfel yes, looks like it is a good direction! Thanks a lot for your time and effort!

maiksprenger commented 4 years ago

@sobolevn Thank you for the kind words. And you're most welcome, thanks for making a super useful package!

wemake-services / django-test-migrations