-
Notifications
You must be signed in to change notification settings - Fork 115
Tolerate missing PostgreSQL database in ixmp.testing #611
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #611 +/- ##
=====================================
Coverage 98.2% 98.3%
=====================================
Files 51 51
Lines 6557 6577 +20
=====================================
+ Hits 6444 6466 +22
+ Misses 113 111 -2
🚀 New features to boost your workflow:
|
9403171 to
27d4ba3
Compare
- Protect database connection in pytest_session{start,finish} hooks in a
try/except block.
- Don't parametrize the "backend" fixture for "ixmp4" if no database
server is available.
- Add pytest_report_collectionfinish() hook to display message if
connection fails.
- Drop any existing "ixmp_test_..." database when connecting.
- Add .util.ixmp4.format_url() to adjust URLs to those supported by ixmp4.
27d4ba3 to
529430c
Compare
- This ensures the tests in test_tutorials will connect to the database server and specific database established for the pytest-xdist worker by pytest_sessionstart(). - Drop "pytest" workflow step to manually establish "template1" and "ixmp_test" databases.
Don't include the "ixmp_test" value used in the test suite as part of the default configuration for users.
c2510e6 to
c92b9c7
Compare
|
The codecov/patch check failure leads me to see (here) that the pytest_sessionstart() hook is not run. I'm not sure why this is, but in order to fix CI for other repos, will merge. We should investigate this later. |
glatterf42
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for these fixes and sorry that I didn't test message_ix against this branch (or note this requirement as a todo before merging).
| _backend._backend.teardown() | ||
| if is_sqlalchemybackend(_backend._backend): | ||
| _backend._backend.close() | ||
| # TODO Properly isinstance check and remove when Python 3.9 is dropped |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this line could have been removed now thanks to the new typeguard :)
| - name: Setup template & default-ixmp4-local Postgres schema | ||
| if: matrix.python-version != '3.9' | ||
| run: | | ||
| ixmp4 platforms add --dsn "postgresql+psycopg://postgres:postgres@localhost:5432/template1" template1 | ||
| ixmp4 platforms add --dsn "postgresql+psycopg://postgres:postgres@localhost:5432/ixmp_test" ixmp_test | ||
| IXMP4_MANAGED=false ixmp4 platforms upgrade |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused about how we ensure the testing DB is running the latest state of the migrations, I thought setup() alone was not enough for that. But the passing tests indicate it works :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked for documentation for setup(), but it seems there isn't any. As far as I can guess, the setup() of a new backend sets it up using the schema corresponding to the ixmp4 version in use.
If the underlying database schema changes between ixmp4 versions, and the user accesses the same database with first one ixmp4 version and then a different version, then that package should do things like:
- apply migrations (from older to newer schemas, maybe in some configurable way), or
- raise appropriate exceptions (e.g. if it's not possible to go backwards from newer to older schemas)
The Java ixmp_source code does this. If the ixmp4 package itself doesn't do that yet, we can just advertise that as a limitation on using ixmp/message_ix with IXMP4Backend, for the time being. If it won't ever do that (considered out of scope), then it should be handled by IXMP4Backend itself, automatically and transparency, and shouldn't require any extra steps.
| session.config.stash[KEY_ENGINE] = engine | ||
| try: | ||
| with engine.connect() as connection: | ||
| connection.execute(text(f"DROP DATABASE IF EXISTS {db_name}")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thing I just thought of here: we should make sure that no one accidentally runs the tests with WRITE permissions on a production DB to avoid deleting the whole DB. Though I'm not sure how we guard against that. Maybe once we have a URL for that, we can hardcode a check that this is not the DB we're about to drop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
once we have a URL for that, we can hardcode a check that this is not the DB we're about to drop?
Yes, that sounds like a great safeguard. IOW before pytestconfig.options.ixmp_postgres is used, we can compare it to some fixed list and abort if it matches.
I think the production DBs should also be configured such that no users or very limited (admin) users have permission to use the SQL DROP DATABASE; and then tightly limit who can connect and authenticate as those users. That way, anyone (person or CI worker) trying to run the test suite will not have the right credentials/permissions, and this would fail even if they managed to connect.
Finally, we could also:
- Avoid using the name
ixmp_test_.*for any production database. The code will only construct such names. - Maybe change the constructed name to something like
__ixmp_test, with the prefix underscore(s) indicating a non-production/special case.
That would be at least four kinds of protection.
#601 added code to the ixmp.testing.pytest_sessionstart() hook that connects to a PostgreSQL server for testing IXMP4Backend. The added code always runs, even if there is no PostgreSQL server available at the hard-coded URL.
This led to failures today in the scheduled message_ix, message-ix-models, and message_data workflows that use ixmp
main:To resolve:
try:/except:block.Also:
postgresql://instead ofpostgresql+psycopg2://where possible.How to review
ixmp.testingas a plugin, e.g. the message-ix test suite.PR checklist
Add, expand, or update documentation.Test changes only.