Skip to content
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

New release #317

Closed
jmbowman opened this issue Feb 10, 2016 · 14 comments
Closed

New release #317

jmbowman opened this issue Feb 10, 2016 · 14 comments

Comments

@jmbowman
Copy link

The --keepdb fix from #261 looks like it'll save about 4-7 minutes on each test run of a project I'm working on (depending on hardware); is there a timeline yet for a new pytest-django release which would include it? I don't see any issues currently tagged as release blockers.

@blueyed
Copy link
Contributor

blueyed commented Feb 10, 2016

Does --reuse-db not work for you?

@jmbowman
Copy link
Author

It does until a new migration is added; then you need to explicitly drop the --reuse-db parameter in order to apply the migration (making for a slow test run), and then add it back afterwards (so the next time is slow again because the last run didn't keep the database). The advantage of --keepdb is that it runs any unapplied migrations before running the tests, so:

  • You don't need to change the parameters used to run the tests depending on whether a new migration was added or not
  • Applying just the new migrations once is much faster than 2 test runs applying them all
  • You still need to recreate the DB if you change or remove a migration in development, but that's true of both flags

This drawback of --reuse-db in the current release is annoying but tolerable when running tests manually, but a royal pain when running continuous integration tests on git commits. I have multiple Jenkins jobs set up to run tox like this for different packages, and there's no good way to add the flag just when the commit contains no new migrations (so the flag is always off and it's always slow). I've been trying to cut down on the duration of those jobs to shorten the feedback loop on code changes, and this looks like a simple way to make some of them up to 50% faster.

Just in case it wasn't clear, I think this change was already made in the PR I mentioned (to have --reuse-db actually use --keepdb behind the scenes on Django 1.8+). I'd just like it to be in an official release to avoid needing to package up a private release (and to aid people for whom that's an even bigger hassle because they haven't already set up a private package server).

@blueyed
Copy link
Contributor

blueyed commented Feb 11, 2016

Thanks for your explanation.

Although I've added the change myself, I was not aware of it's benefits.. :)
I drop the test db in case of missing migrations, which makes --reuse-db recreate it.
FWIW, are you aware of the --nomigrations flag?

Apart from that a new release would be nice to have anyway.

@jmbowman
Copy link
Author

--nomigrations works well in many cases, but I have some projects which have tests to verify that database constraints created by SQL in migrations (because Django models don't yet have a way to define partial unique indexes, etc.) are working as intended and handled correctly when violated. Those tests consistently fail if you skip the migrations altogether, since the constraint never gets created.

@blueyed
Copy link
Contributor

blueyed commented Feb 11, 2016

since the constraint never gets created

I see. For this you could have pytest fixtures instead / additionally: the fixture would either use the data from the DB/migration or create it to be available for the tests.

@jmbowman
Copy link
Author

Yes, and that's roughly what I did pre-1.7 when it was tougher to integrate migrations into the test cycle. But that meant duplicating the code for making the relevant database changes, and introduced a risk that you're testing something which doesn't quite match what you're actually applying in the migrations for real installations.

@blueyed
Copy link
Contributor

blueyed commented Feb 11, 2016

Yeah. Thanks for elaborating again.

@pelme
What do you think about having a new release? :)

@pelme
Copy link
Member

pelme commented Feb 11, 2016

I will try to put out a new release in the next couple of days. I would like to test this change out with my project properly (I use --reuse-db quite extensively).

Btw, I also have problems with slow CI builds because of migrations and would like to improve things further:

  • I did a quick hack the other day calculating a md5 hash of all migration files which was fairly easy. The md5 hash could be saved in a special/hidden table in the database and then be used to re-run all migrations when any migration file actually changed. I think that would give 100 % accuracy on when to re-run migrations and speedy runs all other times.
  • When using xdist: I would like to make it possible to create a single database from the migrations and then copy that database. Currently when I use i.e. -n4, all migrations runs in 4 processes which is a real performance killer. Doing the migrations in a single process and then creating 4 copies with CREATE DATABASE ... TEMPLATE ...; would probably speed things up a lot.

