Skip to content
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

Fix test_sql pytest fixture warnings #22515

Merged
merged 10 commits into from
Sep 14, 2018
Merged

Fix test_sql pytest fixture warnings #22515

Changes from 2 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
44 changes: 27 additions & 17 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):
Copy link
Member

Choose a reason for hiding this comment

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

Given that it's now a fixture, let's remove the initial underscore in the function name.

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 @gfyoung that makes sense! Will go ahead and do this, any other feedback from anyone?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated

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):
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