TST: Load iris and types data in sql tests as needed #55265
Conversation
mroeschke
added
Testing
| }, | ||
| ] | ||
| stmt = insert(datetz).values(datetz_data) | ||
| if isinstance(conn, Engine): |
There was a problem hiding this comment.
I don't think you need the isinstance here - both Engine and Connectable has a begin contextmanager which does the right thing
https://docs.sqlalchemy.org/en/20/core/connections.html#connect-and-begin-once-from-the-engine
There was a problem hiding this comment.
Oh cool! I didn't know this; I'll give it a go
There was a problem hiding this comment.
Looks like we can't fully generalize these two branches due to use of execute, so will just keep the old invocation.
There was a problem hiding this comment.
Hmm strange. I think before when you had
with conn.begin():
...
conn.execute()if that instead was
with conn.begin() as con:
...
con.execute()it would work
|
This generally looks good - thanks for cleaning these up |
| @@ -245,6 +243,43 @@ def create_and_load_types(conn, types_data: list[dict], dialect: str): | |||
| conn.execute(stmt) | |||
|
|
|||
|
|
|||
| def create_and_load_postgres_datetz(conn): | |||
There was a problem hiding this comment.
One thing I've been kind of careful in while setting up the ADBC connectors is an unecessary reliance on SQLAlchemy, since it would be nice to run the test suite without that dependency when not required.
Not a blocker for this PR just a larger consideration; with the ADBC change in particular I think I'd fall back to writing postgres-specific SQL for this
There was a problem hiding this comment.
That's a good point. I can do another pass after this PR to try to refactor places where data is loaded to just use raw SQL
|
Thanks for the review @WillAyd. Going to merge this in and look at the data loading cleanups in a follow up PR |
Also factors out timezone data in a fixture to only apply to the single test where it's used