wemake-services / django-test-migrations

Test django schema and data migrations, including migrations' order and best practices.
https://pypi.org/project/django-test-migrations/
MIT License
527 stars 30 forks source link

truncate_plan should truncate each app separately #398

Open jrobichaud opened 1 year ago

jrobichaud commented 1 year ago

Context

I have a project with multiple apps (Ex: App A and app B).

I once in a while code migrations with tests based on the state of the DB at that time.

Lately I added a new nullable field on my Old model in app A, a migration to fill the field then make it non-nullable. Everything was working properly except one thing, an old migration test started to fail.

The old migration tests for app B started to fail because back then that field did not exist but my app A was being fully migrated.

It did not make any sense until I realized truncate_plan only truncate the end of the migrations plan and not the apps separately.

Explanations

Lets say I have migrations in each apps that were added in that chronological order:

A1
B1
A2
B2
A3
B3 (has explicit dependency to A3 here)
A4
B4
A5
B5 (this is a custom migration)
B6
A6 (Add new nullable field here)
A7 (Migration to fill the field in the DB)
A8 (Migration to make field non-nullable)

The migration test is specified like this:

migration_from = [
    ("A", "4"),
    ("B", "4"),
]
migrate_to = [
    ("B", "5"),
]

However in practice when the test is ran the full plan looks like this:

A1
A2
A3
A4
A5
A6 (Add new nullable field here)
A7 (Migration to fill the field in the DB)
A8 (Migration to make field non-nullable)
B1
B2
B3 (has explicit dependency to A3 here)
B4
B5 (this is a custom migration)
B6

The truncate plan A4 B4 will result to start dropping everything after index of B4 with the current implementation:

A1
A2
A3
A4
A5
A6 (Add new nullable field here)
A7 (Migration to fill the field in the DB)
A8 (Migration to make field non-nullable)
B1
B2
B3 (has explicit dependency to A3 here)
B4
# B5 (this is a custom migration) <-  REMOVAL FROM PLAN BEGINS AT THIS INDEX
# B6

However I would expect truncate_plan A4 B4 to truncate per app independently like this:

A1
A2
A3
A4
# A5  <- REMOVED BECAUSE IT IS ABOVE A4
# A6 (Add new nullable field here) <- REMOVED FROM PLAN BECAUSE IT IS ABOVE A4
# A7 (Migration to fill the field in the DB) <- REMOVED FROM PLAN BECAUSE IT IS ABOVE A4
# A8 (Migration to make field non-nullable) <- REMOVED FROM PLAN BECAUSE IT IS ABOVE A4
B1
B2
B3 (has explicit dependency to A3 here)
B4
# B5 (this is a custom migration) <- REMOVED FROM PLAN BECAUSE IT IS ABOVE B4
# B6 <- REMOVED FROM PLAN BECAUSE IT IS ABOVE B4
skarzi commented 10 months ago

hi @jrobichaud 👋

That's an interesting idea. Could you please prepare the simplest possible example - the simplest Django project reflecting migrations from your message, so we can play with it and craft the best possible solution to handle all (or most 😄) cases?

jrobichaud commented 10 months ago

@skarzi, there you go: https://github.com/jrobichaud/django-test-migrations-398

I made sure to recreate the exact migrations with the exact numbers.

See the readme for the explanations.

jrobichaud commented 10 months ago

I just added CI execution, we can see in the develop branch the migration test fails.

main: https://github.com/jrobichaud/django-test-migrations-398/actions/runs/7305056864/job/19908143449#step:5:17 develop: https://github.com/jrobichaud/django-test-migrations-398/actions/runs/7305060018/job/19908150801#step:5:34