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

Fixed #25251 -- Made data migrations available in TransactionTestCase (2) #6297

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@romgar
Contributor

romgar commented Mar 15, 2016

@timgraham Proposing another solution for #6137.

Flushing and loading initial data migration on the destroy_test_db step.

Show outdated Hide outdated django/db/backends/base/creation.py
@@ -268,6 +273,17 @@ def destroy_test_db(self, old_database_name=None, verbosity=1, keepdb=False, num
settings.DATABASES[self.connection.alias]["NAME"] = old_database_name
self.connection.settings_dict["NAME"] = old_database_name
def _restore_initial_data_migrations(self):
# First, clean the database
call_command('flush', verbosity=0, interactive=False,

This comment has been minimized.

@timgraham

timgraham Mar 15, 2016

Member

What does the flush do? I would expect the tests to leave the database in a clean state already.

@timgraham

timgraham Mar 15, 2016

Member

What does the flush do? I would expect the tests to leave the database in a clean state already.

This comment has been minimized.

@romgar

romgar Mar 15, 2016

Contributor

It was more for "security" to be 100% sure about database state, in case I have missed some possibilities.

But as SimpleTestCase doesn't allow database queries, TestCase rollbacks everything and TransactionTestCase flush everything at the end of the test, I guess it's useless...

Do you think this approach makes more sense and should I create some related tests ?

@romgar

romgar Mar 15, 2016

Contributor

It was more for "security" to be 100% sure about database state, in case I have missed some possibilities.

But as SimpleTestCase doesn't allow database queries, TestCase rollbacks everything and TransactionTestCase flush everything at the end of the test, I guess it's useless...

Do you think this approach makes more sense and should I create some related tests ?

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Mar 15, 2016

Member

I didn't take a deep look, but at first glance it makes sense to me. I don't have any ideas for a test but I wonder if running some of the existing tests with --keepdb (as in ./tests/runtests.py migration_test_data_persistence --settings=test_mysql -k) might work as a regression test.

Member

timgraham commented Mar 15, 2016

I didn't take a deep look, but at first glance it makes sense to me. I don't have any ideas for a test but I wonder if running some of the existing tests with --keepdb (as in ./tests/runtests.py migration_test_data_persistence --settings=test_mysql -k) might work as a regression test.

@romgar

This comment has been minimized.

Show comment
Hide comment
@romgar

romgar Mar 17, 2016

Contributor

@timgraham I have run it on my actual mysql-based project tests. Data are properly kept. I will try with Django suite, as you mentioned.

Contributor

romgar commented Mar 17, 2016

@timgraham I have run it on my actual mysql-based project tests. Data are properly kept. I will try with Django suite, as you mentioned.

Show outdated Hide outdated django/db/backends/base/creation.py
if hasattr(connections[self.connection.alias], "_test_serialized_contents"):
connections[self.connection.alias].creation.deserialize_db_from_string(
connections[self.connection.alias]._test_serialized_contents
)

This comment has been minimized.

@romgar

romgar Apr 27, 2016

Contributor

Shouldn't we have a warning if somebody is using --keepdb option, but with SERIALIZE set to False in its database settings (I guess that will be a situation where we won't have any _test_serialized_contents ?).

@romgar

romgar Apr 27, 2016

Contributor

Shouldn't we have a warning if somebody is using --keepdb option, but with SERIALIZE set to False in its database settings (I guess that will be a situation where we won't have any _test_serialized_contents ?).

This comment has been minimized.

@timgraham

timgraham May 9, 2016

Member

What would the warning say?

@timgraham

timgraham May 9, 2016

Member

What would the warning say?

This comment has been minimized.

@romgar

romgar May 12, 2016

Contributor

Something like: You are using --keepdb option with database SERIALIZE option to False. If you have data migrations and using TransactionTestCase, be aware that, by preventing initial serialisation, the database at the end of the test will be empty.
Sounds not so great :'(

Another solution would be to enforce SERIALIZE to True in case of --keepdb option (and warn the user about this behaviour).

@romgar

romgar May 12, 2016

Contributor

Something like: You are using --keepdb option with database SERIALIZE option to False. If you have data migrations and using TransactionTestCase, be aware that, by preventing initial serialisation, the database at the end of the test will be empty.
Sounds not so great :'(

Another solution would be to enforce SERIALIZE to True in case of --keepdb option (and warn the user about this behaviour).

