TST: Parametrize simple yield tests #15406

QuLogic · 2017-02-15T00:30:18Z

Simple yield tests are those that do for x in y: yield test, x where y is some constant list/tuple. It also requires the tests to be in a simple class that doesn't subclass unittest's classes (because parametrization doesn't work there).

There are still yield tests in

frame/test_query_eval.py
io/test_packers.py
io/test_pickle.py
test_internals.py
test_window.py

but I haven't changed them since they don't conform to the requirements above.

This is also not an attempt to parametrize any and all test loops.

cf pytest followup #15341
tests added / passed
passes git diff upstream/master | flake8 --diff
whatsnew entry

QuLogic · 2017-02-15T00:43:14Z

pandas/tests/io/parser/test_network.py

@@ -31,24 +31,18 @@ def setup(self):
                         'pandas/io/tests/parser/data/salaries.csv')


This test might be broken; I don't see this file, with or without the compressed extension.

i fixed this in master - make sure to rebase

actually u r right
let me push a fix

#15407 pushing shortly

jreback · 2017-02-15T00:44:53Z

pandas/tests/computation/test_compat.py

@@ -11,8 +11,6 @@
 from pandas.computation.engines import _engines
 import pandas.computation.expr as expr

-ENGINES_PARSERS = list(product(_engines, expr._parsers))


might be cleaner to make this a fixture

then parameterize

There's only one parametrized function in this file, so maybe it's not so useful here.

jreback · 2017-02-15T00:45:14Z

pandas/tests/computation/test_eval.py

@@ -774,16 +774,19 @@ def check_chained_cmp_op(self, lhs, cmp1, mid, cmp2, rhs):
 f = lambda *args, **kwargs: np.random.randn()


-ENGINES_PARSERS = list(product(_engines, expr._parsers))
-


Could be useful here though.

I've made the change, but I'll wait until the network test is corrected before updating.

@QuLogic thanks.

just testing my fix out now

TomAugspurger · 2017-02-15T00:54:32Z

@QuLogic thanks for starting on this!

If it helps, you can (temporarily) remove the line

addopts = --disable-pytest-warnings

in setup.cfg so that the warnings aren't suppressed.

QuLogic · 2017-02-15T01:02:09Z

Thanks @TomAugspurger; somehow I'm still getting warnings anyway (in most cases).

max-sixty · 2017-02-15T01:38:08Z

pandas/tests/computation/test_eval.py

-
-    def check_binop_typecasting(self, engine, parser, op, dt):
+    @pytest.mark.parametrize('engine', _engines)
+    @pytest.mark.parametrize('parser', expr._parsers)


Instead of repeating @pytest.mark.parametrize('parser', expr._parsers), you could have a single fixture in the module, and pytest will push those through to each testing function:

@pytest.fixture(params=_engines) def engine(request): return request.param

Yes; see above comment.

Yup - to be clear - with this you could remove every line that's @pytest.mark.parametrize('parser', expr._parsers) in this file - I think about 25 lines

jreback · 2017-02-15T02:47:12Z

@QuLogic ok merged. give your changes a rebase and try.

jreback · 2017-02-15T02:48:36Z

pandas/tests/io/parser/test_network.py

+    @pytest.mark.parametrize('mode', ['explicit', 'infer'])
+    @pytest.mark.parametrize('engine', ['python', 'c'])
+    def test_compressed_urls(self, compression, extension, mode, engine):
+        if extension == '.xz':


oh you changed this too. fine with your changes as well (of course the tm.network issue still there though).

jreback · 2017-02-15T13:43:22Z

@QuLogic I think you have a linting error and

Different tests were collected between gw1 and gw0. The difference is: on the OSX test, very odd.

jreback · 2017-02-15T20:37:50Z

FYI the warnings are now showing on windows (had to fix the build for other reasons):

https://ci.appveyor.com/project/jreback/pandas/build/1.0.3009/job/4xo1hvnr0kb2jjo6

so in next PR should fix these :<

QuLogic · 2017-02-15T21:43:21Z

Oops, I guess flake8 --diff doesn't catch that sort of linting error.

The xdist thing is a bug in pytest-xdist pytest-dev/pytest#920; we have this workaround.

jreback · 2017-02-15T21:46:58Z

@QuLogic ahh if you want to add that work-around for xdist to ci/scripts_multiple.py would be fine.

jreback · 2017-02-15T21:48:16Z

