🧪 TESTS: SQLA Migrations -> pytest

[chrisjsewell]

This PR primarily converts the SQLA Migrations tests from using AiidaTestCase to using pytest fixtures, and splits them out into multiple files (to improve readability).
The tests are functionally equivalent, just restructured and with a reduction in duplication when using sqlalchemy.Session contexts (hence the reduction in lines).
One more test has been added (tests/backends/aiida_sqlalchemy/migrations/test_all_basic.py), to cycle though all migration versions and try migrating them down&up from&to the latest revision (against an empty database). The list of versions is dynamically generated, to ensure none are missed.

To ensure a fully empty database, an option has been added to the test manager aiida_profile.reset_db(with_user=False) (defaults to True), whereby the default User will not be auto-generated. This was required for some migrations to succeed when migrating down then up, and was how the current unittest implementation worked.

Note this was actually done in a seperate PR in the end

Additionally, methods have been renamed:

- `get_schema_version_database()` -> ~~`get_schema_version_current()`~~ `get_schema_version_backend()`
- `get_schema_version_code()` -> ~~`get_schema_version_latest()`~~ `get_schema_version_head()`

I feel this is clearer what they return

Also fixes sqlalchemy v2 deprecation: https://docs.sqlalchemy.org/en/14/orm/extensions/automap.html?highlight=prepare#sqlalchemy.ext.automap.AutomapBase.prepare.params.autoload_with

This is a precursor to #5097

[ltalirz]

Thanks a lot @chrisjsewell !

I haven't looked at the code yet, but just from reading your PR description:

Additionally, methods have been renamed:

• get_schema_version_database() -> get_schema_version_current()
• get_schema_version_code() -> get_schema_version_latest()

I don't know how others feel about this but to me "current" and "latest" are rather easily confused - not when seeing them together like this, but if I just see a get_schema_version_current() somewhere I could imagine both associating this with the schema version in the code and the one in the database.

Given the choice between the old and new naming schemes I would probably stick with the old, but of course open to better alternatives.

[sphuber]

I agree with @ltalirz . I am not married to the current choice, but I definitely think that latest and current are not at all clear as to which one refers to the version of the code and which one to what is actually in the database. That is why I named it database and code originally as that seems to be the distinction to me.

[chrisjsewell]

Firstly, I would note get_schema_version_database() is factually incorrect, since it is not the version of just the database, it is the version for the entire instance of a backend's persistent data, including the repository (for example 1feaea71bd5a_migrate_repository.py).
If not current, then I would suggest get_schema_version_instance()

get_schema_version_code(); I honestly had no idea what this meant, until I looked into the code. It also may have nothing to do with the code, for example if you are simply adding some indexes to tables.
If not latest, then I would suggest get_schema_version_head(), as is already used by alembic.

As I have discussed in #5154, really there is an element missing here, which is a version that can be used to compare different backends, i.e. currently the "head" schema for Django (0049), SQLA (34a831f4286d), and archive (0.13), really give no indication what they are actually implementing or whether they are "compatible".

[sphuber]

Would be fine with get_schema_version_head but how about get_schema_version_backend instead of get_schema_version_instance? Instance feels too vague, and we are actively working towards having the entirety of the data store be referenced to as "backend" so that seems to make sense no?

[chrisjsewell]

Would be fine with get_schema_version_head but how about get_schema_version_backend instead of get_schema_version_instance? Instance feels too vague, and we are actively working towards having the entirety of the data store be referenced to as "backend" so that seems to make sense no?

Yep sounds good, I've changed the names to these, so ready for review

[codecov]

Codecov Report

Merging #5192 (cfdd873) into develop (053efee) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #5192      +/-   ##
===========================================
+ Coverage    81.38%   81.43%   +0.05%     
===========================================
  Files          529      529              
  Lines        37002    37002              
===========================================
+ Hits         30112    30129      +17     
+ Misses        6890     6873      -17     

Flag	Coverage Δ
django	76.85% <100.00%> (-0.02%)	⬇️
sqlalchemy	75.87% <100.00%> (+0.05%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiida/manage/tests/main.py	88.48% <100.00%> (ø)
aiida/engine/daemon/client.py	76.27% <0.00%> (+1.02%)	⬆️
aiida/backends/sqlalchemy/manager.py	84.47% <0.00%> (+3.89%)	⬆️
...ns/62fe0d36de90_add_node_uuid_unique_constraint.py	85.72% <0.00%> (+4.77%)	⬆️
...grations/versions/162b99bca4a2_drop_dbcalcstate.py	100.00% <0.00%> (+8.34%)	⬆️
...ns/versions/5d4d844852b6_invalidating_node_hash.py	100.00% <0.00%> (+18.75%)	⬆️
...ns/versions/a603da2cc809_code_sub_class_of_data.py	100.00% <0.00%> (+20.00%)	⬆️
...sions/140c971ae0a3_migrate_builtin_calculations.py	100.00% <0.00%> (+20.00%)	⬆️

---

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 053efee...cfdd873. Read the comment docs.

[sphuber]

@ltalirz you also happy with get_schema_version_head and get_schema_version_backend?

[chrisjsewell]

@ltalirz you also happy with get_schema_version_head and get_schema_version_backend?

Pinging @ltalirz, would like to get this merged ASAP 😄

[sphuber]

Why separate out some of these migration tests to separate files, but not all of them? Feels a bit arbitrary to take a few out but still leave a lot of them in test_other.py...

[chrisjsewell]

Yeh fair: I felt it was too much for one file (as before), but overkill to split every migration into a separate file.
I've now split them up a bit better; trying to group them in related/sequential migrations, and added numbering to the file names, to make it easier to see the order.

[chrisjsewell]

this is ready for re-review cheers @sphuber

[chrisjsewell]

Ah what a pain in the backside: tox -e py37-sqla tests/backends/aiida_sqlalchemy/migrations runs fine, but there is something about the prior tests/backends/aiida_sqlalchemy/test_session.py, that is leaving the test database in a state that is making the migrations stall (only if you call the tests in a certain order)

[chrisjsewell]

Ah this so ***ing annoying, it runs fine locally, but here it hangs on a migration for some reason

[chrisjsewell]

Ah this so ***ing annoying, it runs fine locally, but here it hangs on a migration for some reason

Yeh err there is just something causing locking of the database in the migration tests, and it seems different whether you are using the test database automatically created by pgtest (what I do locally) or created upfront then setting AIIDA_TEST_PROFILE (whats done on GH Actions).

Note this only affects the tests, not running the migrations manually

It is the same thing I've encountered in #5097 (comment)

@sphuber / @giovannipizzi if you want to give this some "fresh eyes" (namely the perform_migration fixture in tests/backends/aiida_sqlalchemy/migrations/conftest.py), otherwise I'll revisit afresh when I get back from holiday in a week