This comment has been minimized.

@romgar

romgar Jun 30, 2016

Contributor

Any feedback about my previous comment @timgraham? I will continue to work on this issue in the next days, if you think that's the right way.
I will test it manually with different configurations.

@romgar

romgar Jun 30, 2016

Contributor

Any feedback about my previous comment @timgraham? I will continue to work on this issue in the next days, if you think that's the right way.
I will test it manually with different configurations.

This comment has been minimized.

@timgraham

timgraham Jun 30, 2016

Member

I think a documentation note in docs/ref/settings.txt is probably sufficient for now.

@timgraham

timgraham Jun 30, 2016

Member

I think a documentation note in docs/ref/settings.txt is probably sufficient for now.

This comment has been minimized.

@romgar

romgar Jul 2, 2016

Contributor

@timgraham do you want me to update the documentation for incompatibility between TransactionTestCase, keepdb option and SERIALIZE = False in another pull request?
This incompatibility is already present on master and not related to this PR (what I mean is that it won't be fixed by this code).
This PR is only fixing the incompatibility between TransactionTestCase and keepdb option.

The documentation is already explaining the link between SERIALIZE and serialized_rollback=True:

SERIALIZE
Boolean value to control whether or not the default test runner serializes the database
into an in-memory JSON string before running tests (used to restore the database state between 
tests if you don’t have transactions). You can set this to False to speed up creation time if you 
don’t have any test classes with serialized_rollback=True.

I propose to update it to also talk about the keepdb option:

SERIALIZE
Boolean value to control whether or not the default test runner serializes the database
into an in-memory JSON string before running tests (used to restore the database state between 
tests if you don’t have transactions). You can set this to False to speed up creation time if you 
don’t have any test classes with serialized_rollback=True and don't want to keep the database
at the end of the tests (--keepdb option).
@romgar

romgar Jul 2, 2016

Contributor

@timgraham do you want me to update the documentation for incompatibility between TransactionTestCase, keepdb option and SERIALIZE = False in another pull request?
This incompatibility is already present on master and not related to this PR (what I mean is that it won't be fixed by this code).
This PR is only fixing the incompatibility between TransactionTestCase and keepdb option.

The documentation is already explaining the link between SERIALIZE and serialized_rollback=True:

SERIALIZE
Boolean value to control whether or not the default test runner serializes the database
into an in-memory JSON string before running tests (used to restore the database state between 
tests if you don’t have transactions). You can set this to False to speed up creation time if you 
don’t have any test classes with serialized_rollback=True.

I propose to update it to also talk about the keepdb option:

SERIALIZE
Boolean value to control whether or not the default test runner serializes the database
into an in-memory JSON string before running tests (used to restore the database state between 
tests if you don’t have transactions). You can set this to False to speed up creation time if you 
don’t have any test classes with serialized_rollback=True and don't want to keep the database
at the end of the tests (--keepdb option).

This comment has been minimized.

@timgraham

timgraham Jul 2, 2016

Member

Sounds okay.

@timgraham

timgraham Jul 2, 2016

Member

Sounds okay.

@romgar

This comment has been minimized.

Show comment
Hide comment
@romgar

romgar Jul 4, 2016

Contributor

After some testing, I have found a situation where this patch is not working: when the last TransactionTestCase of the test suite has serialize_rollback=False.
In that situation, in the TransactionTestCase teardown, the database is flushed (through flush management command) but the emit_post_migrate_signal signal is emitted.
django.contrib.contenttype app will then create again all content types, but these content types are also in connections[self.connection.alias]._test_serialized_contents, which will lead to unique constraint errors.

We should probably flush the database without this emit_post_migrate_signal before loading serialized data at the end of the test suite to avoid this kind of behavior.

Contributor

romgar commented Jul 4, 2016

After some testing, I have found a situation where this patch is not working: when the last TransactionTestCase of the test suite has serialize_rollback=False.
In that situation, in the TransactionTestCase teardown, the database is flushed (through flush management command) but the emit_post_migrate_signal signal is emitted.
django.contrib.contenttype app will then create again all content types, but these content types are also in connections[self.connection.alias]._test_serialized_contents, which will lead to unique constraint errors.

We should probably flush the database without this emit_post_migrate_signal before loading serialized data at the end of the test suite to avoid this kind of behavior.

@romgar

This comment has been minimized.

Show comment
Hide comment
@romgar

romgar Jul 5, 2016

Contributor

I have added a flush before the data loading, which prevent any data generated with emit_post_migrate_signal to conflict with data deserialization, especially for content types.
Tested with sqlite / mysql / postgresql locally.
The functionality works as expected, I now have to fix the PR according to failing tests.

Contributor

romgar commented Jul 5, 2016

I have added a flush before the data loading, which prevent any data generated with emit_post_migrate_signal to conflict with data deserialization, especially for content types.
Tested with sqlite / mysql / postgresql locally.
The functionality works as expected, I now have to fix the PR according to failing tests.

@romgar

This comment has been minimized.

Show comment
Hide comment
@romgar

romgar Jul 21, 2016

Contributor

Hey @timgraham, I had a look at PR errors, and I don't really know what is happening. The error is:

test_runner.tests.SetupDatabasesTests.test_destroy_test_db_restores_db_name
---
[...]
CommandError: Database test_django_27_pull-requests-trusty couldn't be flushed. Possible reasons:
  * The database isn't running or isn't configured correctly.
  * At least one of the expected database tables doesn't exist.
  * The SQL was invalid.
Hint: Look at the output of 'django-admin sqlflush'. That's the SQL this command wasn't able to run.
The full error: relation "forms_tests_a" does not exist

That's definitely related to flush command, but I'm not sure if I should update my code or mock some calls in the Django test (because this test has nothing to do with db content, but only connection NAME)
Any thoughts?

Contributor

romgar commented Jul 21, 2016

Hey @timgraham, I had a look at PR errors, and I don't really know what is happening. The error is:

test_runner.tests.SetupDatabasesTests.test_destroy_test_db_restores_db_name
---
[...]
CommandError: Database test_django_27_pull-requests-trusty couldn't be flushed. Possible reasons:
  * The database isn't running or isn't configured correctly.
  * At least one of the expected database tables doesn't exist.
  * The SQL was invalid.
Hint: Look at the output of 'django-admin sqlflush'. That's the SQL this command wasn't able to run.
The full error: relation "forms_tests_a" does not exist

That's definitely related to flush command, but I'm not sure if I should update my code or mock some calls in the Django test (because this test has nothing to do with db content, but only connection NAME)
Any thoughts?

@romgar

This comment has been minimized.

Show comment
Hide comment
@romgar

romgar Aug 9, 2016

Contributor

@timgraham tests updated. What else can I do on this PR to go forward?

Contributor

romgar commented Aug 9, 2016

@timgraham tests updated. What else can I do on this PR to go forward?

Show outdated Hide outdated django/db/backends/base/creation.py
if hasattr(connections[self.connection.alias], "_test_serialized_contents"):
# Don't import django.core.management if it isn't needed.
from django.core.management import call_command

This comment has been minimized.

@timgraham

timgraham Aug 9, 2016

Member

Inner imports are only needed to avoid circular imports. Is that the case here?

@timgraham

timgraham Aug 9, 2016

Member

Inner imports are only needed to avoid circular imports. Is that the case here?

This comment has been minimized.

@romgar

romgar Aug 10, 2016

Contributor

That's what I saw in create_test_db function, so I guess there were some issues with this file imports? I just replicated this local import logic.

@romgar

romgar Aug 10, 2016

Contributor

That's what I saw in create_test_db function, so I guess there were some issues with this file imports? I just replicated this local import logic.

Show outdated Hide outdated django/db/backends/base/creation.py
# Flush database, in case of any data created after
# emit_post_migrate_signal
call_command('flush', verbosity=0, interactive=False,

This comment has been minimized.

@timgraham

timgraham Aug 9, 2016

Member

Use hanging indent:

call_command(
    'flush', verbosity=0, interactive=False,
    ...
)
@timgraham

timgraham Aug 9, 2016

Member

Use hanging indent:

call_command(
    'flush', verbosity=0, interactive=False,
    ...
)
@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Aug 9, 2016

Member

It would be nice to try to add some tests for the new functionality, even using mock if nothing else is feasible.

Member

timgraham commented Aug 9, 2016

It would be nice to try to add some tests for the new functionality, even using mock if nothing else is feasible.

Fixed #25251 -- Initial data migration lost after TransactionTestCase
Data loaded in migrations are restored at the beginning of each TransactionTestCase and all the tables are truncated at the end of these test cases.
It means that, at the end of your whole test suite, if there was at least one TransactionTestCase, the migrated data are no more in the database, specially surprising when using --keepdb option.
Now we restore data at the end of the test suite if --keepdb option has been used.
@romgar

This comment has been minimized.

Show comment
Hide comment
@romgar

romgar Sep 13, 2016

Contributor

Tests passing! Ready for the next review @timgraham :-)

Contributor

romgar commented Sep 13, 2016

Tests passing! Ready for the next review @timgraham :-)

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Sep 27, 2016

Member

I see a crash when testing with Django's test suite:

$ ./tests/runtests.py postgres_tests --settings=test_postgres -k
...
Traceback (most recent call last):
  File "/home/tim/code/django/django/db/backends/utils.py", line 64, in execute
    return self.cursor.execute(sql, params)
psycopg2.ProgrammingError: relation "django_site" does not exist
LINE 1: ..."django_site"."domain", "django_site"."name" FROM "django_si...
                                                             ^


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

Traceback (most recent call last):
  File "./tests/runtests.py", line 456, in <module>
    options.exclude_tags,
  File "./tests/runtests.py", line 259, in django_tests
    extra_tests=extra_tests,
  File "/home/tim/code/django/django/test/runner.py", line 541, in run_tests
    self.teardown_databases(old_config)
  File "/home/tim/code/django/django/test/runner.py", line 514, in teardown_databases
    keepdb=self.keepdb,
  File "/home/tim/code/django/django/db/backends/base/creation.py", line 248, in destroy_test_db
    self._restore_initial_data_migrations()
  File "/home/tim/code/django/django/db/backends/base/creation.py", line 281, in _restore_initial_data_migrations
    connections[self.connection.alias]._test_serialized_contents
  File "/home/tim/code/django/django/db/backends/base/creation.py", line 132, in deserialize_db_from_string
    obj.save()
  File "/home/tim/code/django/django/core/serializers/base.py", line 201, in save
    models.Model.save_base(self.object, using=using, raw=True, **kwargs)
  File "/home/tim/code/django/django/db/models/base.py", line 724, in save_base
    update_fields=update_fields)
  File "/home/tim/code/django/django/dispatch/dispatcher.py", line 192, in send
    response = receiver(signal=self, sender=sender, **named)
  File "/home/tim/code/django/django/contrib/sites/models.py", line 117, in clear_site_cache
    del SITE_CACHE[Site.objects.using(using).get(pk=instance.pk).domain]
  File "/home/tim/code/django/django/db/models/query.py", line 382, in get
    num = len(clone)
  File "/home/tim/code/django/django/db/models/query.py", line 241, in __len__
    self._fetch_all()
  File "/home/tim/code/django/django/db/models/query.py", line 1093, in _fetch_all
    self._result_cache = list(self.iterator())
  File "/home/tim/code/django/django/db/models/query.py", line 53, in __iter__
    results = compiler.execute_sql()
  File "/home/tim/code/django/django/db/models/sql/compiler.py", line 835, in execute_sql
    cursor.execute(sql, params)
  File "/home/tim/code/django/django/db/backends/utils.py", line 64, in execute
    return self.cursor.execute(sql, params)
  File "/home/tim/code/django/django/db/utils.py", line 94, in __exit__
    six.reraise(dj_exc_type, dj_exc_value, traceback)
  File "/home/tim/code/django/django/utils/six.py", line 685, in reraise
    raise value.with_traceback(tb)
  File "/home/tim/code/django/django/db/backends/utils.py", line 64, in execute
    return self.cursor.execute(sql, params)
