fix and refactor migrations test teardown and setup #76

skarzi · 2020-05-09T18:59:31Z

Closes #38
Closes #40

This commit implements new migrations test teardown and setup.
Teardown simply call migrate command, but on already tested database.
Setup is more complicated but more similar to real case of running migrations on production (I described it pros in #38) :

drop all models tables
flush (make empty) django_migrations table
create as broad as possible plan including all migrate_from targets
apply all migrations (only forward - so like in production environments and avoid any potential side effects implemented in backward migrations)

This part related to custom plan creation is necessary to make API developers' friendly just to not force them to include migrations targets for all models they want to use in test.
Generated plan cause also .after() applies migrations more like its done on production (only these migrations that aren't applied yet will be applied - on production all older migrations are already applied), because it is just truncated full (clean) migrations plan.

This reverts commit 55a5261.

Move ``_Migration`` and ``_MigrationSpec`` to newly introduced module and rename them to ``MigrationTarget`` and ``MigrationSpec``. Fix type hints of ``MigratorTestCase`` class attributes: ``migrate_from`` and ``migrate_to``.

codecov · 2020-05-09T19:01:27Z

Codecov Report

Merging #76 into master will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master       #76   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         9    +4     
  Lines          113       189   +76     
  Branches        13        19    +6     
=========================================
+ Hits           113       189   +76

Impacted Files	Coverage Δ
django_test_migrations/contrib/unittest_case.py	`100.00% <100.00%> (ø)`
django_test_migrations/exceptions.py	`100.00% <100.00%> (ø)`
django_test_migrations/logic/migrations.py	`100.00% <100.00%> (ø)`
django_test_migrations/migrator.py	`100.00% <100.00%> (ø)`
django_test_migrations/plan.py	`100.00% <100.00%> (ø)`
django_test_migrations/sql.py	`100.00% <100.00%> (ø)`
django_test_migrations/types.py	`100.00% <100.00%> (ø)`
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cbb8626...c3d0e6c. Read the comment docs.

sobolevn

Amazing work!

This is a very complicated feature, but a very simple and thoughtful implementation.
Really amazing 👍

However, there is some polishing to be done!

sobolevn · 2020-05-10T07:58:21Z

django_test_migrations/plan.py

+    targets: List[MigrationTarget],
+    plan: MigrationPlan,
+) -> MigrationPlan:
+    """Truncate migrations ``plan`` up to ``targets``."""


Can you please add some more docs on why do we need this function.
It is not clear from the source code (at least for me).

sobolevn · 2020-05-10T07:58:37Z

django_test_migrations/sql.py

+
+_Connection = Union[DefaultConnectionProxy, BaseDatabaseWrapper]
+
+DJANGO_MIGRATIONS_TABLE_NAME = 'django_migrations'


Should it be Final?

sobolevn · 2020-05-10T08:00:42Z

django_test_migrations/sql.py

+        include_views=False,
+    )
+    sql_drop_tables = [
+        connection.SchemaEditorClass.sql_delete_table % {


Is this operation safe in terms of sql-injection? Maybe there are safer methods to join sql vars?

Should we even care?

It should be safe, because sql_delete_table is string format defined by database backend (no user input) and its table argument is table's name (not any users input again), so it should be save.
This implementation is based on https://github.com/django/django/blob/c226c6cb3209122b6732fd501e2994c408dc258e/django/db/backends/base/schema.py#L342

sobolevn · 2020-05-10T08:01:12Z

django_test_migrations/sql.py

+    database_name: str,
+    style: Optional[Style] = None,
+) -> None:
+    """Flush `django_migrations` table."""


Let's also explain why do we handle it separately.

sobolevn · 2020-05-10T08:02:19Z

django_test_migrations/sql.py

+
+    This function is copy of ``connection.ops.execute_sql_flush``
+    method from Django's source code:
+    https://github.com/django/django/blob/227d0c7365cfd0a64d021cb9bdcf77bed2d3f170/django/db/backends/base/operations.py#L401


let's use bit.ly for this link.

sobolevn · 2020-05-10T08:02:56Z

django_test_migrations/sql.py

+    method from Django's source code:
+    https://github.com/django/django/blob/227d0c7365cfd0a64d021cb9bdcf77bed2d3f170/django/db/backends/base/operations.py#L401
+    to make `django-test-migrations` compatible with `Django==1.11`.
+    ``connection.ops.execute_sql_flush()`` was introduced in `Django==2.0`.


Django==2.0 should be in double backquotes.

Surely I will correct it.
To be honest I don't feel the difference between single backquote and double backquotes.
I would be really grateful if you could explain it ;)

sobolevn · 2020-05-10T08:03:42Z

