Skip to content

Fix test_sql pytest fixture warnings #22515

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.

Sign up for GitHub

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

Merged
merged 10 commits into from
Sep 14, 2018
Merged

Fix test_sql pytest fixture warnings #22515

Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1e6cc7d
Avoid calling pytest fixtures directly
alimcmaster1 Aug 26, 2018
0f30be7
Add newline
alimcmaster1 Aug 26, 2018
84df6ea
Rename method
alimcmaster1 Aug 28, 2018
d6df77f
Avoid calling pytest fixtures directly
alimcmaster1 Aug 26, 2018
bb905db
Add newline
alimcmaster1 Aug 26, 2018
816e855
Rename method
alimcmaster1 Aug 28, 2018
27c4133
Remove final fixture warning
alimcmaster1 Sep 5, 2018
0d8d239
Remove final fixture warning
alimcmaster1 Sep 5, 2018
13707e7
Merge remote-tracking branch 'remotes/upstream/master' into pytest-fi…
alimcmaster1 Sep 6, 2018
7427924
Merge remote-tracking branch 'origin/pytest-fixtures' into pytest-fix…
alimcmaster1 Sep 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 34 additions & 21 deletions pandas/tests/io/test_sql.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,9 +253,13 @@ def _get_exec(self):
else:
return self.conn.cursor()

def _load_iris_data(self, datapath):
@pytest.fixture(params=[('io', 'data', 'iris.csv')])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a technical reason the path needs to passed here as a param (instead of how it was before)?
We use datapath like this in many other places in the tests

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jorisvandenbossche yes there is, in all the other places we pass in datapath we don't call it directly as a function. Like this code originally did datapath('io', 'data', 'iris.csv'). Which is now deprecated, see this pytest issue, with the suggested refactor.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean with "we don't call it directly as a function" (the datapath fixture itself, or the function that it is passed to?). Because the fixture datapath itself is called directly in many other places in the code base.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alimcmaster1 can you respond here?

I'm in the middle of a PR fixing transitioning warnings to errors. Going to skip sql for now, but this will need to be merged first.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But, if I go from memory, is it because it's passed as a fixture to the caller of this method? The classes setup_method, which then passes it through to load_iris_data as a function, not a fixture?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Firstly apologies for the delayed response I havn't had internet access for the past few days. Yes @jorisvandenbossche agree we do, I worded that badly, but I basically meant what @TomAugspurger said above, we want to avoid passing it like self._load_iris_data(datapath). Then calling datapath directly.

But I see your original point and you are quite right we could write as so

@pytest.fixture()
def load_iris_data(self, datapath)
     csv = datapath('io', 'data', 'iris.csv')

The code in setup_method would remain as i've currently implemented. But I thought why not parameterize? In case we have another data-set we want to run these tests against in the future?

def load_iris_data(self, datapath, request):
import io
iris_csv_file = datapath('io', 'data', 'iris.csv')
iris_csv_file = datapath(*request.param)

if not hasattr(self, 'conn'):
self.setup_connect()

self.drop_table('iris')
self._get_exec().execute(SQL_STRINGS['create_iris'][self.flavor])
Expand Down Expand Up @@ -503,10 +507,14 @@ class _TestSQLApi(PandasSQLTest):
flavor = 'sqlite'
mode = None

@pytest.fixture(autouse=True)
def setup_method(self, datapath):
def setup_connect(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason this is a dedicated method? Seems like a simple enough expression to have just included in the setup method alone

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

load_iris_data fixture basically requires that self has a conn attribute set, see line 262. I basically wanted to separate out the connection setup for each test classes. Since _TestSQLAlchemy already did this I though it would be nice to keep it consistent

self.conn = self.connect()
self._load_iris_data(datapath)

@pytest.fixture(autouse=True)
def setup_method(self, load_iris_data):
self.load_test_data_and_sql()

def load_test_data_and_sql(self):
self._load_iris_view()
self._load_test1_data()
self._load_test2_data()
Expand Down Expand Up @@ -1027,8 +1035,8 @@ class _EngineToConnMixin(object):
"""

@pytest.fixture(autouse=True)
def setup_method(self, datapath):
super(_EngineToConnMixin, self).setup_method(datapath)
def setup_method(self, load_iris_data):
super(_EngineToConnMixin, self).load_test_data_and_sql()
engine = self.conn
conn = engine.connect()
self.__tx = conn.begin()
Expand Down Expand Up @@ -1153,14 +1161,14 @@ def setup_class(cls):
msg = "{0} - can't connect to {1} server".format(cls, cls.flavor)
pytest.skip(msg)

@pytest.fixture(autouse=True)
def setup_method(self, datapath):
self.setup_connect()

self._load_iris_data(datapath)
def load_test_data_and_sql(self):
self._load_raw_sql()
self._load_test1_data()

@pytest.fixture(autouse=True)
def setup_method(self, load_iris_data):
Copy link
Member

@WillAyd WillAyd Aug 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I realize this is not something you have introduced but we seem to be ambiguously using both the classic xunit-style setup and a fixture. I think we should just use one or the other. Probably easiest to just remove the fixture decorator for now

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we remove the fixture annotation this will cause the tests to fail in the current implementation. It seems like this is somewhat common to mix the old xunit style setup and fixtures. See @nicoddemus first comment on this issue. pytest-dev/pytest#517

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But definitely agree we should maybe look at getting rid of the xunit style set up eventually :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What failure did that cause? In the provided link AFAICT it is showcasing an easy transition from old style to new, which is what I'm suggesting here as well

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The xunit setup is applied before any fixtures if we remove the annotation. For example in the test: TestSQLApi.test_read_sql_iris. The function self.load_test_data_and_sql() will be invoked before the load_iris_data fixture ( when @pytest.fixture(autouse=True) is not present ).

Since self.load_test_data_and_sql() requires self.conn to be set, this will error.

Hope that explains the issue clearly :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey a friendly ping: @WillAyd are you happy to review ? Hope my explanation addressed the concerns here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm OK understood on the issues you are facing but just seems like this can be simplified instead of adding new methods. Is it not possible to set the connection here instead of having it as a separate method?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for getting back to me appreciate it - My thinking behind having it as a separate method was that the _TestSQLAlchemy class already has a separate method setup_connect. I did the same in TestSQLiteFallback and _TestSQLApi so that my fixture load_iris_data used across all these test classes could call the same method to do the required connection set up.

Perhaps we need to consider a larger refactor of this file in order to make this set up just solely use fixtures, but with regards to getting rid of 100+ warnings in the 3.6 build mentioned #22338 this was the simplest and clearest change I could think of, let me know if you think otherwise, would be interested to see. Thanks as always for taking a look!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation and your patience. I do see your point; could you open a follow up issue to refactor this module to use fixtures exclusively instead of the mix with inheritance?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure i've raised here #22624. Mind adding the Tags, "Testing", "IO SQL" and perhaps "Medium Difficulty"

self.load_test_data_and_sql()

@classmethod
def setup_import(cls):
# Skip this test if SQLAlchemy not available
Expand Down Expand Up @@ -1925,15 +1933,17 @@ class TestSQLiteFallback(SQLiteMixIn, PandasSQLTest):
def connect(cls):
return sqlite3.connect(':memory:')

@pytest.fixture(autouse=True)
def setup_method(self, datapath):
def setup_connect(self):
self.conn = self.connect()
self.pandasSQL = sql.SQLiteDatabase(self.conn)

self._load_iris_data(datapath)

def load_test_data_and_sql(self):
self.pandasSQL = sql.SQLiteDatabase(self.conn)
self._load_test1_data()

@pytest.fixture(autouse=True)
def setup_method(self, load_iris_data):
self.load_test_data_and_sql()

def test_read_sql(self):
self._read_sql_iris()

Expand Down Expand Up @@ -2151,6 +2161,12 @@ def setup_method(self, request, datapath):
self.method = request.function
self.conn = sqlite3.connect(':memory:')

# In some test cases we may close db connection
# Re-open conn here so we can perform cleanup in teardown
yield
self.method = request.function
self.conn = sqlite3.connect(':memory:')

def test_basic(self):
frame = tm.makeTimeDataFrame()
self._check_roundtrip(frame)
Expand Down Expand Up @@ -2227,7 +2243,7 @@ def test_execute_fail(self):
with pytest.raises(Exception):
sql.execute('INSERT INTO test VALUES("foo", "bar", 7)', self.conn)

def test_execute_closed_connection(self, request, datapath):
def test_execute_closed_connection(self):
create_sql = """
CREATE TABLE test
(
Expand All @@ -2246,9 +2262,6 @@ def test_execute_closed_connection(self, request, datapath):
with pytest.raises(Exception):
tquery("select * from test", con=self.conn)

# Initialize connection again (needed for tearDown)
self.setup_method(request, datapath)

def test_na_roundtrip(self):
pass

Expand Down