Opened 9 years ago

Last modified 2 months ago

#25251 assigned Bug

Inconsistent availability of data from migrations in TestCases when using --keepdb

Reported by: David Szotten Owned by: David Sanders
Component: Testing framework Version: 1.8
Severity: Normal Keywords:
Cc: romain.garrigues.cs@…, rpkilby@…, jedie, Sarah Boyce, David Sanders, Sage Abdullah Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

I may have misunderstood something, but keepdb doesn't work as i expect when used with TransactionTestCase.

# models.py

class Model(models.Model):
    field = models.TextField()

# migrations

# 0001: default, as created by makemigrations
# 0002: datamigration

def create_data(apps, schema_editor):
    Model = apps.get_model("app", "Model")
    Model.objects.create(field='foo')


class Migration(migrations.Migration):

    dependencies = [
        ('app', '0001_initial'),
    ]

    operations = [
        migrations.RunPython(create_data)
    ]


# tests.py

from .models import Model

class Test(TransactionTestCase):
    def test_foo(self):
        self.assertEqual(Model.objects.count(), 1)

fails with --keepdb (then second time so feature is being used)

needs a non-sqlite db so as to not use :memory:

Change History (45)

comment:1 by Tim Graham, 9 years ago

A TransactionTestCase resets the database after the test runs by truncating all tables. For this reason, the docs say, "Any initial data loaded in migrations will only be available in TestCase tests and not in TransactionTestCase tests." I think that technically the data is available in the first TransactionTestCase (since truncation hasn't happened yet), but not any others. Perhaps this should be fixed or the docs clarified.

comment:2 by Tim Graham, 9 years ago

Summary: TransactionTestCase and --keepdbInconsistent availability of data migrations in TransactionTestCase when using --keepdb
Triage Stage: UnreviewedAccepted

comment:3 by Romain Garrigues, 9 years ago

I'm facing the same problem: my initial data are no more in the database at the end of my test suite, even with --keepdb option.
As TransactionTestCase is always flushing data on the tearDown, why not reloading the serialized data after the flush, instead of on the setUp step ?
I have tried a naive approach by moving the logic related to string serialized data loading on the tearDown step, https://github.com/romgar/django/commit/7219308cf8f196463d03d1407b0ad0e9b918a3db and the data is then kept.
Not really sure about any other impact anywhere else.
I run the django test suite without any failure.
Any feedbacks on that ?

comment:4 by Tim Graham, 9 years ago

If you could submit a patch with that approach along with a regression test, I'll take a closer look.

comment:5 by Romain Garrigues, 9 years ago

Great ! I will spend some time to create this regression test then.

comment:6 by Romain Garrigues, 9 years ago

I had a quick look at django test suite and didn't managed to easily get where the keepdb option is tested.
I have seen admin_scripts.tests to launch some commands during tests and migration_test_data_persistance.tests related to TransactionTestCase data persistence.

A relevant test in that situation would be:

  • launch the test command with --keepdb option (or even directly the TestRunner ?) over an app that contains initial data migrations and a TransactionTestCase,
  • check after the return of this command if the database is still containing initial data migrations,
  • clean the database that has been created through the test command.

I would appreciate some guidance and/or code references to help me.

comment:7 by Tim Graham, 9 years ago

I think keepdb isn't an important part of testing this (it doesn't really have any tests). Instead you could use an approach similar to what's done in tests/test_utils/test_transactiontestcase.py where you invoke TransactionTestCase's methods within a test and check the availability of the data.

comment:8 by Romain Garrigues, 9 years ago

Patch submitted https://github.com/django/django/pull/6137 with a regression test.

I will be glad to hear any comment about it.

I have created a new folder test_utils2 because I needed some migrations and didn't want to create the migrations related to already existing models in test_utils.

comment:9 by Tim Graham, 9 years ago

Has patch: set
Patch needs improvement: set

Please uncheck "Patch needs improvement" when tests are passing.

comment:10 by Romain Garrigues, 9 years ago

Patch needs improvement: unset

comment:11 by Romain Garrigues, 9 years ago

Can I do something else to help on that issue ?

comment:12 by Tim Graham, 9 years ago

Find a colleague to review the patch.

comment:13 by Moritz Sichert, 9 years ago

Patch needs improvement: set

Left comments on the PR.

comment:14 by Romain Garrigues, 9 years ago

Thanks @MoritzS for spending some of your time on it !!

comment:15 by Romain Garrigues, 9 years ago

Cc: romain.garrigues.cs@… added

comment:16 by Tim Graham, 9 years ago

Left a few more comments. Please uncheck "Patch needs improvement" when the PR is updated.

comment:17 by Romain Garrigues, 9 years ago

Patch needs improvement: unset

comment:18 by Romain Garrigues, 9 years ago

I have updated the rollback section and backward incompatible changes. Not really sure if if a good idea to add a "Changed in Django 1.9" and "Changed in Django 1.10" sections at the same time.

comment:19 by Tim Graham, 9 years ago

Patch needs improvement: set

comment:20 by Romain Garrigues, 8 years ago

Patch needs improvement: unset

comment:21 by Tim Graham, 8 years ago

Patch needs improvement: set

The test isn't passing.

comment:22 by Romain Garrigues, 8 years ago

Patch needs improvement: unset

comment:23 by Tim Graham, 8 years ago

Patch needs improvement: set

$ ./tests/runtests.py postgres_tests --settings=test_postgres -k is crashing with the patch where it seems to work fine currently.

comment:24 by Romain Garrigues, 8 years ago

Patch needs improvement: unset

comment:25 by Romain Garrigues, 8 years ago

Owner: changed from nobody to Romain Garrigues
Status: newassigned

I updated the MR according to my discussion with @aaugustin.
It impacted the DiscoverRunner explicit test ordering. I didn't want to update the current tests to ensure backward-compatibility, I created new ones for TransactionTestCase tests ordering.

comment:26 by Tim Graham, 8 years ago

Patch needs improvement: set

Some cosmetic comments for improvement are on the PR.

comment:27 by Ryan P Kilby, 8 years ago

Cc: rpkilby@… added

comment:28 by jedie, 6 years ago

confirm this bug with Django==1.11.13

Any news, work-a-round for this?

comment:29 by jedie, 6 years ago

Cc: jedie added

comment:30 by Romain Garrigues, 6 years ago

I would be keen to continue and have a look again, but I previously had a hard time finding reviewers for it.
There currently are only cosmetic changes requested by Tim Graham, and I would prefer to update them only when the approach used to fix the issue is approved first.

comment:31 by Romain Garrigues, 6 years ago

Patch needs improvement: unset

comment:32 by Tim Graham, 6 years ago

Patch needs improvement: set

comment:33 by Tim Graham <timograham@…>, 6 years ago

Resolution: fixed
Status: assignedclosed

In b3b1d3d:

Fixed #25251 -- Made data migrations available in TransactionTestCase when using --keepdb.

Data loaded in migrations were restored at the beginning of each
TransactionTestCase and all the tables are truncated at the end of
these test cases. If there was a TransactionTestCase at the end of
the test suite, the migrated data weren't restored in the database
(especially unexpected when using --keepdb). Now data is restored
at the end of each TransactionTestCase.

comment:34 by Tim Graham <timograham@…>, 6 years ago

In 9fa0d37:

Refs #25251 -- Filtered out skipped tests when processing the test suite to set _next_serialized_rollback.

comment:35 by Tim Graham, 6 years ago

There's still an issue with this. Test methods that are decorated with skip doesn't have the __unittest_skip__ attribute set so the filtering of skipped tests doesn't work in that case (see PR comment). Since it seems a solution for that isn't forthcoming, I'm going to revert the patches to get the build that runs with --reverse passing again. I'll also close #29948 (a regression after the initial fix) which should be addressed if another patch for this issue is provided. PR

comment:36 by Tim Graham <timograham@…>, 6 years ago

In 4c7c608:

Reverted "Fixed #25251 -- Made data migrations available in TransactionTestCase when using --keepdb."

This reverts commits b3b1d3d45fc066367f4fcacf0b06f72fcd00a9c6 and
9fa0d3786febf36c87ef059a39115aa1ce3326e8 due to reverse build failures
for which a solution isn't forthcoming.

comment:37 by Tim Graham, 6 years ago

Resolution: fixed
Status: closednew

comment:38 by Markus Holtermann, 4 years ago

Summary: Inconsistent availability of data migrations in TransactionTestCase when using --keepdbInconsistent availability of data from migrations in TestCases when using --keepdb

A few more insights in what's happening:

  1. The issue occurs when TransactionTestCases are involved.
  2. Data will be unavailable even in regular TestCases.

Let's assume an empty database. Additionally, the database must support transactions (e.g. PostgreSQL or MySQL (but not MyISAM)). Let's further say you have two test classes (TestRegular(TestCase) and TestTx(TransactionTestCase)) with the test methods TestRegular.test_1 and TestTx.test_2.

  • When you run manage.py test --keepdb for the first time, Django will apply all migrations. That means, data created within those migrations will be present.
  • At the end of the test suite, however, Django does _not_ reinsert serialized test data, leaving the database empty after the first --keepdb run.
  • Running manage.py test --keepdb again means, the database is empty (has no data that was previously created by the migrations). This means, tests in regular TestCases (e.g. test_1() will now fail as well.

comment:39 by Markus Holtermann, 4 years ago

Owner: changed from Romain Garrigues to Markus Holtermann
Status: newassigned

Here's a first draft at restoring data at the end of the test suite: https://github.com/django/django/pull/14147

comment:40 by Blaž Šnuderl, 3 years ago

Anyone still looking into this? I would be interested to pick this up

comment:41 by Markus Holtermann, 3 years ago

Owner: Markus Holtermann removed
Status: assignednew

Feel free to take over https://github.com/django/django/pull/14147, Blaž. I'm not using TransactionTestCase anymore at the moment.

comment:42 by Sarah Boyce, 16 months ago

Cc: Sarah Boyce added

comment:43 by David Sanders, 8 months ago

Cc: David Sanders added

comment:44 by David Sanders, 8 months ago

Needs documentation: set
Needs tests: set
Owner: set to David Sanders
Status: newassigned

comment:45 by Sage Abdullah, 2 months ago

Cc: Sage Abdullah added
Note: See TracTickets for help on using tickets.
Back to Top