Convert test_sql to pytest idiom #54936
Conversation
| @@ -476,17 +491,25 @@ def sqlite_conn(sqlite_engine): | |||
|
|
|||
|
|
|||
| @pytest.fixture | |||
| def sqlite_iris_str(sqlite_str, iris_path): | |||
| def sqlite_iris_str(sqlite_str, iris_path, types_data): | |||
There was a problem hiding this comment.
I think in the long run it would be better to not have separate _iris fixtures. The challenge is there is inconsistency across the drivers right now whether they work with the iris loading function (sqlite_building and sqlite_str being two that come to mind). So didn't try to change that much in this PR; leaving it to a future enhancement
There was a problem hiding this comment.
Hmm personally I would prefer to have separate fixtures for the connection and connection + iris
@pytest.fixture
def connectable():
with connection as con:
yield conn
conn.dump_all_tables_and_dispose()
@pytest.fixture
def connectable_with_iris(connectable):
connectable.insert_iris_data()
yield connectable
connectable.delete_iris_data()It makes it clear which tests specifically need iris data and ensures that there's no leftover state for subsequent tests if there's a failure
There was a problem hiding this comment.
Sounds good. No objection to that either - just will take a little more effort to get us there, which I don't plan on doing in this PR
There was a problem hiding this comment.
There's also the types table - do you think that should be a separate fixture or rolled into the iris one?
There was a problem hiding this comment.
Sounds good. No objection to that either - just will take a little more effort to get us there, which I don't plan on doing in this PR
No worries, I can help out with this when I have spare cycles
There's also the types table - do you think that should be a separate fixture or rolled into the iris one?
I don't remember how the types tables are used but my first reaction is that they should
There was a problem hiding this comment.
I think we can also make it so the iris / type table fixtures re-use the all_connectables fixture, instead of duplicating each connection for iris / type. So loosely would look something like this:
@pytest.fixture
def postgres_conn():
...
@pytest.fixture
def sqlite_conn():
...
all_connectable = [postgres_conn, slqite_conn, ...]
@pytest.fixture
def iris_data(all_connectable):
...
@pytest.fixture
def types_data(all_connectable):
...Today we have sqlite_conn_iris, postgres_conn_iris iterations and the types tables are loosely mixed in somewhere
| @@ -138,7 +138,7 @@ def _parse_date_columns(data_frame, parse_dates): | |||
| if isinstance(df_col.dtype, DatetimeTZDtype) or col_name in parse_dates: | |||
| try: | |||
| fmt = parse_dates[col_name] | |||
| except TypeError: | |||
| except (KeyError, TypeError): | |||
There was a problem hiding this comment.
I couldn't reproduce this locally but it seems like this code didn't like the case when DateColWithTz was inferred to be temporal and was not included in parse_dates
| @pytest.mark.parametrize("conn", all_connectable) | ||
| def test_api_to_sql_index_label_multiindex(conn, request): | ||
| conn_name = conn | ||
| if "mysql" in conn_name: |
There was a problem hiding this comment.
I wasn't able to reproduce this locally using MariaDB but kept seeing it on CI. Not sure if it is a version thing or MySQL <> MariaDB difference.
|
I think this is mergable. Plan is to keep getting rid of these classes in follow ups; let me know of any feedback |
mroeschke
added
Testing
|
Thanks @WillAyd |
* Convert test_sql to pytest idiom * Try KeyError catch * Added drop_view to existing test method * xfail MySQL issue
Should make onboarding the new ADBC drivers a little easier.
This can be done in a few phases/PRs
@jorisvandenbossche