django.db.utils.ProgrammingError: relation "django_site" does not exist
LINE 1: ..."django_site"."domain", "django_site"."name" FROM "django_si...
Member

timgraham commented Sep 27, 2016

I see a crash when testing with Django's test suite:

$ ./tests/runtests.py postgres_tests --settings=test_postgres -k
...
Traceback (most recent call last):
  File "/home/tim/code/django/django/db/backends/utils.py", line 64, in execute
    return self.cursor.execute(sql, params)
psycopg2.ProgrammingError: relation "django_site" does not exist
LINE 1: ..."django_site"."domain", "django_site"."name" FROM "django_si...
                                                             ^


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

Traceback (most recent call last):
  File "./tests/runtests.py", line 456, in <module>
    options.exclude_tags,
  File "./tests/runtests.py", line 259, in django_tests
    extra_tests=extra_tests,
  File "/home/tim/code/django/django/test/runner.py", line 541, in run_tests
    self.teardown_databases(old_config)
  File "/home/tim/code/django/django/test/runner.py", line 514, in teardown_databases
    keepdb=self.keepdb,
  File "/home/tim/code/django/django/db/backends/base/creation.py", line 248, in destroy_test_db
    self._restore_initial_data_migrations()
  File "/home/tim/code/django/django/db/backends/base/creation.py", line 281, in _restore_initial_data_migrations
    connections[self.connection.alias]._test_serialized_contents
  File "/home/tim/code/django/django/db/backends/base/creation.py", line 132, in deserialize_db_from_string
    obj.save()
  File "/home/tim/code/django/django/core/serializers/base.py", line 201, in save
    models.Model.save_base(self.object, using=using, raw=True, **kwargs)
  File "/home/tim/code/django/django/db/models/base.py", line 724, in save_base
    update_fields=update_fields)
  File "/home/tim/code/django/django/dispatch/dispatcher.py", line 192, in send
    response = receiver(signal=self, sender=sender, **named)
  File "/home/tim/code/django/django/contrib/sites/models.py", line 117, in clear_site_cache
    del SITE_CACHE[Site.objects.using(using).get(pk=instance.pk).domain]
  File "/home/tim/code/django/django/db/models/query.py", line 382, in get
    num = len(clone)
  File "/home/tim/code/django/django/db/models/query.py", line 241, in __len__
    self._fetch_all()
  File "/home/tim/code/django/django/db/models/query.py", line 1093, in _fetch_all
    self._result_cache = list(self.iterator())
  File "/home/tim/code/django/django/db/models/query.py", line 53, in __iter__
    results = compiler.execute_sql()
  File "/home/tim/code/django/django/db/models/sql/compiler.py", line 835, in execute_sql
    cursor.execute(sql, params)
  File "/home/tim/code/django/django/db/backends/utils.py", line 64, in execute
    return self.cursor.execute(sql, params)
  File "/home/tim/code/django/django/db/utils.py", line 94, in __exit__
    six.reraise(dj_exc_type, dj_exc_value, traceback)
  File "/home/tim/code/django/django/utils/six.py", line 685, in reraise
    raise value.with_traceback(tb)
  File "/home/tim/code/django/django/db/backends/utils.py", line 64, in execute
    return self.cursor.execute(sql, params)
django.db.utils.ProgrammingError: relation "django_site" does not exist
LINE 1: ..."django_site"."domain", "django_site"."name" FROM "django_si...
def _restore_initial_data_migrations(self):
# Load initial data migrations, if any.
if hasattr(connections[self.connection.alias], "_test_serialized_contents"):
self._flush_db()

This comment has been minimized.

@timgraham

timgraham Sep 27, 2016

Member

Is a separate method needed?

@timgraham

timgraham Sep 27, 2016

Member

Is a separate method needed?

This comment has been minimized.

@romgar

romgar Nov 4, 2016

Contributor

That was the only way I thought about to mock it properly in tests, as the call_command is imported locally...

@romgar

romgar Nov 4, 2016

Contributor

That was the only way I thought about to mock it properly in tests, as the call_command is imported locally...

# Flush database, in case of any data created after
# emit_post_migrate_signal
call_command('flush', verbosity=0, interactive=False,

This comment has been minimized.

@timgraham

timgraham Sep 27, 2016

Member

Use hanging indent:

call_command(
    'flush', verbosity=0, interactive=False,
     database=self.connection.alias, reset_sequences=True,
     inhibit_post_migrate=True,
)
@timgraham

timgraham Sep 27, 2016

Member

Use hanging indent:

call_command(
    'flush', verbosity=0, interactive=False,
     database=self.connection.alias, reset_sequences=True,
     inhibit_post_migrate=True,
)
@romgar

This comment has been minimized.

Show comment
Hide comment
@romgar

romgar Nov 4, 2016

Contributor

@timgraham I will try to reproduce it with your command.

Contributor

romgar commented Nov 4, 2016

@timgraham I will try to reproduce it with your command.

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Nov 6, 2016

Member

Closing in favor of #6137.

Member

timgraham commented Nov 6, 2016

Closing in favor of #6137.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment