test: refactor of tokenserver python tests to support postgres #1855
Conversation
taddes
changed the title
| import unittest | ||
|
|
||
| from integration_tests.tokenserver.test_support import TestCase | ||
| from sqlalchemy.sql import text as sqltext |
There was a problem hiding this comment.
We use this sqltext alias elsewhere, so keeping it consistent.
| engine = create_engine(os.environ["SYNC_TOKENSERVER__DATABASE_URL"]) | ||
| self.database = engine.execution_options(isolation_level="AUTOCOMMIT").connect() | ||
|
|
||
| self.db_mode = os.environ["SYNC_TOKENSERVER__DATABASE_URL"].split(":")[0] |
There was a problem hiding this comment.
Enables us to check Postgres or SQL. This ended up being the best way to check.
| if id: | ||
| query += ", id) VALUES(%s, %s, %s, %s, %s, %s, %s, %s)" | ||
| data += (id,) | ||
| if not id: |
There was a problem hiding this comment.
I know this is verbose, but I have a personal dislike of constructing queries as strings and using concatenation, which in my experience have let to annoying bugs. I thought for our purposes, it's just simpler to have the different explicit queries, and then their Postgres variants. If/when we ever just move to a single DB for support, this can go, but for now, it's much less a headache.
taddes
force-pushed
the
test/adapt-tokenserver-python-tests-postgres-STOR-384
branch
2 times, most recently
from
71e150c to
f224fca
Compare
taddes
force-pushed
the
test/adapt-tokenserver-python-tests-postgres-STOR-384
branch
from
f224fca to
f52320d
Compare
| if self.db_mode == "postgres": | ||
| insert_sql = sqltext("""\ | ||
| insert into users (service, email, nodeid, generation, keys_changed_at, client_state, created_at, replaced_at) | ||
| values (:service, :email, :nodeid, :generation, :keys_changed_at, :client_state, :created_at, :replaced_at) |
There was a problem hiding this comment.
(very much nit on formatting) I guess we don't have linter for this python code? These lines are long but also inconsistent with those in the else block.
There was a problem hiding this comment.
We do have a linter (ruff) but it doesn't work/apply well to the multi-line comments, which can be annoying. I think this can be made to be similar to the SQL standard comment guide where the SQL statements can line up. No argument on it looking inconsistent...
chenba
left a comment
chenba
left a comment
There was a problem hiding this comment.
LGTM. I like the explicit separation of MySQL and Postgres so we can delete the MySQL specific code easily.
taddes
deleted the
test/adapt-tokenserver-python-tests-postgres-STOR-384
branch
Description
The integration test’s base TestCase writes directly to the database with raw mysql queries.
These have been adapted to work with postgres, while maintaining MySQL support. This is through:
db_modeattribute that allows for checking if theSYNC_TOKENSERVER__DATABASE_URLenv var resolves to apostgresormysqlprefix, to determine the logic paths.textmethod (aliased assqltext) so that agnostic bind parameters work in both implementations. This also removes the clunky use of %s params.LAST_ROW_IDwhich only works for SQL. There were only 3 references to this method anyway, so it was not a particularly beneficial abstraction.Testing
Running Tokenserver integration tests
Issue(s)
Closes STOR-384.