-
Notifications
You must be signed in to change notification settings - Fork 13
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
[Issue #2190] Fix test setup and expand factories for foreign table load #2191
Conversation
api/tests/conftest.py
Outdated
@pytest.fixture(scope="class") | ||
def truncate_foreign_tables(self, db_session): | ||
for table in foreign_metadata.tables.values(): | ||
db_session.execute(delete(table)) |
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.
Not sure if it's worth the trouble, but doing TRUNCATE TABLE is often way more efficient (skips writing the DELETEs to the log) for large tables. From my poking around it seems like SQLAlchemy doesn't support it via it's API (which makes sense as not every version of every DB engine supports it) but we could run it as a raw SQL if we wanted to go that route.
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.
Only a minor change to do - it works fine in this case.
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.
Looks good, just flagged about running TRUNCATE instead of DELETE but that could just be a note for the future.
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.
Great!
Summary
Fixes #2190
Time to review: 10 mins
Changes proposed
Make the load from Oracle logic be tested with actual table data
Add factories for foreign tables by refactoring the staging table factories to inherit from a base class
Explicitly define which tables we want to load (statically, but allow user override for testing)
Adjustments to the way data is generated in the local seed script (to be expanded later)
Context for reviewers
Apologies for the size - this is a lot of work towards adding more testing in our transform logic that will come in future PRs. Think of this as a lot of cleanup and pre-work in order to properly make more thorough testing.
This work started as fixing an issue I noticed where before it would attempt to always load EVERY table we have configured as a foreign/staging table. This would mean in order to add any new table to these schemas, we would need to make sure the foreign data wrapper connection existed in an environment before deploying which would not be ideal. This way lets us create the tables, and test them in a decoupled fashion. For example, I recently added a
tgroups
table. This needs to be manually tested before we turn it on with the rest of the job.Additional information
Running locally, you can test some of this by doing:
You can also run something like this after to just have it process a single table