tests/test_plan/conftest.py

+
+
+@pytest.fixture()
+def plan(mocker):


Why do we need a mocker here?

leftover from some "first attempts" code, thanks for catching!

sobolevn · 2020-05-10T08:04:34Z

tests/test_sql/test_drop_models_table.py

+
+
+def test_drop_models_table_table_detected(mocker):
+    """Ensure `DROP TABLE` statements are executed when any table detected."""


Should be in double backquotes.

sobolevn · 2020-05-10T08:04:38Z

tests/test_sql/test_drop_models_table.py

+
+
+def test_drop_models_table_no_tables_detected(mocker):
+    """Ensure any `DROP TABLE` statement executed when no tables detected."""


Should be in double backquotes.

Add more detailed docstrings. Fix backquotes usage in docstrings. Use bit.ly for long URLs.

sobolevn

Thanks a lot! Merging! 🎉

sobolevn · 2020-05-10T17:07:31Z

django_test_migrations/sql.py

+        include_views=False,
+    )
+    sql_drop_tables = [
+        connection.SchemaEditorClass.sql_delete_table % {


skarzi added 6 commits May 9, 2020 16:42

Revert "refactor database reset on test's teardown (#42)"

63ed305

This reverts commit 55a5261.

always reset database being tested instead of default

5812cef

add types modules for common typing aliases etc

994bfd4

Move ``_Migration`` and ``_MigrationSpec`` to newly introduced module and rename them to ``MigrationTarget`` and ``MigrationSpec``. Fix type hints of ``MigratorTestCase`` class attributes: ``migrate_from`` and ``migrate_to``.

show missing lines in coverage report

fd50fb5

extract migration target normalization to function

0210cb7

update migrations test setup to migrate only forward

276f1a9

skarzi requested a review from sobolevn May 9, 2020 18:59

skarzi self-assigned this May 9, 2020

skarzi changed the title ~~Issue 38 and 40~~ fix and refactor migrations test teardown and setup May 9, 2020

skarzi mentioned this pull request May 9, 2020

update database cleaning after migrations test #46

Closed

sobolevn requested changes May 10, 2020

View reviewed changes

apply code review suggestions

c3d0e6c

Add more detailed docstrings. Fix backquotes usage in docstrings. Use bit.ly for long URLs.

skarzi force-pushed the issue-38-and-40 branch from 42e20ee to c3d0e6c Compare May 10, 2020 10:10

sobolevn approved these changes May 10, 2020

View reviewed changes

django_test_migrations/sql.py

include_views=False,

)

sql_drop_tables = [

connection.SchemaEditorClass.sql_delete_table % {

Copy link

Member

sobolevn May 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

sobolevn merged commit d35f4b1 into master May 10, 2020

delete-merged-branch bot deleted the issue-38-and-40 branch May 10, 2020 17:08

sobolevn mentioned this pull request May 10, 2020

Correct post_migrate signal muting #69

Closed

asfaltboy mentioned this pull request May 10, 2020

properly reset databases on tests teardown #38

Closed

skarzi mentioned this pull request May 16, 2020

migrator fixture clashes with db fixture #82

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix and refactor migrations test teardown and setup #76

fix and refactor migrations test teardown and setup #76

skarzi commented May 9, 2020 •

edited

Loading

codecov bot commented May 9, 2020 •

edited

Loading

sobolevn left a comment

sobolevn May 10, 2020

sobolevn May 10, 2020

sobolevn May 10, 2020

skarzi May 10, 2020

sobolevn May 10, 2020

sobolevn May 10, 2020

sobolevn May 10, 2020

sobolevn May 10, 2020

skarzi May 10, 2020

sobolevn May 10, 2020

skarzi May 10, 2020

sobolevn May 10, 2020

sobolevn May 10, 2020

sobolevn left a comment

sobolevn May 10, 2020


		_Connection = Union[DefaultConnectionProxy, BaseDatabaseWrapper]

		DJANGO_MIGRATIONS_TABLE_NAME = 'django_migrations'



		def test_drop_models_table_table_detected(mocker):
		"""Ensure `DROP TABLE` statements are executed when any table detected."""



		def test_drop_models_table_no_tables_detected(mocker):
		"""Ensure any `DROP TABLE` statement executed when no tables detected."""

fix and refactor migrations test teardown and setup #76

fix and refactor migrations test teardown and setup #76

Conversation

skarzi commented May 9, 2020 • edited Loading

codecov bot commented May 9, 2020 • edited Loading

Codecov Report

sobolevn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sobolevn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skarzi commented May 9, 2020 •

edited

Loading

codecov bot commented May 9, 2020 •

edited

Loading