tearDown does not delete the test data before to migrate back to latest

[jrobichaud]

Let assume we want a migration to rather fail when the data is not correct (Ex: require human intervention to fix the data) and we want to test that.

1. We create test data
2. Call self.run_migration()
3. Migration throws exception
4. We catch the exception and validate the data migration had no effect, test succeeds so far (everything is fine so far)
5. Then teardown run and exception is thrown. Test fail.
6. Database is now in a bad state

The teardown of MigrationTest will run the migration again to restore the previous state of the migration which will fail since the test data will be still there.

The workaround is to do the teardown of the data ourself but it is impractical, I would assume MigrationTest would clear the test data before to roll the migrations.

[plumdog]

That's true, and it's definitely a shame that this situation would upset all future tests.

However - in principle, at least - clearing the data could also be considered a "bad state" for some future migrations if they assume certain things do exist from previous migrations. (I wouldn't describe doing that as "best practice" necessarily, but I'm sure it will happen.) So if MigrationTest did the data-wipe before running migrations in tearDown, this could trip-up migrations that would work otherwise. (Hope that made sense, if not I'll knock together an example.)

So I think I'm inclined to say that it would be more surprising (and harder to workaround) if MigrationTest were to do the data-wipe. We could add it as an optional switch though I think.

You can probably just do something like:

class MigrationFailTestCase(MigrationTest):

    def test_fail_hard(self):
        # put data in bad state
        MyModel.objects.create(state='bad')

        with self.assertRaises(Exception):
            self.run_migration()

        # put data back into sane state, eg:
        MyModel.objects.all().delete()

or maybe even

class MigrationFailTestCase(MigrationTest)
    def tearDown(self):
        call_command('flush')
        super().tearDown()

would work for your case.

See what TransactionTestCase does which is a bit more involved, but caters for things like multiple databases.

Hope that's helpful.

[jrobichaud]

The workaround you propose is fine when you expect a migration to fail, but if someone did not do that and something goes wrong in a migration test the failure has potentially catastrophic side effects on other tests.

It is pretty hard to debug, the call stack does not relates to "tearDown" since the migration is called from "call_command".

Good tests should revert the state of any resources they use so I am sure rollbacking automatically the test data should be a good thing. Don't you think?

[plumdog]

If there was a way of saying "restore DB state such that it will pass all future migrations", then absolutely. But I am not convinced that such a thing exists. And we're using TransactionTestCase here, so the DB can't help us with a rollback. Only you with your project know how the data needs to look to pass the future migrations.

As for debugging, it might help if tearDown did a try-except around the call_command so it could re-raise and explain saying "failed to restore state in tearDown". Though in py2 without raise from this would lose information which is annoying, but probably better than the current situation.

Good tests should revert the state of any resources they use so I am sure rollbacking automatically the test data should be a good thing. Don't you think?

Absolutely. But we're already through the proverbial looking-glass here. MigrationTest is really abusing Django's TestCase. If we really want guaranteed restoration, then the safest way of getting the db back on track would be to drop it and rebuild which I would love, but then the tests would take a stupidly long time.

Relatedly, Django's docs mention something similar in the warning in this section and suggests using serialized_rollback = True. Not sure if this would help or not. Might be worth trying, or investigating what it does. It also mentions different backends - what DB engine are you using?

So I think the options are:

1. As now but try-except to explain the tearDown failure, and leave the restoration up to the developer.
2. A class-level boolean called something like flush_db_after_hard_fail that would basically enable my call_command('flush') snippet from before, defaulting to False.
3. As above but defaulting to True, though this changes existing behaviour and may introduce the problem you found for others.
4. Something else, maybe?

[jrobichaud]

I like Option 1 but could it be this option instead of 2/3?:

Run a savepoint() in the setUp after having ran the migration to the first migration and then call savepoint_rollback() in the tearDown before to run the migrations again?

[plumdog]

So you mean make the savepoint just after we run the migrations down in setUp, then restore to the savepoint just before we run them back again in tearDown?

Or make the savepoint just before we run the migrations down in setUp, then just restore in tearDown, no migrations back again needed?

The second option has the benefit that removes the tearDown migration completely, but has the problem that it will not work at all on mysql. But the first option may well work, and I'd be interested to know if it works for the situation you encountered if you can try it out.

[jrobichaud]

I run tests with sqlite and MySQL.

I did some tests with rollback and it does work inside a transaction block. See documentation

So I made this decorator that actually works pretty well.

def idempotent_transaction(func):
    @transaction.atomic
    def func_wrapper(*args, **kwargs):
        sp = savepoint()
        try:
            func(*args, **kwargs)
        except:
            raise
        finally:
            savepoint_rollback(sp)
    return func_wrapper

When I decorate my test function with it the issue in the tearDown does not occur.

[jrobichaud]

Of course this decorator is only "safe" when testing a custom migrations that does not change the schema.

I suppose the Django migration table is also rolled back but the schema remain changed. This could leave the DB in a pretty bad state.

[jrobichaud]

Here's a test for the itempotent_transaction:

class TestIdempotentTransaction(TestCase):

    def test_is_idempotent(self):
        @idempotent_transaction
        def create_user():
            models.User.objects.create(
                username='foo',
            )
        create_user()
        self.assertFalse(models.User.objects.exists())

[jrobichaud]

@plumdog I was thinking to create a PR(with tests) for this decorator. Does that make sense to you?

[plumdog]

Yeah, it looks like a neat solution to me.

[jrobichaud]

@plumdog, I created the pull request. However I couldn't make it to work with django < 1.7.

[plumdog]

PR merged. Version 0.0.13 now on PyPI. Thanks for contributing!