TST: Refactor sql test classes. #49757
Conversation
mroeschke
added
Testing
There was a problem hiding this comment.
Just noting that ideally we would like the use the connection fixtures at the top of the file instead of the setup_method to set up connections. If you see an easy opportunity to incorporate them into these test happy to have that change as well (if not can be a followup for someone else)
|
Unfortunately, that would take a while for me to figure out. |
pandas/tests/io/test_sql.py
Outdated
|
|
||
| @classmethod | ||
| def teardown_class(cls): | ||
| cls.engine.dispose() |
There was a problem hiding this comment.
Shouldn't this still be in the SQLAlchemyMixIn so that the other test classes that inherit this also dispose the engine?
There was a problem hiding this comment.
Agreed, I am changing this.
| def setup_method(self, iris_path, types_data): | ||
| try: | ||
| self.conn = self.engine.connect() | ||
| self.conn.begin() |
There was a problem hiding this comment.
Is the commit or rollback handled in sql.SQLDatabase?
There was a problem hiding this comment.
sql.SQLDatabase doesn't commit or rollback, but sql.SQLiteDatabase does both. I clarified this behavior in the to_sql documentation in #49531 after confirming that it's true in pandas version 1.5.1.
There was a problem hiding this comment.
Okay great thanks for confirming
|
Thanks @cdcadman |
Co-authored-by: Chuck Cadman <[email protected]>
doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.After #49531 , the
sql.SQLDatabaseclass only accepts SQLAlchemy Connections and not Engines. As a result, any test class that would have passed an Engine had to be turned off in that PR, even though it was still needed to support the test classes which passed Connections. The_EngineToConnMixinclass bridged between the Engine test classes and the Connection test classes.In this refactor, I remove the
_EngineToConnMixinclass and edit the setup and teardown methods so that the number of classes is reduced. The number of tests intest_sql.pyand their outcomes remain the same. With sqlalchemy installed, 330 pass, 6 xfail, and 2 are skipped. Without sqlalchemy, 83 pass, and 255 are skipped. The tests seem to be slightly faster after the refactor.This refactor will simplify the PR that I'm ultimately working towards (#48576).