-
-
Notifications
You must be signed in to change notification settings - Fork 30
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
properly reset databases on tests teardown #38
Comments
I've investigated and
So in these cases calling We can refactor a bit and leave What's your opinion? |
Sounds good! |
I face an issue linked to this issue. The change proposed breaks my tests when using Django unittest native runner with keepdb and parallel: If I run the three commands in a row:
At the last run I get errors linked to DB schema not being migrated back to the last migrations. I tried to understand why the situation is different between normal and parallel test runner, but couldn't find a reasonable explanation. I tried using the new The issue is solved by adding the following
Any idea for an explanation or a better fix? |
Hello ;) It would be great if you can share some minimal version of code that causes this issue.
I think the issue may be caused by the way But to confirm my thesis and provide fix, firstly please share above requested information |
Thanks for your feedback. I'll work on trying to get a simple failing test to work on. To answer your questions:
|
Thank you for posting more details.
3rd option is my favorite one - it is the closest to real life It's really catchy topic, so if something is not clear for you, let me know and I will try to explain it better. @sobolevn what's your thoughts on that? |
Thank you @skarzi for looking into the issue. I have been able to narrow down the issue. It actually affects the tests ran on Postgres, but not on SQLite. I managed to make a simple failing app. I'll send it to you tomorrow. |
I just uploaded the failing app in the branch below. To reproduce the error, run in an environment with Postgres and django_test_migrations master the following commands:
As mentioned before, adding the following tearDown method is a fix to the issue:
Below is the full output of the last command:
|
Thank you @chel-ou for uploading failing app ;) So this error is related to the fact how I have also partially implemented (partially, because So now I think we should do one of following:
#46 already contains implementation of point 3 What's your opinion guys? |
I don't really have an opinion on this one, because it is quite a hard topic to follow. |
I also do not have a clear opinion on the topic. I have some thoughts though. One thing that troubles me that is this bug only affects Postgres, and not SQLite. So the issue might not be at the SQL commands level, but more linked to the intricacies of implementation of Postgres. Regarding your point 1., I am not sure I understand the philosophical problem with applying migrate after the test is completed. Migrate is a built-in command in Django, enabling us to make sure that the DB is restored to an up to date state. It may not be the most efficient method but at least we ensure compatibility with future versions of Django on this specific topic. One (not very clean) option could be to have a different behavior in the unittest case if Regarding your points 2. and 3. I am not sure how it would solve the issue. My feeling is that it would create too much complexity compared to the issue. |
I don't think it's related to DB backend.
Data migrations or any complicated migrations are fragile operations, because they are ran on production database, so if anything go wrong then really bad things will happen. |
Hi @skarzi thanks for investing much time into this, this is indeed a tough issue to solve. I was thinking about this a bit more, and it seems that, typically, the reason data migrations fail to be re-applied in teardown is due to "primary key sequence" mismatch. Note that I tried testing #46 locally and I'm having issues with dependency migrations not being applied (while they are being applied in current 0.2.0 ). I was not able to overcome this by specifying all the related apps migrations (I gave up mid-way), but even if I did it would not be ideal. How come this is not the same behavior as on 0.2.0, I thought #46 only fixes the teardown? |
@asfaltboy awesome idea! Thanks! |
Hello @asfaltboy,
Do you mean "primary key sequence" mismatch of all models' tables or just
Unfortunately, it's not possible to reset sequences of
I will try to reproduce this error and investigate it more deeply on the weekend.
Yes, definitely it won't be a solution for this problem, it should work as previously.
This PR fixes teardown (I have introduced regression when removed Probably I have used misleading words in PR's title/description and also this issue and solution for it have evolved a lot during all discussions. Firstly I thought it's related only to test teardown and I wanted to keep all this code in |
@asfaltboy I have digged into Django's migrations deeper and had some answers for your problem, but also many new concerns that I'd like to discuss.
This issue is related to how Django built migrations' plan. (NOTE: in both cases, before running tests, django calls
Flow implemented in #46 looks like below:
Let's imagine we have 2 apps:
Then let's imagine we are testing migration 0010, so In The problem is more trickier than I thought :( Yesterday I was experimenting with creating migrations plan manually starting from one of:
and then truncating such plan on |
Hey @skarzi and @sobolevn thanks a lot for the super thorough and clean implementation in #76 , good work! I didn't get a chance to test it, I'll be sure to try it out on the next migration test we add. Site node: our "migration tests" seem to be short lived. That is, as time goes by, the dependencies on moving parts grow to become a hassle to maintain. And, when such a migration has been applied in production, (and a few other migrations followed it). So, we end up removing the test, and eventually flatten/remove migrations. |
This What do you think about creating some |
how about something like:
I'm not sure what's the best practices? Retain the tests only as long as you need them? Continuously clear old migrations (is that a Django best practice)? I guess common sense is to only keep tests around as long as they have a use... |
Currently multiple databases are supported by creating instance of
Migrator
for each database, but only default database is cleaned at the end of test - https://github.com/wemake-services/django-test-migrations/blob/master/django_test_migrations/migrator.py#L77Also using
migrate
management command is not the best idea when it comes to performance, because there is no need to apply all unapplied migrations just before the next test, which should start with clean state.Django's
TestCase
usesflush
management command to clean databases after test.We need to reuse Django's code responsible for cleaning database on test teardown (or establish our own way to do that) and fix
Migrator.reset()
method to cleanMigrator.database
instead of default one.The text was updated successfully, but these errors were encountered: