Use migra instead of pum for the db migration tests

[elemoine]

With this PR we switch from pum to migra for the db migration tests.

migra provides for a better API, making our db migration tests look better. And migra provides the SQL DDL code to use to go from the source database to the target database, which can help developers write migrations for their changes.

Successful PR Checklist:

• Tests
  • (not applicable?)
• Documentation
  • (not applicable?)
• Had a good time contributing?
• (Maintainers: add PR labels)

[codecov]

Codecov Report

Merging #308 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##            master      #308   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           30        30           
  Lines         1609      1609           
  Branches       183       183           
=========================================
  Hits          1609      1609           

---

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 046a34c...6afcf7d. Read the comment docs.

[elemoine]

There are pros and cons to switching from pum to migra for our database migration tests.

pros

When migra detects differences between the two databases it provides the SQL DDL code to use to go from the source database to the target database, which can help developers write migrations for their changes.

Here's an example of output when the test_migration.py::test_migration test fails:

>       assert not m.statements                                                                                                                                                                
E       assert not ['alter table "public"."procrastinate_jobs" add column "queueing_lock" text;', "CREATE UNIQUE INDEX procrastinate_jobs..._lock_idx ON public.procrastinate_jobs USING btree 
(queueing_lock) WHERE (status = 'todo'::procrastinate_job_status);"]                                                                                                                           
E        +  where ['alter table "public"."procrastinate_jobs" add column "queueing_lock" text;', "CREATE UNIQUE INDEX procrastinate_jobs..._lock_idx ON public.procrastinate_jobs USING btree (
queueing_lock) WHERE (status = 'todo'::procrastinate_job_status);"] = <migra.migra.Migration object at 0x7fd6bd3f32b0>.statements

Compare it to when pum is used:

Check...DIFFERENCES FOUND                                                                                                                                                                      
columns:                                                                                                                                                                                       
- '- (''public'', ''procrastinate_jobs'', ''queueing_lock'', None, ''YES'', ''text'',                                                                                                          
  None, None, None, None)'                                                                                                                                                                     
constraints: []                                                                                                                                                                                
functions: []                                                                                                                                                                                  
indexes:                                                                                                                                                                                       
- '- (''procrastinate_jobs'', ''procrastinate_jobs_queueing_lock_idx'', ''queueing_lock'')'                                                                                                    
rules: []                                                                                                                                                                                      
sequences: []                                                                                                                                                                                  
tables: []                                                                                                                                                                                     
triggers: []                                                                                                                                                                                   
views: [] 

The output of pum is not nice, and it's not of the most useful either.

cons

The part that I like the least with migra is that it pulls the dependencies sqlbag, schemainspect and sqlalchemy. I actually like sqlbag and sqlalchemy, but we don't use them in Procrastinate and it feels a bit strange to use them in our tests.

[ewjoachim]

I'm ok having tests new dependencies as long as it's only test.

[ewjoachim]

Preliminary 👍, waiting for the docs

[k4nar]

Maybe we could include migra as an optional dep, and only install it in a dedicated env in our tests? That way we make sure that the rest of the code works without sqlalchemy & sqlbag.

[elemoine]

Maybe we could include migra as an optional dep, and only install it in a dedicated env in our tests? That way we make sure that the rest of the code works without sqlalchemy & sqlbag.

Yes, migra is already part of the "dev" extra requires. See https://github.com/peopledoc/procrastinate/blob/5d7f98cf08774951fc1caf3404091394a9584d0a/setup.cfg#L42-L49.

So migra (and its dependencies) are installed when using procrastinate[dev], or when using the requirements.txt that we provide for creating the development environment.

[k4nar]

That's not what I meant, sorry :) . I meant as a non mandatory requirement for the dev / tests. The related tests would be skipped if the dependency is not present, and they would be ran in a dedicated env in CI.

[elemoine]

That's not what I meant, sorry :) . I meant as a non mandatory requirement for the dev / tests. The related tests would be skipped if the dependency is not present, and they would be ran in a dedicated env in CI.

I should have figured out that you knew "migra" was a dev extra require 😉

Going back to your idea, yes, that's an option. But not sure it's worth the effort and extra complexity. And developers would not understand if the tests passed on their local dev environments, but not on the CI.

[ewjoachim]

I think our tox setup makes it not so complicated.

Are sqlalchemy or sqlbag known to have side effects ? Would there be a reason the lib would fail without them if we don't explicitely write import sqlalchemy or import sqlbag in the code ? (because otherwise, we'd have the same problem with pytest. But I think I see your point: import pytest would much more likely raise a flag in a review of non-test code than import sqlalchemy would.)

[elemoine]

I removed the pum howto, and replaced it with a migrations howto.

This is ready for review.

[ewjoachim]

a few details in the doc. Otherwise, 👍