Fixed #25251 -- Made data migrations available in TransactionTestCase. #6137

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
@romgar
Contributor

romgar commented Feb 12, 2016

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.

I propose to restore data at the end of a TransactionTestCase (just after the flush), to be sure that the next test will be in the expected environment (data loaded from initial migrations) and the database will be in the expected state at the end of the test running suite.

This PR follows the discussion related to https://code.djangoproject.com/ticket/25251 with @timgraham.

@romgar

This comment has been minimized.

Show comment
Hide comment
@romgar

romgar Feb 13, 2016

Contributor

I will have a look on generated errors. One seems trivial (python unicode decorator), the remaining ones are unexpected, specially on sqlite db that I have tested locally with py2/py3.

It seems a bit tricky to test an internal function of a TransactionTestCase in itself, as _fixture_teardown is then called twice. Hard to know if it's my update or the test that is wrong.

Contributor

romgar commented Feb 13, 2016

I will have a look on generated errors. One seems trivial (python unicode decorator), the remaining ones are unexpected, specially on sqlite db that I have tested locally with py2/py3.

It seems a bit tricky to test an internal function of a TransactionTestCase in itself, as _fixture_teardown is then called twice. Hard to know if it's my update or the test that is wrong.

@romgar

This comment has been minimized.

Show comment
Hide comment
@romgar

romgar Feb 14, 2016

Contributor

After some side effects with other tests, specially when defining migrations, I tried to mock the _test_serialized_contents to isolate the impacts of my new test.

Seems better now, I had to update TestSerializedRollbackInhibitsPostMigrate which was impacted by my code change.

Tests are passing on windows, waiting for default tests. travis seems to be stuck since build #5490. It should be better with last PR updates.

Contributor

romgar commented Feb 14, 2016

After some side effects with other tests, specially when defining migrations, I tried to mock the _test_serialized_contents to isolate the impacts of my new test.

Seems better now, I had to update TestSerializedRollbackInhibitsPostMigrate which was impacted by my code change.

Tests are passing on windows, waiting for default tests. travis seems to be stuck since build #5490. It should be better with last PR updates.

tests/test_utils/models.py
+ will_survive = models.CharField(max_length=255)
+
+ def __str__(self):
+ return self.will_survive

This comment has been minimized.

@charettes

charettes Feb 14, 2016

Member

Please re-use on of the existing models.

@charettes

charettes Feb 14, 2016

Member

Please re-use on of the existing models.

+ initial_data_migration = '[]'
+ _connections_test_serialized_content = {}
+
+ def _pre_setup(self):

This comment has been minimized.

@MoritzS

MoritzS Feb 19, 2016

Contributor

I feel like we shouldn't override "private" methods to test, I also don't see why that would be necessary here.

@MoritzS

MoritzS Feb 19, 2016

Contributor

I feel like we shouldn't override "private" methods to test, I also don't see why that would be necessary here.

This comment has been minimized.

@romgar

romgar Feb 19, 2016

Contributor

I was sharing the same feeling, but I'm testing something that is related to the testing environment, which is a kind of special case.

By overwriting these methods, I want to be sure to control the _test_serialized_contents data during the whole test. It is important to mock them also in TestSerializedRollbackInhibitsPostMigrate test.

If we don't mock it, the logic here tries to load some data from previous django test suite serialized contents (because the test suite has some initial data migrations), which is adding some dependencies between tests in the test suite.

This mock should be done every time we want to test something with TransactionTestCase and serialized_rollback = True to avoid test dependencies. Maybe some documentation to explain it would be nice.

@romgar

romgar Feb 19, 2016

Contributor

I was sharing the same feeling, but I'm testing something that is related to the testing environment, which is a kind of special case.

By overwriting these methods, I want to be sure to control the _test_serialized_contents data during the whole test. It is important to mock them also in TestSerializedRollbackInhibitsPostMigrate test.

If we don't mock it, the logic here tries to load some data from previous django test suite serialized contents (because the test suite has some initial data migrations), which is adding some dependencies between tests in the test suite.

This mock should be done every time we want to test something with TransactionTestCase and serialized_rollback = True to avoid test dependencies. Maybe some documentation to explain it would be nice.

This comment has been minimized.

@timgraham

timgraham Mar 10, 2016

Member

Yes, it would be good to add a docstring.

@timgraham

timgraham Mar 10, 2016

Member

Yes, it would be good to add a docstring.

@MoritzS

This comment has been minimized.

Show comment
Hide comment
@MoritzS

MoritzS Feb 19, 2016

Contributor

The way the TestSerializedRollbackInhibitsPostMigrate works, it will break if you don't use your mixin. I suggest changing it to:

