-
-
Notifications
You must be signed in to change notification settings - Fork 31.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed #25251 -- Made data migrations available in TransactionTestCase. #6137
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't the first sentence of this section need to be revised, "Any initial data loaded in migrations will only be available in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure, as this logic is still dependant on "Django can reload that data for you on a per-testcase basis by setting the Not really sure if it makes sense to use Just wondering, a mix of If the last test of the suite has no |
||
|
||
Data are now loaded at the end of a ``TransactionTestCase``, after | ||
the database flush (it was previously loaded at the beginning of the test). | ||
This change makes ``--keepdb`` option work properly if your test suite | ||
contains ``TransactionTestCase`` tests. | ||
|
||
Other test conditions | ||
--------------------- | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't it make migration data available in all TransactionTestCases (in older versions, it was only available in the first one, I believe)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that my explanation is partially wrong. It won't have any impact on test suites if people have defined all their
TransactionTestCase
withserialised_rollback
option toTrue
.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 theTransactionTestCase
have the sameserialized_rollback
logic.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe another way to fix it is not to impact
TransactionTestCase
class, butTRUNCATE
and load initial data migrations at the end of the test suite run, if--keepdb
option has been activated.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it's feasible but you could try. It's possible might have to document the limitation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@timgraham I would be happy to add some tests for
serialized_rollback
option. These added tests should probably live in another MR, right ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I will create it.
Some feelings about the loading before and after ?
It could also be a new parameter in
TransactionTestCase
, likeserialized_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
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are already some tests in
migration_test_data_persistance.tests
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loading afterward seems redundant -- as you noted, this won't work if there are any
TransactionTestCases
withoutserialized_rollback=True
. It might be possible to take care of the data loading indestroy_test_db()
(ifkeepdb=True
).