Better handling of south migrations for newer versions of south.

[calebbrown]

Hi,

We have tests that require south migrations to be run. This runner currently disables them and our tests fail.

In south >= 0.7.1 there is a helper in south.management.commands called patch_for_test_db_setup that inspects the Django configuration for a setting and either disables migrations or handles them.

This patch changes the _disable_south_management_command() method so that it uses this method.

[pelme]

Thanks for the report!

We are about to release a new version of pytest-django, were most of the code is rewritten.

I will revisit this issue when that release is out, the patch will probably not apply cleanly since we made some big changes to the database setup code.

[calebbrown]

Thanks for taking a look.

How long until the new version drops?

[pelme]

It is released as of now! :)

Can you please update the pull request for the latest master? Thanks!

[xordoquy]

@calebbrown did you rework this issue ?
I'd be interested in being able to run south migrations during the tests in order to ensure that the migrations themselves aren't broken.

[pelme]

I don't really see the point of running migrations with the test suite. It will slow down running the tests a lot. (Please prove me wrong though)

To see if all migrations run, why not just drop the development database and run manage.py migrate?

Also, datamigrations will likely not really test anything since the tables will all be empty, so you will probably never find problems with running data migrations with real data.

What is the benefit of integrating it with the test running, rather than just running from the manage.py command?

[xordoquy]

Actually there are a couple of cases where the migration can fail even with empty database.
For example if you have a FK to a table and forgot to set the migration prerequisites.
This is the kind of things you won't notice - not even on your dev or integration boxes. Unless you start with an empty database chances are that you already have that table while an empty database could lead to an issue (story at the bottom).

My thoughts on the topic are:

• There's almost no point in running the migrations for every test. It indeed slow down things.
• The migration on a populated database isn't the scope of unit or functional tests. It's the staging's job.
• Postponing migration issues down to staging isn't what you want, in particular if it's a library. Tests passes, you tag and then see that you need to redo a version because it won't migrate under certain circonstances.
• You haven't covered the case where you start from an empty database.

My position is that I'd rather have a 10% slower test suite with migration covered rather than the other way round.

This being said there's an interesting alternative:

• Have an extra test that would simply run the migrations and ensures it works well.
• Have a settings we can set with setting_override to turn on or off the south migration

Story about why you want to test your migrations with an empty database.
Last year I was working on a project with someone else. We added a couple of features, created the migrations, tested, moved to production every was working like a charm.

Then we wanted another instance of that project for production. It crashed when trying to deploy it because south tried to create a FK to a table that was to be created in a later migration.

We did not see it because the later migration was already played on all our systems except the empty database.

[pelme]

Testing migrations is indeed tricky. :-)

Are you proposing that the test database should be created by running migrations rather than the standard syncdb way? Would that have caught the problem with the FK/migration ordering that you describe?

My point is that setting up the test database with migrations to catch migration errors is very fragile. Sure, it might catch some errors, but that is more a side effect than an actual test.

I check migrations by first running them locally on my dev machine. I inspect the tables and data manually to make sure it looks good. Then I push it to the staging environment with a full production database copy. I don't think these steps can be skipped even if the test database are constructed by running migrations.

Third party apps that ship migrations are trickier, since they must work in all kinds of environments. I think this is basically impossible to test since there are so many things involved, and creating the test database with migrations wouldn't really help.

[scott-coates]

For anyone who is interested, if you really want to force south to run, you can use this gist to do so:
https://gist.github.com/scoarescoare/6380588

[qris]

I needed to run South migrations because I have data migrations on which my tests depend.

I didn't know about this ticket, only the blanket "migrations are not used" stated on the website, so I went ahead and fixed it independently, kind of similar to @scoarescoare's gist above. This is the monkeypatch I used:

from django.conf import settings
from django.core import management
commands = management.get_commands()
if commands['syncdb'] == 'south' and getattr(settings,
    'SOUTH_TESTS_MIGRATE', True):

    @patch(management, 'call_command')
    def call_command_calling_south_instead_of_django_syncdb(original_function,
        name, *args, **options):

        # undo the damage done by _django_db_setup
        commands['syncdb'] = 'south'

        if name == 'syncdb':
            options['migrate'] = True
        elif name == 'flush':
            return True

        return original_function(name, *args, **options)

It's important to disable the flush command, otherwise django removes the data that our migrations loaded into the table.

Migrations only run when using --create-db, because we use --reuse-db by default. This allows us to test that migrations leave the database in the right state, without slowing down test running most of the time, because they are not always run. Our app isn't designed to run with syncdb, so forcing it to do so is rather weird, and doesn't catch errors such as failing to create migrations for our new models for example.

+1 for fixing this in pytest_django. The competing django-pytest does run migrations:

The management command has been set up so that syncdb will use the django core syncdb if SOUTH_TESTS_MIGRATE is set to False, if south is installed. This prevents migrations from running when running unit tests. This speeds up test setup significantly, but it means your test db may not be identical to production, if you have faulty migrations.

[pelme]

I am thinking of introducing a hook, something like this that could be placed in your conftest.py file:

from django.core.management import call_command

def pytest_django_create_db():
    call_command('migrate')

There seems to be a set of different use cases to customize database setup: #37, #38, #57.

@qris What are your thoughts of this?

[qris]

@pelme thanks for listening! This would work for me if:

• Django syncdb was also replaced with South syncdb (otherwise tables will be created by a global syncdb and South will then try to create them again, and crash); and
• It can be made conditional on the SOUTH_MIGRATE_TESTS setting (the above code clearly could be, but the syncdb switch needs to be as well); and
• It's possible to stop the database being flushed as well.

My first version of the patch actually modified the _django_db_setup fixture (is that the right word for it?), so that approach definitely works. But when I discovered I couldn't monkey-patch pytest fixtures I had to take an alternative route to monkeypatch it for my project.

I didn't see any way to disable the default _django_db_setup fixture, but if that's possible then I can install my own replacement instead, which reads SOUTH_MIGRATE_TESTS and does the Right Thing(tm). Some monkeypatching still appears necessary, because something calls flush after syncdb, and I must disable it to keep the data added by my data migrations.

[blueyed]

Please check out my PR at #129.

It is based on this PR, but also fixes handling of initial data, which is skipped in South (http://south.aeracode.org/ticket/1395#comment:3).