class TestSerializedRollbackInhibitsPostMigrate(TransactionTestCase):
    """
    TransactionTestCase._fixture_teardown() inhibits the post_migrate signal
    for test classes with serialized_rollback=True.
    """
    available_apps = None
    serialized_rollback = True

    @mock.patch('django.test.testcases.call_command')
    def test(self, call_command):
        # with a mocked call_command(), this doesn't have any effect.
        self._fixture_teardown()
        call_command.assert_called_with(
            'flush', interactive=False, allow_cascade=False,
            reset_sequences=False, inhibit_post_migrate=True,
            database='default', verbosity=0,
        )

It was written the way it is because of this comment but it is way easier to just explicitly set available_apps = None. That way it also doesn't mess with your changes.

Contributor

MoritzS commented Feb 19, 2016

The way the TestSerializedRollbackInhibitsPostMigrate works, it will break if you don't use your mixin. I suggest changing it to:

class TestSerializedRollbackInhibitsPostMigrate(TransactionTestCase):
    """
    TransactionTestCase._fixture_teardown() inhibits the post_migrate signal
    for test classes with serialized_rollback=True.
    """
    available_apps = None
    serialized_rollback = True

    @mock.patch('django.test.testcases.call_command')
    def test(self, call_command):
        # with a mocked call_command(), this doesn't have any effect.
        self._fixture_teardown()
        call_command.assert_called_with(
            'flush', interactive=False, allow_cascade=False,
            reset_sequences=False, inhibit_post_migrate=True,
            database='default', verbosity=0,
        )

It was written the way it is because of this comment but it is way easier to just explicitly set available_apps = None. That way it also doesn't mess with your changes.

+ serialized_rollback = True
+ initial_data_migration = '[{"model": "test_utils.car", "pk": 1, "fields": {"name": "K 2000"}}]'
+
+ def _post_teardown(self):

This comment has been minimized.

@MoritzS

MoritzS Feb 19, 2016

Contributor

The test should be possible by just having two test methods, that each test if the car is still there, shouldn't it?

def test_first_car(self):
    self.assertTrue(Car.objects.exists())
def test_second_car(self):
    self.assertTrue(Car.objects.exists())

Then you have the benefit that you don't have to override _post_teardown().

@MoritzS

MoritzS Feb 19, 2016

Contributor

The test should be possible by just having two test methods, that each test if the car is still there, shouldn't it?

def test_first_car(self):
    self.assertTrue(Car.objects.exists())
def test_second_car(self):
    self.assertTrue(Car.objects.exists())

Then you have the benefit that you don't have to override _post_teardown().

This comment has been minimized.

@romgar

romgar Feb 19, 2016

Contributor

These tests won't show the problem, they will be ok because the current logic is loading data from migrations before launching each test.
But after these 2 tests, your database will be empty, even with --keepdb option.

I overwrite _post_teardown() to be able to test that my data are still there after each test, and tearDown() is not enough because the cleaning logic is in _post_teardown().

@romgar

romgar Feb 19, 2016

Contributor

These tests won't show the problem, they will be ok because the current logic is loading data from migrations before launching each test.
But after these 2 tests, your database will be empty, even with --keepdb option.

I overwrite _post_teardown() to be able to test that my data are still there after each test, and tearDown() is not enough because the cleaning logic is in _post_teardown().

This comment has been minimized.

@MoritzS

MoritzS Feb 19, 2016

Contributor

Maybe test it in tearDownClass() then? That method is executed after all tests.

@MoritzS

MoritzS Feb 19, 2016

Contributor

Maybe test it in tearDownClass() then? That method is executed after all tests.

This comment has been minimized.

@romgar

romgar Feb 19, 2016

Contributor

Not really convinced. The code I have updated is run after each test, not at the end of all the tests of the class.

I get the idea that this is nicer to subclass tearDownClass(), but I have the feeling that's better to stick more closely to the part I'm testing.

@romgar

romgar Feb 19, 2016

Contributor

Not really convinced. The code I have updated is run after each test, not at the end of all the tests of the class.

I get the idea that this is nicer to subclass tearDownClass(), but I have the feeling that's better to stick more closely to the part I'm testing.

This comment has been minimized.

@MoritzS

MoritzS Feb 19, 2016

Contributor

I mean you are running only one test anyway, so "running after each test" basically means "running once".

@MoritzS

MoritzS Feb 19, 2016

Contributor

I mean you are running only one test anyway, so "running after each test" basically means "running once".

This comment has been minimized.

@romgar

romgar Feb 20, 2016

Contributor

In this example yes. But my PR is updating some code that is related to a post-test cleaning, not a post-class cleaning, so I test my post-test cleaning.