yeah the flake checker on travis picks up these types of things but for some reason it doesn't locally. not sure why.

max-sixty · 2017-02-15T22:00:22Z

pandas/tests/computation/test_eval.py

+                       reason='numexpr enabled->{enabled}, '
+                              'installed->{installed}'.format(
+                            enabled=_USE_NUMEXPR,
+                            installed=_NUMEXPR_INSTALLED))(engine)


Cosmetic + not worth changing, but for future pytesters - this sort of thing can go in the function

@MaximilianR can you show an example of what you mean?

@pytest.fixture(params=_engines) def engine(request): if engine == 'numexpr' and not _USE_NUMEXPR: pytest.skip(reason='numexpr enabled->{enabled}, ' 'installed->{installed}'.format( enabled=_USE_NUMEXPR, installed=_NUMEXPR_INSTALLED))(engine) return request.param

sure that's reasonable.

I have always tried to see if there is a way to make a parameterized yielder

def create_things(): return list(range(5)) @pytest.mark.parametrize('t', create_things()) def test_thing(t): ...

where I really want to do:

@pytest.fixture def things(request): for in i range(5): yield i @pytest.mark.parametrize('t', things): def test_thing(t): ....

note that this is NOT a yield fixture, but something that I need a function to generate the 'things' that I want to parametrize (rather than a simple list of strings).

I think pytest_generate_tests hook would do it, but seems overly complicated.

Yes I very much agree - I've had the same inclination for a while now

I guess you could have:

def things(): for in i range(5): yield i @pytest.mark.parametrize('t', things()) def test_thing(t): ....

but then it's not a fixture, which defeats it a bit

Ideally I'd want:

@pytest.fixture def things(): for in i range(5): yield i def test_thing(things): ....

@MaximilianR There is a slight difference in your method; the one I have will mark the skip at collection time, while yours requires running the fixture to find out about the skip. It's a rather minor difference though.

Also, @jreback your first code block works fine. In fact, pytest will understand the generator, so you don't need the list:

def create_things(): return range(5) @pytest.mark.parametrize('t', create_things()) def test_thing(t): ...

works fine.

thanks @QuLogic just tried it out ....looks much nicer!

max-sixty · 2017-02-15T22:02:22Z

pandas/util/testing.py

    try:
        return compat.import_lzma()
    except ImportError:
-        import pytest
-        pytest.skip('need backports.lzma to run')
+        return False


Same as above re cosmetic
But pytest has its own importorskip which can do these

yes these should be changed to use pexpect = pytest.importorskip('pexpect')
should be simpler

Can't use it here because there's a 2 vs 3 import in compat, though I guess it could be in an if PY3 instead of try/except like it is now.

Simple meaning those that loop through a constant and are not in a unittest class.

jreback · 2017-02-16T01:20:00Z

@QuLogic this looks good. windows completed, so ping on green on travis.

jreback · 2017-02-16T01:20:51Z

https://travis-ci.org/pandas-dev/pandas/jobs/202066838

hmm, can you add in the PYTHONHASHSEED setting? (ci/scripts_multiple.py)

codecov-io · 2017-02-16T06:14:08Z

Codecov Report

Merging #15406 into master will increase coverage by <.01%.
The diff coverage is 80%.

@@            Coverage Diff             @@
##           master   #15406      +/-   ##
==========================================
+ Coverage   90.36%   90.37%   +<.01%     
==========================================
  Files         135      135              
  Lines       49438    49440       +2     
==========================================
+ Hits        44675    44680       +5     
+ Misses       4763     4760       -3

Impacted Files	Coverage Δ
pandas/util/testing.py	`82.09% <80%> (+0.31%)`	✅

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 d6f8b46...b002752. Read the comment docs.

jreback · 2017-02-16T14:18:03Z

thanks @QuLogic nice fixes!

keep em coming!

QuLogic · 2017-02-16T22:21:49Z

Do you squash locally or something? Seems odd PR is closed instead of merged...

jreback · 2017-02-16T23:35:31Z

no this is merged !

you don't need to squash generally as it's done in merge

jreback · 2017-02-16T23:35:50Z

ddb22f5

jreback · 2017-02-17T20:02:06Z

@QuLogic looks like: frame/test_query_eval.py is not giving any yield errors AFAICT

QuLogic · 2017-02-19T00:40:50Z

It does use yield, but I think because they're in classes subclassed from tm.TestCase(which is a unittest.TestCase subclass), they don't cause any warnings. Being such a subclass also means you can't use pytest.parametrize anyway, but it's something to keep in mind if tm.TestCase is modified.

jreback · 2017-02-20T14:46:28Z

@QuLogic ahh ok. yeah the need for subclassing tm.TestCase is much less (as we needed certain things like self.assertTrue to be unified).

xref pandas-dev#15341 Author: Elliott Sales de Andrade <[email protected]> Closes pandas-dev#15406 from QuLogic/pytest-simple-yield and squashes the following commits: b002752 [Elliott Sales de Andrade] TST: Set PYTHONHASHSEED so xdist doesn't break. 8368772 [Elliott Sales de Andrade] TST: Use fixtures for engine/parser where possible. c6cd346 [Elliott Sales de Andrade] TST: Parametrize remaining simple yield tests. 47bf1a1 [Elliott Sales de Andrade] TST: Replace ENGINES_PARSERS by parametrize.

QuLogic commented Feb 15, 2017

View reviewed changes

jreback requested changes Feb 15, 2017

View reviewed changes

jreback added the Testing pandas testing functions or related to the test suite label Feb 15, 2017

max-sixty reviewed Feb 15, 2017

View reviewed changes

jreback reviewed Feb 15, 2017

View reviewed changes

QuLogic force-pushed the pytest-simple-yield branch 2 times, most recently from 47756be to a1ad150 Compare February 15, 2017 05:48

jorisvandenbossche changed the title ~~Parametrize simple yield tests~~ TST: Parametrize simple yield tests Feb 15, 2017

jreback approved these changes Feb 15, 2017

View reviewed changes

jreback added this to the 0.20.0 milestone Feb 15, 2017

TomAugspurger mentioned this pull request Feb 15, 2017

pytest followup #15341

Closed

17 tasks

max-sixty reviewed Feb 15, 2017

View reviewed changes

QuLogic added 3 commits February 15, 2017 18:25

TST: Replace ENGINES_PARSERS by parametrize.

47bf1a1

TST: Parametrize remaining simple yield tests.

c6cd346

Simple meaning those that loop through a constant and are not in a unittest class.

TST: Use fixtures for engine/parser where possible.

8368772

QuLogic force-pushed the pytest-simple-yield branch from a1ad150 to 8368772 Compare February 15, 2017 23:26

TST: Set PYTHONHASHSEED so xdist doesn't break.

b002752

QuLogic force-pushed the pytest-simple-yield branch from d1e5bcd to b002752 Compare February 16, 2017 06:14

jreback closed this in ddb22f5 Feb 16, 2017

QuLogic deleted the pytest-simple-yield branch February 16, 2017 22:21

		@@ -31,24 +31,18 @@ def setup(self):
		'pandas/io/tests/parser/data/salaries.csv')

		@@ -774,16 +774,19 @@ def check_chained_cmp_op(self, lhs, cmp1, mid, cmp2, rhs):
		f = lambda args, *kwargs: np.random.randn()


		ENGINES_PARSERS = list(product(_engines, expr._parsers))

TST: Parametrize simple yield tests #15406

TST: Parametrize simple yield tests #15406

Conversation

QuLogic commented Feb 15, 2017 • 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

TomAugspurger commented Feb 15, 2017

QuLogic commented Feb 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Feb 15, 2017

Choose a reason for hiding this comment

jreback commented Feb 15, 2017

jreback commented Feb 15, 2017

QuLogic commented Feb 15, 2017

jreback commented Feb 15, 2017

jreback commented Feb 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

max-sixty Feb 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

max-sixty Feb 15, 2017 • 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

QuLogic Feb 15, 2017 • edited Loading

Choose a reason for hiding this comment

jreback commented Feb 16, 2017

jreback commented Feb 16, 2017

codecov-io commented Feb 16, 2017 • edited Loading

Codecov Report

jreback commented Feb 16, 2017

QuLogic commented Feb 16, 2017

jreback commented Feb 16, 2017

jreback commented Feb 16, 2017

jreback commented Feb 17, 2017

QuLogic commented Feb 19, 2017

jreback commented Feb 20, 2017

QuLogic commented Feb 15, 2017 •

edited

Loading

max-sixty Feb 15, 2017 •

edited

Loading

max-sixty Feb 15, 2017 •

edited

Loading

QuLogic Feb 15, 2017 •

edited

Loading

codecov-io commented Feb 16, 2017 •

edited

Loading