@blueyed
Copy link
Contributor

blueyed commented Feb 11, 2016

For what it's worth, you could also use something like this to skip Travis builds for migration tests (instead of the md5 hash):

if git diff --quiet --exit-code "$TRAVIS_COMMIT_RANGE" \
    project/app/migrations project/app/tests/test_migrations.py; then
        ...

@blueyed
Copy link
Contributor

blueyed commented Feb 11, 2016

btw: https://github.com/fastmonkeys/stellar is really nice in this regard, too: it creates DB snapshots.
But there was some issue when I've tried to integrate it with pytest-django / our CI: fastmonkeys/stellar#53.

@blueyed
Copy link
Contributor

blueyed commented Feb 18, 2016

I've just tried using master (with the --keepdb patch), instead of dropping the test database, but that resulted in an error:

psycopg2.ProgrammingError: relation "django_content_type" already exists

Maybe related to using --nomigrations also?

The hopefully relevant part of the traceback:

File "…/pyenv/project/lib/python3.5/site-packages/_pytest/python.py", line 2057, in call_fixture_func
  res = fixturefunc(**kwargs)
File "…/pytest-django/pytest_django/fixtures.py", line 57, in _django_db_setup
  interactive=False, **db_args)
File "…/django/django/test/runner.py", line 726, in setup_databases
  serialize=connection.settings_dict.get("TEST", {}).get("SERIALIZE", True),
File "…/django/django/db/backends/base/creation.py", line 70, in create_test_db
  run_syncdb=True,
File "…/django/django/core/management/__init__.py", line 119, in call_command
  return command.execute(*args, **defaults)
File "…/django/django/core/management/base.py", line 399, in execute
  output = self.handle(*args, **options)
File "…/django/django/core/management/commands/migrate.py", line 200, in handle
  executor.migrate(targets, plan, fake=fake, fake_initial=fake_initial)
File "…/django/django/db/migrations/executor.py", line 92, in migrate
  self._migrate_all_forwards(plan, full_plan, fake=fake, fake_initial=fake_initial)
File "…/django/django/db/migrations/executor.py", line 121, in _migrate_all_forwards
  state = self.apply_migration(state, migration, fake=fake, fake_initial=fake_initial)
File "…/django/django/db/migrations/executor.py", line 198, in apply_migration
  state = migration.apply(state, schema_editor)

File "…/django/django/db/migrations/migration.py", line 123, in apply
  operation.database_forwards(self.app_label, schema_editor, old_state, project_state)
File "…/django/django/db/migrations/operations/models.py", line 59, in database_forwards
  schema_editor.create_model(model)
File "…/django/django/contrib/gis/db/backends/postgis/schema.py", line 56, in create_model
  super(PostGISSchemaEditor, self).create_model(model)
File "…/django/django/db/backends/base/schema.py", line 284, in create_model
  self.execute(sql, params or None)
File "…/django/django/db/backends/base/schema.py", line 110, in execute
  cursor.execute(sql, params)
File "…/django/django/db/backends/utils.py", line 64, in execute
  return self.cursor.execute(sql, params)
File "…/django/django/db/utils.py", line 95, in __exit__
  six.reraise(dj_exc_type, dj_exc_value, traceback)
File "…/django/django/utils/six.py", line 685, in reraise
  raise value.with_traceback(tb)
File "…/django/django/db/backends/utils.py", line 62, in execute
  return self.cursor.execute(sql)
django.db.utils.ProgrammingError: relation "django_content_type" already exists

@blueyed
Copy link
Contributor

blueyed commented Feb 18, 2016

Ok, for clarification: --nomigrations skips trying to apply the migration, but without it the above error happens.

@sasha0
Copy link

sasha0 commented Apr 21, 2016

@blueyed did you run tests without --nomigrations and with --keepdb option on the fresh database?

@blueyed
Copy link
Contributor

blueyed commented Aug 25, 2016

@sasha0
Looks like it, but I don't remember - sorry for the late response.

Anyway, the issue itself is fixed: there's a 3.0 release now.. 🎉

@blueyed blueyed closed this as completed Aug 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants