Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#711 closed enhancement (fixed)

Switch unit tests to using in-memory sqlite databases

Reported by: Christopher Allan Webber Owned by:
Priority: minor Milestone: 0.5.0
Component: programming Keywords:
Cc: Elrond Parent Tickets:


At the moment, running tests that don't use a full mediagoblin app are fast, ones that do are slow. Probably the reason for this is that we use 3 sqlite databases (one for celery, one for Kombu, one for mediagoblin itself).

I'm not sure whether the celery of kombu (which isn't used during tests anyway) databases would be affected by unit tests if we just switched them over to an in-memory database. It might be easy enough to just do those and they'd be just fine. I don't know.

MediaGoblin's db is a bit different because it needs to run the migration machinery to init the db. This means that things will be a bit tricky: Somehow we need to create a reference to the database *before* the MediaGoblin application spins up and connects it to the ORM. Then we need to pass that database connection (without the ORM initialized) to the migrations, then pass it into the MediaGoblin application, which must then connect the ORM to the database based on init.

Problem: we set up MediaGoblin's application wrapped in paste, so we don't have much control over this.

Here's an alternate solution, also evil, maybe even more so: MediaGoblin the application itself could contain a config option where it will, on init, run dbupdate from *within itself* if needed. (Note: that *could* be used in production environments also but I think it would more likely than not, especially if a person is both running MediaGoblin *and* celery, shoot us in the foot.)

Change History (5)

comment:1 by Emily O'Leary, 11 years ago

Status: newreview

I've implemented this in the following commit:

I had it check for a run_migrations configuration option and if it was set to true (default is false) then it runs the run_all_migrations function which was part of the old dbupdate function. I moved that functionality to this new function and simply called it from within dbupdate as to facilitate code reuse.

Now with these config changes in place tests take approximately one minute to run all 103 current tests on my machine versus before where it would take approximately six minutes to complete them all.

comment:2 by Christopher Allan Webber, 11 years ago

This is really great. The speed difference here is huge! I have a few comments, but already this is really useful:

  • Thanks for writing a docstring on run_all_migrations. It sounds a bit like a commit message though since it explained what you did rather than what the function does?
  • It's good to wrap text at 80 characters. This greatly enhances readability (terminals in general don't go farther than this, and the human eye does an easier job "scanning down", and long lines prevent this... 80 characters is around "book page length")
    • Not going to make you redo the commit, but in the pfuture, please do try to wrap the commit comment at the 80th column.
    • Similarly, the config_spec.ini comment should wrap.
  • Could you add a comment to config_spec.ini explaining that this is for testing only, and we do not recommend users turn it on their own instances?
  • Lastly, I wonder if there would be a speedup if we switch the celery/kombu database lines over. That would be the CELERY_RESULT_DBURI and BROKER_HOST lines in the [celery] section of the config. However I just tested it and noticed no difference... oh well ;)

Thanks, Emma! Looking forward to merging this!

comment:3 by Emily O'Leary, 11 years ago

I altered the comments based on your suggestions. See the feature branch:

comment:4 by Christopher Allan Webber, 11 years ago

Resolution: fixed
Status: reviewclosed

Awesome. I merged it! Great work!

comment:5 by Christopher Allan Webber, 11 years ago

Milestone: 0.4.1
Note: See TracTickets for help on using tickets.