Fix test_sql pytest fixture warnings #22515

alimcmaster1 · 2018-08-26T18:42:53Z

[y] closes TST: incorrectly used fixtures causing warnings #22338
[y] tests added / passed
[y] passes git diff upstream/master -u -- "*.py" | flake8 --diff

See Travis CI Python 3.6 build before and after my changes get rid of the warning for calling fixtures directly.

You can see this by searching for the string "pandas/tests/io/test_sql.py" to see that well over 100 warnings have been removed.

pep8speaks · 2018-08-26T18:42:57Z

Hello @alimcmaster1! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on August 31, 2018 at 17:49 Hours UTC

codecov · 2018-08-26T20:35:37Z

Codecov Report

Merging #22515 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #22515   +/-   ##
=======================================
  Coverage   92.04%   92.04%           
=======================================
  Files         169      169           
  Lines       50782    50782           
=======================================
  Hits        46744    46744           
  Misses       4038     4038

Flag	Coverage Δ
#multiple	`90.45% <ø> (ø)`	⬆️
#single	`42.26% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 14ccdd2...7427924. Read the comment docs.

alimcmaster1 · 2018-08-26T20:52:45Z

See Travis CI Python 3.6 build before and after my changes this get rid of the warning for calling fixtures directly.

gfyoung · 2018-08-27T09:40:43Z

pandas/tests/io/test_sql.py

@@ -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')])
+    def _load_iris_data(self, datapath, request):


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

Sure @gfyoung that makes sense! Will go ahead and do this, any other feedback from anyone?

gfyoung · 2018-08-29T03:38:15Z

https://travis-ci.org/pandas-dev/pandas/jobs/421777999#L2544

@jreback : Not sure why scipy is breaking on these builds suddenly...did something happen to the 2.7 package for scipy ?

WillAyd · 2018-08-29T04:35:27Z

pandas/tests/io/test_sql.py

        self._load_raw_sql()
        self._load_test1_data()

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


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

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

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

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

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

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

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?

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!

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?

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

WillAyd · 2018-08-29T04:36:11Z

pandas/tests/io/test_sql.py

@@ -503,10 +507,14 @@ class _TestSQLApi(PandasSQLTest):
    flavor = 'sqlite'
    mode = None

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


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

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

alimcmaster1 · 2018-08-31T17:51:29Z

Close/Re-open to restart CI after SciPy failures

gfyoung · 2018-08-31T18:01:54Z

@alimcmaster1: feel to also ping us if you want to restart one of your builds so that you don't have to retrigger the entire test suite.

alimcmaster1 · 2018-09-01T17:56:31Z

Thanks @gfyoung will do that in the future!

WillAyd

Checking the 3.6 log there still appears to be one more warning - do you mind taking a look to see if that can be resolved?

WillAyd · 2018-09-04T23:09:56Z

pandas/tests/io/test_sql.py

        self._load_raw_sql()
        self._load_test1_data()

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


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?

…xtures

…tures

alimcmaster1 · 2018-09-06T23:37:05Z

Thanks @WillAyd for reviewing this. I've update to get rid of the final warning for this file. See travis 3.6 build now is warning free for this file :)

WillAyd

Generally OK though certainly do not want to lose sight of the refactor for the issue you opened

@gfyoung what do you think?

gfyoung

LGTM too! Good call on breaking off the refactor for later.

cc @jreback

jorisvandenbossche · 2018-09-07T12:44:39Z

pandas/tests/io/test_sql.py

@@ -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')])


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

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.

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.

@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.

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?

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?

TomAugspurger · 2018-09-14T01:54:06Z

This looks good to me. Let's merge so we can make progress on getting our CI in shape again.

* Avoid calling pytest fixtures directly

Avoid calling pytest fixtures directly

1e6cc7d

Add newline

0f30be7

alimcmaster1 changed the title ~~WIP: Fix test_sql pytest fixture warnings~~ Fix test_sql pytest fixture warnings Aug 26, 2018

gfyoung added Testing pandas testing functions or related to the test suite IO SQL to_sql, read_sql, read_sql_query labels Aug 27, 2018

gfyoung reviewed Aug 27, 2018

View reviewed changes

Rename method

84df6ea

WillAyd requested changes Aug 29, 2018

View reviewed changes

WillAyd mentioned this pull request Aug 30, 2018

Travis 27 SciPy Failures #22538

Closed

alimcmaster1 closed this Aug 31, 2018

alimcmaster1 reopened this Aug 31, 2018

alimcmaster1 added 3 commits September 4, 2018 23:08

Avoid calling pytest fixtures directly

d6df77f

Add newline

bb905db

Rename method

816e855

WillAyd requested changes Sep 4, 2018

View reviewed changes

alimcmaster1 added 4 commits September 5, 2018 01:27

Remove final fixture warning

27c4133

Remove final fixture warning

0d8d239

Merge remote-tracking branch 'remotes/upstream/master' into pytest-fi…

13707e7

…xtures

Merge remote-tracking branch 'origin/pytest-fixtures' into pytest-fix…

7427924

…tures

alimcmaster1 mentioned this pull request Sep 6, 2018

Refactor test_sql.py #22624

Closed

WillAyd approved these changes Sep 7, 2018

View reviewed changes

gfyoung approved these changes Sep 7, 2018

View reviewed changes

jorisvandenbossche reviewed Sep 7, 2018

View reviewed changes

WillAyd mentioned this pull request Sep 11, 2018

BUG: Make sure that sas7bdat parsers memory is initialized to 0 (#21616) #22651

Merged

4 tasks

TomAugspurger mentioned this pull request Sep 13, 2018

TST: Fail on warning #22699

Merged

TomAugspurger merged commit b7e5704 into pandas-dev:master Sep 14, 2018

aeltanawy pushed a commit to aeltanawy/pandas that referenced this pull request Sep 20, 2018

Fix test_sql pytest fixture warnings (pandas-dev#22515)

7d6f275

* Avoid calling pytest fixtures directly

Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018

Fix test_sql pytest fixture warnings (pandas-dev#22515)

45283e0

* Avoid calling pytest fixtures directly

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix test_sql pytest fixture warnings #22515

Fix test_sql pytest fixture warnings #22515

alimcmaster1 commented Aug 26, 2018 •

edited

Loading

pep8speaks commented Aug 26, 2018 •

edited

Loading

codecov bot commented Aug 26, 2018 •

edited

Loading

alimcmaster1 commented Aug 26, 2018

gfyoung Aug 27, 2018

alimcmaster1 Aug 27, 2018

alimcmaster1 Aug 29, 2018

gfyoung commented Aug 29, 2018

WillAyd Aug 29, 2018 •

edited

Loading

alimcmaster1 Aug 30, 2018

alimcmaster1 Aug 30, 2018

WillAyd Aug 30, 2018

alimcmaster1 Aug 30, 2018

alimcmaster1 Sep 3, 2018

WillAyd Sep 3, 2018

alimcmaster1 Sep 3, 2018

WillAyd Sep 4, 2018

alimcmaster1 Sep 6, 2018

WillAyd Aug 29, 2018

alimcmaster1 Aug 29, 2018

alimcmaster1 commented Aug 31, 2018

gfyoung commented Aug 31, 2018

alimcmaster1 commented Sep 1, 2018

WillAyd left a comment

WillAyd Sep 4, 2018

alimcmaster1 commented Sep 6, 2018

WillAyd left a comment

gfyoung left a comment

jorisvandenbossche Sep 7, 2018

alimcmaster1 Sep 7, 2018

jorisvandenbossche Sep 7, 2018

TomAugspurger Sep 13, 2018

TomAugspurger Sep 13, 2018

alimcmaster1 Sep 13, 2018

TomAugspurger commented Sep 14, 2018

Fix test_sql pytest fixture warnings #22515

Fix test_sql pytest fixture warnings #22515

Conversation

alimcmaster1 commented Aug 26, 2018 • edited Loading

pep8speaks commented Aug 26, 2018 • edited Loading

Comment last updated on August 31, 2018 at 17:49 Hours UTC

codecov bot commented Aug 26, 2018 • edited Loading

Codecov Report

alimcmaster1 commented Aug 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfyoung commented Aug 29, 2018

WillAyd Aug 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alimcmaster1 commented Aug 31, 2018

gfyoung commented Aug 31, 2018

alimcmaster1 commented Sep 1, 2018

WillAyd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alimcmaster1 commented Sep 6, 2018

WillAyd left a comment

Choose a reason for hiding this comment

gfyoung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger commented Sep 14, 2018

alimcmaster1 commented Aug 26, 2018 •

edited

Loading

pep8speaks commented Aug 26, 2018 •

edited

Loading

codecov bot commented Aug 26, 2018 •

edited

Loading

WillAyd Aug 29, 2018 •

edited

Loading