Opened 21 months ago

Last modified 4 months ago

#25251 assigned Bug

Inconsistent availability of data migrations in TransactionTestCase when using --keepdb

Reported by: David Szotten Owned by: Romain Garrigues
Component: Testing framework Version: 1.8
Severity: Normal Keywords:
Cc: romain.garrigues.cs@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no 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 (26)

comment:1 Changed 21 months ago by Tim Graham

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 Changed 21 months ago by Tim Graham

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

comment:3 Changed 15 months ago by Romain Garrigues

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 Changed 15 months ago by Tim Graham

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

comment:5 Changed 15 months ago by Romain Garrigues

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

comment:6 Changed 15 months ago by Romain Garrigues

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 Changed 15 months ago by Tim Graham

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 Changed 15 months ago by Romain Garrigues

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 Changed 15 months ago by Tim Graham

Has patch: set
Patch needs improvement: set

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

comment:10 Changed 15 months ago by Romain Garrigues

Patch needs improvement: unset

comment:11 Changed 15 months ago by Romain Garrigues

Can I do something else to help on that issue ?

comment:12 Changed 15 months ago by Tim Graham

Find a colleague to review the patch.

comment:13 Changed 15 months ago by Moritz Sichert

Patch needs improvement: set

Left comments on the PR.

comment:14 Changed 15 months ago by Romain Garrigues

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

comment:15 Changed 14 months ago by Romain Garrigues

Cc: romain.garrigues.cs@… added

comment:16 Changed 14 months ago by Tim Graham

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

comment:17 Changed 14 months ago by Romain Garrigues

Patch needs improvement: unset

comment:18 Changed 14 months ago by Romain Garrigues

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 Changed 14 months ago by Tim Graham

Patch needs improvement: set

comment:20 Changed 9 months ago by Romain Garrigues

Patch needs improvement: unset

comment:21 Changed 8 months ago by Tim Graham

Patch needs improvement: set

The test isn't passing.

comment:22 Changed 8 months ago by Romain Garrigues

Patch needs improvement: unset

comment:23 Changed 7 months ago by Tim Graham

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 Changed 6 months ago by Romain Garrigues

Patch needs improvement: unset

comment:25 Changed 6 months ago by Romain Garrigues

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 Changed 4 months ago by Tim Graham

Patch needs improvement: set

Some cosmetic comments for improvement are on the PR.

Note: See TracTickets for help on using tickets.
Back to Top