In this example, I agree that the 2 of them are close. By testing with _post_teardown, I also ensure that all the tests, if people are using TransactionTestCase with several tests, will have the proper data. By testing with tearDownClass, I just test that my database is in a correct state after a whole suite of tests included in a TransactionTestCase.

This is a bit disturbing because I am using a TransactionTestCase to test a functionality of the inner logic of TransactionTestCase.

@romgar

romgar Feb 20, 2016

Contributor

In this example yes. But my PR is updating some code that is related to a post-test cleaning, not a post-class cleaning, so I test my post-test cleaning.

In this example, I agree that the 2 of them are close. By testing with _post_teardown, I also ensure that all the tests, if people are using TransactionTestCase with several tests, will have the proper data. By testing with tearDownClass, I just test that my database is in a correct state after a whole suite of tests included in a TransactionTestCase.

This is a bit disturbing because I am using a TransactionTestCase to test a functionality of the inner logic of TransactionTestCase.

@romgar

This comment has been minimized.

Show comment
Hide comment
@romgar

romgar Feb 19, 2016

Contributor

@MoritzS About your proposition for TestSerializedRollbackInhibitsPostMigrate, that's what I have initially done, but I had some tests failing because they were trying to load from _test_serialized_contents some data from django.contrib.sites.

That's why I finished by isolating these data to not be dependent from other tests.

Contributor

romgar commented Feb 19, 2016

@MoritzS About your proposition for TestSerializedRollbackInhibitsPostMigrate, that's what I have initially done, but I had some tests failing because they were trying to load from _test_serialized_contents some data from django.contrib.sites.

That's why I finished by isolating these data to not be dependent from other tests.

@MoritzS

This comment has been minimized.

Show comment
Hide comment
@MoritzS

MoritzS Feb 19, 2016

Contributor

Yeah, I got the same errors, but they only occurred because available_apps was set to None in setUp() instead of in the class definition. For me all tests pass with my proposed test case.

Contributor

MoritzS commented Feb 19, 2016

Yeah, I got the same errors, but they only occurred because available_apps was set to None in setUp() instead of in the class definition. For me all tests pass with my proposed test case.

@romgar

This comment has been minimized.

Show comment
Hide comment
@romgar

romgar Feb 20, 2016

Contributor

@MoritzS I think it's better to isolate the data that were provoking this problem because of test dependencies than updating the inner logic of another test than mine.

Contributor

romgar commented Feb 20, 2016

@MoritzS I think it's better to isolate the data that were provoking this problem because of test dependencies than updating the inner logic of another test than mine.

@MoritzS

This comment has been minimized.

Show comment
Hide comment
@MoritzS

MoritzS Feb 20, 2016

Contributor

I'm not sure what you mean. The logic of the other test is exactly the same as it was before, my changes merely were a "fix" that in turn allow your changes to the TestCase to not fail on the old (buggy) test.

Contributor

MoritzS commented Feb 20, 2016

I'm not sure what you mean. The logic of the other test is exactly the same as it was before, my changes merely were a "fix" that in turn allow your changes to the TestCase to not fail on the old (buggy) test.

@romgar

This comment has been minimized.

Show comment
Hide comment
@romgar

romgar Feb 20, 2016

Contributor

@MoritzS I tried your suggestion. Some tests are failing (http://djangoci.com/job/pull-requests-trusty/5588/). I have spent a lot of time on this MR to make the tests pass, and I realized that there were always some side effects somewhere else.

That's why I decided at the end to not be dependent to any other test and decided to also mock _test_serialized_content in TestSerializedRollbackInhibitsPostMigrate

Contributor

romgar commented Feb 20, 2016

@MoritzS I tried your suggestion. Some tests are failing (http://djangoci.com/job/pull-requests-trusty/5588/). I have spent a lot of time on this MR to make the tests pass, and I realized that there were always some side effects somewhere else.

That's why I decided at the end to not be dependent to any other test and decided to also mock _test_serialized_content in TestSerializedRollbackInhibitsPostMigrate

@romgar

This comment has been minimized.

Show comment
Hide comment
@romgar

romgar Mar 2, 2016

Contributor

@MoritzS Any other feedback ?

Contributor

romgar commented Mar 2, 2016

@MoritzS Any other feedback ?

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Mar 10, 2016

Member

Don't we need to update the Rollback emulation documentation? Also, a mention in the release notes is needed -- I guess in the "Backwards-incompatible changes" section.

Member

timgraham commented Mar 10, 2016

Don't we need to update the Rollback emulation documentation? Also, a mention in the release notes is needed -- I guess in the "Backwards-incompatible changes" section.

django/test/testcases.py
@@ -928,6 +917,16 @@ def _fixture_teardown(self):
allow_cascade=self.available_apps is not None,
inhibit_post_migrate=inhibit_post_migrate)
+ # If there was some initial data from migrated apps, we should restore them for next tests.

This comment has been minimized.

@timgraham

timgraham Mar 10, 2016

Member

", restore them for the next test." (chop "we", add "the") -- wrap comment at 79 characters.

@timgraham

timgraham Mar 10, 2016

Member

", restore them for the next test." (chop "we", add "the") -- wrap comment at 79 characters.

+
+class TestDataConsistencyOnTearDown(TestSerializedContentMockMixin, TransactionTestCase):
+ """
+ TransactionTestCase._fixture_teardown() is flushing the whole database.

This comment has been minimized.

@timgraham

timgraham Mar 10, 2016

Member

simply this by just stating the expected behavior:

"""
Initial data should be recreated in TransactionTestCase._fixture_teardown()
after the database is flushed so it's available in all tests.
"""
@timgraham

timgraham Mar 10, 2016

Member

simply this by just stating the expected behavior:

"""
Initial data should be recreated in TransactionTestCase._fixture_teardown()
after the database is flushed so it's available in all tests.
"""
@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Mar 10, 2016

Member

I agree the test isn't the most elegant but I don't have an alternative right now. Moritz, if you'd like to try to craft an alternative test, I'll happily compare the two approaches.

Member

timgraham commented Mar 10, 2016

I agree the test isn't the most elegant but I don't have an alternative right now. Moritz, if you'd like to try to craft an alternative test, I'll happily compare the two approaches.

+
+ def _post_teardown(self):
+ super(TestDataConsistencyOnTearDown, self)._post_teardown()
+ self.assertTrue(Car.objects.exists())

This comment has been minimized.

@timgraham

timgraham Mar 10, 2016

Member

Isn't the Car left in the database after this test? It should be deleted here too, I think.

@timgraham

timgraham Mar 10, 2016

Member

Isn't the Car left in the database after this test? It should be deleted here too, I think.

This comment has been minimized.

@romgar

romgar Mar 10, 2016

Contributor

The initial_data_migration mock represents which data have been serialised by Django and coming from initial data migration(s).

I want to emulate this Car object to be created from these initial data migrations.

It should still exists at the end of the TransactionTestCase, that's the idea behind this MR. If another test after this one needs this initial data migration, it will still be there. If there is no other test after this one, then the database will be in the same state as after the initial data migration(s) (specially important if we use --keepdb option).

I have tried to summarise the current situation: http://5minutes.youkidea.com/transactiontestcase-keepdb-django-issues.html

@romgar

romgar Mar 10, 2016

Contributor

The initial_data_migration mock represents which data have been serialised by Django and coming from initial data migration(s).

I want to emulate this Car object to be created from these initial data migrations.

It should still exists at the end of the TransactionTestCase, that's the idea behind this MR. If another test after this one needs this initial data migration, it will still be there. If there is no other test after this one, then the database will be in the same state as after the initial data migration(s) (specially important if we use --keepdb option).

I have tried to summarise the current situation: http://5minutes.youkidea.com/transactiontestcase-keepdb-django-issues.html

This comment has been minimized.

@timgraham

timgraham Mar 10, 2016

Member

What I meant is, to leave the database in a clean state for other tests, we need:

self.assertTrue(Car.objects.exists())
Car.objects.delete()
@timgraham

timgraham Mar 10, 2016

Member

What I meant is, to leave the database in a clean state for other tests, we need:

self.assertTrue(Car.objects.exists())
Car.objects.delete()

This comment has been minimized.

@romgar

romgar Mar 11, 2016

Contributor

The problem is what are we defining as clean state. For me, the clean state is the state where the database is after the initial data migration(s) and before the test suite runs.

When you apply initial migrations, they can for example initialise let's say some permissions.

You then want each test to be launched with these initial data migrations. The permissions created initially should be in each test, which means that when we flush the db on TransactionTestCase, we have to get our initial permissions back, so that the next TransactionTestCase will run with these permissions.

And more than that, with --keepdb option, you need your test database to still contain these permissions (they won't be initialised anymore because your db has kept django_migrations history and won't recreate initial data migrations), or your tests will probably fail next time you run them.

@romgar

romgar Mar 11, 2016

Contributor

The problem is what are we defining as clean state. For me, the clean state is the state where the database is after the initial data migration(s) and before the test suite runs.

When you apply initial migrations, they can for example initialise let's say some permissions.

You then want each test to be launched with these initial data migrations. The permissions created initially should be in each test, which means that when we flush the db on TransactionTestCase, we have to get our initial permissions back, so that the next TransactionTestCase will run with these permissions.

And more than that, with --keepdb option, you need your test database to still contain these permissions (they won't be initialised anymore because your db has kept django_migrations history and won't recreate initial data migrations), or your tests will probably fail next time you run them.

This comment has been minimized.

@timgraham

timgraham Mar 11, 2016

Member

I understand all that, but I don't see any need for the car instance created by this test case to persistent for other tests? What's the problem with deleting that object as I proposed? Without deleting the object, adding this test will pass:

class TestE(TransactionTestCase):
    available_apps = ['test_utils']

    def test(self):
        self.assertTrue(Car.objects.exists())

which I think is unexpected.

@timgraham

timgraham Mar 11, 2016

Member

I understand all that, but I don't see any need for the car instance created by this test case to persistent for other tests? What's the problem with deleting that object as I proposed? Without deleting the object, adding this test will pass:

class TestE(TransactionTestCase):
    available_apps = ['test_utils']

    def test(self):
        self.assertTrue(Car.objects.exists())

which I think is unexpected.

This comment has been minimized.

@romgar

romgar Mar 11, 2016

Contributor

I get it. Definitely. Will update it.

@romgar

romgar Mar 11, 2016

Contributor

I get it. Definitely. Will update it.

This comment has been minimized.

@romgar

romgar Mar 12, 2016

Contributor

I think this object deletion should live in TestSerializedContentMockMixin, as this class is related to serialized object creation. If we don't clean in that Mixin, it means that every test inheriting from that class will have to clean created data themselves.

@romgar

romgar Mar 12, 2016

Contributor

I think this object deletion should live in TestSerializedContentMockMixin, as this class is related to serialized object creation. If we don't clean in that Mixin, it means that every test inheriting from that class will have to clean created data themselves.

@timgraham timgraham changed the title from Fixed #25251 -- Initial data migration lost after TransactionTestCase to Fixed #25251 -- Made data migrations availabe in TransactionTestCase. Mar 10, 2016

@romgar romgar changed the title from Fixed #25251 -- Made data migrations availabe in TransactionTestCase. to Fixed #25251 -- Made data migrations available in TransactionTestCase. Mar 10, 2016

@romgar

This comment has been minimized.

Show comment
Hide comment
@romgar

romgar Mar 11, 2016

Contributor

@timgraham I will write some details in rollback emulation section, and think about what we can say on any backward-incompatible change.

Contributor

romgar commented Mar 11, 2016

@timgraham I will write some details in rollback emulation section, and think about what we can say on any backward-incompatible change.

@MoritzS

This comment has been minimized.

Show comment
Hide comment
@MoritzS

MoritzS Mar 11, 2016

Contributor

I tried to write the test as I suggested it, however now I get what @romgar meant. To test this the migrations have to run before the test case even gets touched, so @override_settings(MIGRATION_MODULES={...}) doesn't work. Also you somehow have to emulate --keepdb.

I'm now convinced that monkey patching the internals of the connection is the best thing to do here.

Contributor

MoritzS commented Mar 11, 2016

I tried to write the test as I suggested it, however now I get what @romgar meant. To test this the migrations have to run before the test case even gets touched, so @override_settings(MIGRATION_MODULES={...}) doesn't work. Also you somehow have to emulate --keepdb.

I'm now convinced that monkey patching the internals of the connection is the best thing to do here.

@@ -259,6 +259,13 @@ To prevent serialized data from being loaded twice, setting
:data:`~django.db.models.signals.post_migrate` signal when flushing the test
database.
+.. versionchanged:: 1.10

This comment has been minimized.

@timgraham

timgraham Mar 14, 2016

Member

Doesn't the first sentence of this section need to be revised, "Any initial data loaded in migrations will only be available in TestCase tests and not in TransactionTestCase tests"?

@timgraham

timgraham Mar 14, 2016

Member

Doesn't the first sentence of this section need to be revised, "Any initial data loaded in migrations will only be available in TestCase tests and not in TransactionTestCase tests"?

This comment has been minimized.

@romgar

romgar Mar 14, 2016

Contributor

I'm not sure, as this logic is still dependant on serialized_rollback parameter, as explained on second paragraph:

"Django can reload that data for you on a per-testcase basis by setting the serialized_rollback option to True in the body of the TestCase or TransactionTestCase"

Not really sure if it makes sense to use serialized_rollback with TestCase, as these tests are always launched before TransactionTestCase ones, and so the initial data migrations should not be impacted by TestCase tests, isn't it ?

Just wondering, a mix of TransactionTestCase tests with serialised_rollback to True and False won't guarantee the --keepdb option to work.

If the last test of the suite has no serialised_rollback option, then the database will be empty at the end. And as we cannot guarantee test ordering, it means that somebody who wants to reuse the database with --keepdb option should absolutely use serialized_rollback option to True on every TransactionTestCase he is defining...

@romgar

romgar Mar 14, 2016

Contributor

I'm not sure, as this logic is still dependant on serialized_rollback parameter, as explained on second paragraph:

"Django can reload that data for you on a per-testcase basis by setting the serialized_rollback option to True in the body of the TestCase or TransactionTestCase"

Not really sure if it makes sense to use serialized_rollback with TestCase, as these tests are always launched before TransactionTestCase ones, and so the initial data migrations should not be impacted by TestCase tests, isn't it ?

Just wondering, a mix of TransactionTestCase tests with serialised_rollback to True and False won't guarantee the --keepdb option to work.

If the last test of the suite has no serialised_rollback option, then the database will be empty at the end. And as we cannot guarantee test ordering, it means that somebody who wants to reuse the database with --keepdb option should absolutely use serialized_rollback option to True on every TransactionTestCase he is defining...

+In older versions, these data were loaded at the beginning of the test, but
+this behavior was preventing ``--keepdb`` option to work properly (the
+database was empty at the end of the whole test suite).
+It shouldn't have any impact on your test suite if you have not customized

This comment has been minimized.

@timgraham

timgraham Mar 14, 2016

Member

Doesn't it make migration data available in all TransactionTestCases (in older versions, it was only available in the first one, I believe)?

@timgraham

timgraham Mar 14, 2016

Member

Doesn't it make migration data available in all TransactionTestCases (in older versions, it was only available in the first one, I believe)?

This comment has been minimized.

@romgar

romgar Mar 14, 2016

Contributor

I think that my explanation is partially wrong. It won't have any impact on test suites if people have defined all their TransactionTestCase with serialised_rollback option to True.

If somebody is using serialized_rollback in just one test, he is expecting to have the initial data loaded at the beginning of the test, not the end. And it makes hard to predict: by loading the data at the end of the test, we know that we prepare a clean environment for the next test, which is unknown by the developer. It then makes sense only if all the TransactionTestCase have the same serialized_rollback logic.

@romgar

romgar Mar 14, 2016

Contributor

I think that my explanation is partially wrong. It won't have any impact on test suites if people have defined all their TransactionTestCase with serialised_rollback option to True.

If somebody is using serialized_rollback in just one test, he is expecting to have the initial data loaded at the beginning of the test, not the end. And it makes hard to predict: by loading the data at the end of the test, we know that we prepare a clean environment for the next test, which is unknown by the developer. It then makes sense only if all the TransactionTestCase have the same serialized_rollback logic.

This comment has been minimized.

@romgar

romgar Mar 14, 2016

Contributor

Maybe another way to fix it is not to impact TransactionTestCase class, but TRUNCATE and load initial data migrations at the end of the test suite run, if --keepdb option has been activated.

@romgar

romgar Mar 14, 2016

Contributor

Maybe another way to fix it is not to impact TransactionTestCase class, but TRUNCATE and load initial data migrations at the end of the test suite run, if --keepdb option has been activated.

This comment has been minimized.

@timgraham

timgraham Mar 14, 2016

Member

I'm not sure it's feasible but you could try. It's possible might have to document the limitation.

@timgraham

timgraham Mar 14, 2016

Member

I'm not sure it's feasible but you could try. It's possible might have to document the limitation.

This comment has been minimized.

@timgraham

timgraham Mar 14, 2016

Member

If it's possible to salvage the test changes you've made here to write a regression test for incorrect behavior the current patch introduces, that would be great. I think serialized_rollback doesn't really have any tests right now.

@timgraham

timgraham Mar 14, 2016

Member

If it's possible to salvage the test changes you've made here to write a regression test for incorrect behavior the current patch introduces, that would be great. I think serialized_rollback doesn't really have any tests right now.

This comment has been minimized.

@romgar

romgar Mar 14, 2016

Contributor

@timgraham I would be happy to add some tests for serialized_rollback option. These added tests should probably live in another MR, right ?

@romgar

romgar Mar 14, 2016

Contributor

@timgraham I would be happy to add some tests for serialized_rollback option. These added tests should probably live in another MR, right ?

This comment has been minimized.

@timgraham

timgraham Mar 14, 2016

Member

Yes

This comment has been minimized.

@romgar

romgar Mar 14, 2016

Contributor

Ok, I will create it.

Some feelings about the loading before and after ?

It could also be a new parameter in TransactionTestCase, like serialized_rollback_after (needs a better naming) to be used in --keepdb configuration, so that it's loading twice only when people want to use --keepdb.

@romgar

romgar Mar 14, 2016

Contributor

Ok, I will create it.

Some feelings about the loading before and after ?

It could also be a new parameter in TransactionTestCase, like serialized_rollback_after (needs a better naming) to be used in --keepdb configuration, so that it's loading twice only when people want to use --keepdb.

This comment has been minimized.

@romgar

romgar Mar 14, 2016

Contributor

There are already some tests in migration_test_data_persistance.tests.

@romgar

romgar Mar 14, 2016

Contributor

There are already some tests in migration_test_data_persistance.tests.

This comment has been minimized.

@timgraham

timgraham Mar 15, 2016

Member

Loading afterward seems redundant -- as you noted, this won't work if there are any TransactionTestCases without serialized_rollback=True. It might be possible to take care of the data loading in destroy_test_db() (if keepdb=True).

@timgraham

timgraham Mar 15, 2016

Member

Loading afterward seems redundant -- as you noted, this won't work if there are any TransactionTestCases without serialized_rollback=True. It might be possible to take care of the data loading in destroy_test_db() (if keepdb=True).

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 a TransactionTestCase, to be sure that the next test will be in the expected environment (data loaded from initial migrations)
@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Mar 17, 2016

Member

Closing in favor of #6297.

Member

timgraham commented Mar 17, 2016

Closing in favor of #6297.

@timgraham timgraham closed this Mar 17, 2016

@romgar

This comment has been minimized.

Show comment
Hide comment
@romgar

romgar Nov 4, 2016

Contributor

@aaugustin The MR we were discussing today, and the proposition I initially made to change the way TransactionTestCase was flushing/reinitialising the db.

@MarkusH The MR we can have a look tomorrow during the sprint.

Contributor

romgar commented Nov 4, 2016

@aaugustin The MR we were discussing today, and the proposition I initially made to change the way TransactionTestCase was flushing/reinitialising the db.

@MarkusH The MR we can have a look tomorrow during the sprint.

@timgraham timgraham reopened this Nov 5, 2016

@aaugustin

This comment has been minimized.

Show comment
Hide comment
@aaugustin

aaugustin Nov 5, 2016

Member

A few comments:

  • I'm strongly convinced that the correct approach is to restore serialized data just after flush. The reason why Django needs to restore it is because Django flushed it. So that's the correct point to do it. Restoring it only at the end of tests when --keepdb is enabled seems wrong. (It's fairly obvious to me, happy to elaborate if someone isn't convinced.)
  • I reviewed the documentation and it seems that this change falls within the currently described semantics. So it isn't backwards-incompatible (but is worth mentioning in the release notes as people often have fragile code around test setup and data loading).

I talked with @romgar and here's the remaining problem as far as I understand it. Django currently allows each TransactionTestCase to declare whether they need the initial data to be restored with the serialized_rollback attribute. If we're moving the restore of the initial data at the end of the previous TransactionTestCase, then that test case needs to know about the value of serialized_rollback in the following test case, and the last TransactionTestCase always needs to restore data.

Otherwise, if the next run uses --keepdb the initial data will go missing for the entire execution. (This is where the ticket started.)

In the test runner we have this line:

suite = reorder_suite(suite, self.reorder_by, self.reverse)

At this point we know the sequence of TestCase and TransactionTestCase. I think we should loop on all test cases here and set testcase._next_serialized_rollback = next_testcase.serialized_rollback as well as last_testcase._next_serialized_rollback = True. (Well hopefully you get the idea.)

Then you'd use that flag to restore data after flush.

Member

aaugustin commented Nov 5, 2016

A few comments:

  • I'm strongly convinced that the correct approach is to restore serialized data just after flush. The reason why Django needs to restore it is because Django flushed it. So that's the correct point to do it. Restoring it only at the end of tests when --keepdb is enabled seems wrong. (It's fairly obvious to me, happy to elaborate if someone isn't convinced.)
  • I reviewed the documentation and it seems that this change falls within the currently described semantics. So it isn't backwards-incompatible (but is worth mentioning in the release notes as people often have fragile code around test setup and data loading).

I talked with @romgar and here's the remaining problem as far as I understand it. Django currently allows each TransactionTestCase to declare whether they need the initial data to be restored with the serialized_rollback attribute. If we're moving the restore of the initial data at the end of the previous TransactionTestCase, then that test case needs to know about the value of serialized_rollback in the following test case, and the last TransactionTestCase always needs to restore data.

Otherwise, if the next run uses --keepdb the initial data will go missing for the entire execution. (This is where the ticket started.)

In the test runner we have this line:

suite = reorder_suite(suite, self.reorder_by, self.reverse)

At this point we know the sequence of TestCase and TransactionTestCase. I think we should loop on all test cases here and set testcase._next_serialized_rollback = next_testcase.serialized_rollback as well as last_testcase._next_serialized_rollback = True. (Well hopefully you get the idea.)

Then you'd use that flag to restore data after flush.

@aaugustin

This comment has been minimized.

Show comment
Hide comment
@aaugustin

aaugustin Nov 5, 2016

Member

This isn't the most elegant approach but it's the best you can do to preserve the current serialized_rollback API. The situation is fundamentally messy; IMO serialized_rollback expresses the only behavior that makes sense here but it can't be implemented within the TransactionTestCase that declares it.

Member

aaugustin commented Nov 5, 2016

This isn't the most elegant approach but it's the best you can do to preserve the current serialized_rollback API. The situation is fundamentally messy; IMO serialized_rollback expresses the only behavior that makes sense here but it can't be implemented within the TransactionTestCase that declares it.

@aaugustin

This comment has been minimized.

Show comment
Hide comment
@aaugustin

aaugustin Nov 5, 2016

Member

With the current implementation (leaving --keepdb aside for a minute):

  • the first TransactionTestCase always gets a database populated with the initial data — and will reload this data if serialized_rollback = True for no good reason, which suggest this isn't the right point to do it
  • further TransactionTestCases get an empty database if serialized_rollback = False and a database populated with the initial data if serialized_rollback = True. However, when they're run in isolation, we're back to the "first" case, and these test cases get a populated database. So depending on an empty database is an error.

So there's an invariant: an TransactionTestCase gets either an empty database or a database populate with the initial date. I don't think it's reasonable to depend on this invariant and that's why I don't mind that the solution I recommend above breaks it. Since the database can be empty you can't rely on what's in there. You might rely on the fact that there isn't more than a given number of objects (your initial data) but that's super fragile -- what if you add more initial data?

For this reason, I think documenting the change is sufficient. After the change, TransactionTestCase gets exactly the initial data if serialized_rollback is True, like it currently does, and whatever the previous test case left behind if serialized_rollback is False (instead of either an empty database or the initial data).

Member

aaugustin commented Nov 5, 2016

With the current implementation (leaving --keepdb aside for a minute):

  • the first TransactionTestCase always gets a database populated with the initial data — and will reload this data if serialized_rollback = True for no good reason, which suggest this isn't the right point to do it
  • further TransactionTestCases get an empty database if serialized_rollback = False and a database populated with the initial data if serialized_rollback = True. However, when they're run in isolation, we're back to the "first" case, and these test cases get a populated database. So depending on an empty database is an error.

So there's an invariant: an TransactionTestCase gets either an empty database or a database populate with the initial date. I don't think it's reasonable to depend on this invariant and that's why I don't mind that the solution I recommend above breaks it. Since the database can be empty you can't rely on what's in there. You might rely on the fact that there isn't more than a given number of objects (your initial data) but that's super fragile -- what if you add more initial data?

For this reason, I think documenting the change is sufficient. After the change, TransactionTestCase gets exactly the initial data if serialized_rollback is True, like it currently does, and whatever the previous test case left behind if serialized_rollback is False (instead of either an empty database or the initial data).

@aaugustin

This comment has been minimized.

Show comment
Hide comment
@aaugustin

aaugustin Nov 5, 2016

Member

It's unclear to me that we can write meaningful tests for this behavior of the test runner.

Even if we write TransactionTestCase in the Django test suite, they'll depend on being executed in order and will fail in isolation, which doesn't seem acceptable to me.

Member

aaugustin commented Nov 5, 2016

It's unclear to me that we can write meaningful tests for this behavior of the test runner.

Even if we write TransactionTestCase in the Django test suite, they'll depend on being executed in order and will fail in isolation, which doesn't seem acceptable to me.

@aaugustin

This comment has been minimized.

Show comment
Hide comment
@aaugustin

aaugustin Nov 5, 2016

Member

Pro tip: for previous, next in zip(testcases[:-1], testcases[1:])

Member

aaugustin commented Nov 5, 2016

Pro tip: for previous, next in zip(testcases[:-1], testcases[1:])

@romgar

This comment has been minimized.

Show comment
Hide comment
@romgar

romgar Nov 11, 2016

Contributor

Closing this on in favor of #7528

Contributor

romgar commented Nov 11, 2016

Closing this on in favor of #7528

@romgar romgar closed this Nov 11, 2016

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