TST: Use pytest

[TomAugspurger]

• closes pydata#13097
• tests added / passed
• passes git diff upstream/master | flake8 --diff
• whatsnew entry

Just a WIP for now, will need to iterate on the CI stuff to ensure that's all good.

What can I provide to make this easier to review? Refactoring tests is scary.
I'll give verbose outputs of nose on master and pytest on this branch.
junit.xml output too. Anything else?

Here are some notes I took along the way

Kinds of Changes

• Nosetests Generators to fixtures

See TestPickle, TestMsgPack and this SO post. We switched to using parametrized fixtures (yay) and dependency injection instead of setUp + a for loop inside the test.

• Different Discover Rules

See the sql tests. py.test discovers _Test* class (if they inherit from TestCase?), while unittest does not.
Solution: move TestCase inheritance down a level

• Assert Rewriting

See e.g. pandas/src/testing.py

• solution: rewrite assert cond, msg as if not cond; raise AssertionError(msg)
• looking into just excluding these files... They aren't actual tests, just test helpers.
• CI work:
  • updating nose to pytest

[jreback]

factoring asserts can be in a later PR

pytest will work with nose tools (though we mostly use our own tm.* ones)

[sinhrks]

Shouldn't this procedure remain?

[MaximilianR]

Thanks a lot @TomAugspurger !

One note on something above:

py.test discovers _Test* class (if they inherit from TestCase?), while unittest does not.

My understanding is that the only way to use pytest fixtures within a class is to not inherit from TestCase. This can make it a bit more difficult to gradually transition, but you can use autouse=True on existing setUp methods, and the inheritance then isn't necessary.

And pytest discovers tests in Test* named classes, not *Test, unless they inherit from TestCase.

Let me know if that's unclear.

[TomAugspurger]

factoring asserts can be in a later PR

Agreed, the only asserts I've manually rewritten were in src/testing.pyx to get the tests to pass. I believe this is due to pytest-dev/pytest#645, which has been fixed and will be included in pytest 3.0 which is coming out soonish. They were targeting Europython, but pushed it back.
I'll revert those changes and wait for pytest3 I think.

@MaximilianR thanks for the heads about autouse=True, I'll check that out. My initial goal was to have the test-suite runnable by either nose or pytest runners. Then followup PRs could make use of pytest features.
Doesn't look like I'll be able to achieve that though due to the nose generator tests.

[TomAugspurger]

pytest 3 is out now. Do we want to make the switch just after 0.19 is released?

[codecov-io]

Codecov Report

❗️ No coverage uploaded for pull request base (master@3d6fcdc). Click here to learn what that means.

@@            Coverage Diff            @@
##             master   #13856   +/-   ##
=========================================
  Coverage          ?   87.88%           
=========================================
  Files             ?      142           
  Lines             ?    51106           
  Branches          ?        0           
=========================================
  Hits              ?    44913           
  Misses            ?     6193           
  Partials          ?        0

Impacted Files	Coverage Δ
pandas/init.py	91.42% <100%> (ø)
pandas/util/testing.py	82.79% <100%> (ø)
pandas/util/_tester.py	46.15% <46.15%> (ø)
pandas/conftest.py	83.33% <83.33%> (ø)

---

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 3d6fcdc...59e2be9. Read the comment docs.

[jreback]

could just rename these

[jreback]

I would remove nose (so that it doesn't accidently run)

[TomAugspurger]

@jreback I think we should keep nose for now for things like SkipTest. I'll make a followup issue to clean all this up though (changing skips, removing generator-based tests, and using more fixtures are the big ones).

[jreback]

need to remove nose from any ci/requirements* files

[jreback]

don't use autouse its a bit magical, create explict fixtures (we can come back and fix this later though if easier)

[MaximilianR]

I think this might have been my suggestion.

Without this, I'm not sure setUp will run unless the class inherits from unittest.TestCase.

I think we could change setUp to [setup_class](http://doc.pytest.org/en/latest/xunit_setup.html) and it would run pytest. (Or setup_method if we need access to self rather than class)

[TomAugspurger]

@MaximilianR thanks, changed to use setup_class and all the tests run / pass. Made the same change in test_packers to remove autouse.

[jreback]

I would still inherit from our base class for tests (which is not that useful for pytest, but simplifes things when changing)

[TomAugspurger]

IIRC, the pytest runner either didn't discover real test subclasses below, or didn't properly run the setup and teardown methods. I needed to move the TestCase inheritance down to the actual Test* classes to get the tests to pass.

[MaximilianR]

pytest class names need to start with Test... IIRC

[MaximilianR]

@TomAugspurger A gentle reminder here - in my experience pytest won't run this class, because of its name. Not 100% sure, but at least worth double-checking it's run.

[TomAugspurger]

Thanks. This is the base class, there aren't any test_ methods attached to PandasSQLTest. All of the real test classes, which inherit from PandasSQLTest do start with with Test. IIRC the root problem was the setUp methods weren't being run with the prior inheritance. I'll post a fuller explanation later, because the diff isn't very informative.

[MaximilianR]

I see! Sorry.
FWIW I've generally called those ...Base rather than ...Test, to make that explicit. But only a personal preference. And in time fixtures can replace most of this inheritance

[jreback]

I would put these in conftest (and then just import into this name space)

[jreback]

go ahead and blow away all of the commented lines :>

[jreback]

same

[jreback]

@TomAugspurger is this working? (aside from the core dump in master).

issue is pd.test() which is still using the older code as well

I think nose needs to be removed from the ci/requirements* files as well

[TomAugspurger]

@jreback pretty close I think. The only ones I'm stumped on are the read_stata errors. You can see the failures in the last build here, This build from pre-segfaults has the tracebacks. I haven't been able to reproduce locally.

Other than that, I just have a bit of cleanup and verification that we didn't drop any tests. I'll do that tonight and on Sunday, hopefully get the merged soon. And the pd.test issue.

[TomAugspurger]

d653086 fixed pd.test() to use pytest. Are we OK with adding that pandas/api/test.py file?

Without pytest:

>>> import pandas
>>> pandas.test()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/tom.augspurger/Envs/py3/lib/python3.5/site-packages/pandas/pandas/api/test.py", line 13, in test
    raise ImportError("Need pytest>=3.0 to run tests")
ImportError: Need pytest>=3.0 to run tests

With pytest (until I canceled it):

>>> import pandas
>>> pandas.test()
...............^C

I'd like to do the complete removal of nose (skips and generator-based tests for the most part) as a followup issue.

[TomAugspurger]

@bashtage, do you have any guesses on what's causing this failure (scroll down a bit for the traceback)? The actual error comes from trying to lookup StataReader.DTYPE_MAP[0], which throws a KeyError (that we catch and re-raise). AFAICT, the 0 is coming from a null byte ord('\x00') == 0.

I suspect the problem is at a higher-level, since this passes with the nose runner, and it passes on other parts of our build matrix with pytest: https://travis-ci.org/pandas-dev/pandas/builds/175935337 Any hunches would be appreciated, but no worries if you're stumped.

[bashtage]

I don't recall having any issues there, so nothing obvious.

[jorisvandenbossche]

@TomAugspurger What's the status of this?
There are still few failures you didn't figure out? (the stata one? The others you know how to fix?)

[jorisvandenbossche]

not that important, but NOSE_ARGS -> (PY)TEST_ARGS ?

[bashtage]

@TomAugspurger I have found the problem with StataWriter. Date conversion is not robust to locale changes, and so when the file date is written, instead of 24 Jan 2017 12:10, 24 ???? 2017 12:10 is being writen, where ???? is some unicode that I can't quite decipher. Because this is inserting an extra byte, the rest of the header conversion is off.

Will need to either use a custom date converter for the month or to temporarily set the locale when writing stata files. Former is probably easier although maybe uglier.

[TomAugspurger]

@bashtage thanks! One of the other failing tests is locale related as well. Haven't been able to determine if it's an actual bug in pandas, or just something wrong with how we're setting up the test that pytest doesn't like.

[bashtage]

@TomAugspurger The reason why you found this is that you ran all tests with zh_CN. In nose testing this config only runs slow, and none of test_stata are slow, and so this bug was allowed to persist. I assume the other errors have occurred for the same reason.

[jreback]

@bashtage hmm, https://travis-ci.org/pandas-dev/pandas/jobs/194558929

DOES run all tests with a locale (IT and only on 2.7)

[jreback]

@TomAugspurger if its easier (not sure), you can certainly make an additional build that runs py.test, of course then these changes need to be compat with nose . not sure if this possible / desirable. Then could merge and iterate rather than switch over all at once.

[bashtage]

@bashtage hmm, https://travis-ci.org/pandas-dev/pandas/jobs/194558929
DOES run all tests with a locale (IT and only on 2.7)

But the short month for Italian is 3 non-unicode characters. For Chinese it is Unicode, which is why when writing the date as a string, Chinese requires an extra byte while Italian doesn't. I suspect that Chinese is a somewhat harder language to pass w.r.t. locale issues than most western languages.

edit

[jreback]

so does this make sense to flip the locale for these tests? (e.g. not-slow tests with chinese, e.g. more unicode), and IT for slow?

[bashtage]

I would think so.  Should be OK since the other locale testing is allowed failure.

[TomAugspurger]

In nose testing this config only runs slow,

Oh I guess I misunderstood our NOSE_ARGS. slow means only slow, not not all?
I can add another pytest option --only-slow to replicate that, if we want.

[jreback]

@bashtage

I would think so. Should be OK since the other locale testing is allowed failure.

no allowed failure is simply to order the builds. these shouldn't actually fail (its just easier to have the 5 main builds run first and give feedback).

[jreback]

@TomAugspurger yes we have essentially split tests, e.g. slow/non-slow. I think replicating that is a good ideal.

in fact we can eventually define more markers to split things further at some point

[bashtage]

no allowed failure is simply to order the builds. these shouldn't actually fail (its just easier to have the 5 main builds run first and give feedback).

Then if you switched them master would be failing.

It would probably make the most sense to run zh_CN with both slow and not slow configs so that all of pandas have to pass a more difficult locale.

[jreback]

#15211

though its on the 34_notslow

we try to cover everything.......but you know how that goes!

[jreback]

I don't think we actually have any disabled tests, so fine to remove this marker

but I think you need to make network a marker as well

[bashtage]

Have you tried using pytest-xdist on travis,

pip install pytest-xdist 
pytest n auto pandas`

Seeing nice speedup on slow statsmodels runs (18-20 min down to 11-13). Travis instances have 1.5 vCPUs.

[TomAugspurger]

Have you tried using pytest-xdist on travis,

Will try that as soon as I get the basic version passing. Down to one failure on a single job: https://travis-ci.org/pandas-dev/pandas/jobs/197031785#L1445 a locale test on 27_nslow

Any guess on why that is, before I dig into it? That job doesn't have any custom locale settings (and it passes fine locally).

Looking into the appveyor failures now.

[jreback]

@TomAugspurger you can simulate the locale locally easily

put in pandas/__init__.py

import locale
locale.setlocale(locale.LC_ALL, 'zh_CN.UTF-8')

[jreback]

ok, though very odd that this occurs only in pytest......let's put a note to try to remove this later

[TomAugspurger]

Yeah, very strange. Wasn't able to find anything about why that's happening, but that test / the .setlocale contextmanager relies on getlocale() being valid initially. I've started a list of followup issues, will add this.

[jreback]

@TomAugspurger all passing! that's great! and of course love the pytest output.

I would remove -s from the invocation (only on travis), as we sometimes have weird things being printed, though not too much.

I'll have a look soon at the changes.

did you fix pd.test()?

[TomAugspurger]

🎉

did you fix pd.test()?

I think so, but need to do a bit more testing here. I think that the appveyor testing proves that I have things setup correctly, since we cd to the root (outside of the pandas repo), and pd.test() is still able to find the conftest.py correctly, since we use the --skip-slow --skip-network options. AFAIK, we don't have to change the setup.py or anything, since it's just a regular python file inside the package.

[jreback]

some people use pytest --pyargs pandas and run from the outside (though I don't find that intuitve)

[jreback]

this should for sure be in the top-level and not here

[TomAugspurger]

I don't like where it's at now either, but I think it's necessary. As this note says:

This function [pytest_addoption] should be implemented only in plugins or conftest.py files situated at the tests root directory due to how pytest discovers plugins during startup.

The other option, I suppose is to bundle a setuptools entrypoint so that our custom options show up for anyone who has pytest and pandas. We could prefix our --run-slow, etc with --pandas-run-slow, to avoid name clashes.

Do you have a preference between the two?

[jreback]

i have never have a problem putting conftest in the root directory -

an entry point is for a separate program not sure that is needed here

[TomAugspurger]

Have you needed to distribute the conftest with the package though? Here's with conftest.py at the root, next to setup.py

python -c "import pandas; pandas.test(['--only-slow'])"                                                                                                            
usage: -c [options] [file_or_dir] [file_or_dir] [...]
-c: error: unrecognized arguments: --only-slow
  inifile: None
  rootdir: /Users/tom.augspurger/Envs/stitch/lib/python3.5/site-packages/pandas

If we don't care about allowing users to specify any extra options we have (or anything else we add to the conftest in the future), then this isn't a problem. pandas.test() works just fine with conftest.py in the root of the repo.

[jreback]

moving ``conftest``` to ~/pandas works fine for me (and the command above works just fine as well)

(pandas) bash-3.2$ py.test --skip-slow --skip-network
==================================================================================================== test session starts ====================================================================================================
platform darwin -- Python 3.5.2, pytest-3.0.5, py-1.4.31, pluggy-0.4.0
rootdir: /Users/jreback/pandas, inifile: setup.cfg
collected 11983 items / 2 skipped 

pandas/api/tests/test_api.py .........
pandas/computation/tests/test_compat.py .....
pandas/computation/tests/test_eval.py ssss...............s.ssss.........^C.....^C^C

[TomAugspurger]

I think where to put the conftest.py file depends on whether we want users to be able to pass options to pd.test()

• pandas.test() (no options) will always work, regardless of where conftest.py is.
• pandas.test(['--custom-option']) will only work if
  • conftest.py is distributed with the package (pandas/conftest.py)
  • conftest.py is at the git repo root, and you start the tests from within the git repo (which won't work for end users)

I was having trouble getting appveyor to work, since IIUC the package is installed into site-packages. I think this last commit, with conftest.py in the git root, should work though.

[jreback]

need to take this out

[jreback]

FYI, I think this might just work if you make PKG= PKG + '../..' IOW make it outside the package)

[TomAugspurger]

That would, but I think then pytest discovers "tests" in vb_suite, numpydoc, and scripts.

[jreback]

@TomAugspurger as soon as you get this fully working, we can merge it.

also add a whatsnew in highlites (but can do later if needbe)

[jorisvandenbossche]

Possibly that a rebase went wrong? (or not yet done?) as the diff seems to indicate that a full test_timeseries.py is added

Some small comments added.

[jorisvandenbossche]

Is it actually needed to keep those if __name__ == '__main__': parts? Does anybody use this?
(but can of course remove this in another PR if we want this)

[TomAugspurger]

I've never used it.

[jreback]

yeah I agree. I think we should do a separate PR to remove ALL of these (makes this PR a bit simpler too).

we don't advocate actually running indivdidual files, rather it is appropriate to use the test framework.

[TomAugspurger]

I have these all in a single commit. If someone (I'll have time tonight) wants to make a PR removing them, I should be able to delete that commit before rebasing, and there won't be any conflicts (hopefully).

[jorisvandenbossche]

the backtick at the end of pytest is incorrect, as the 'env' is closed with the backtick after the link on the following line

[jorisvandenbossche]

I would keep this in util (but maybe util/_tester.py instead of util/nosetester.py

In any case, IMO this is a strange place, as we currently use pd.api to put things from other places in a consistent place, not to put actual code for which the entry point for users is not here.

[jreback]

re the rebas:

FYI @TomAugspurger I did merge a few new files yesterday (and removed test_timeseries.py). sorry about that....

[TomAugspurger]

Fixed the rebase. I saw the new files, just forgot to remove the old one.

here's the problem I saw with the conftest earlier. Added a debug print to this next appveyor run, so we'll see.

I've addressed your other comments Joris, thanks.

[jorisvandenbossche]

The re-addition of test_timeseries.py is still in the diff, I think

[TomAugspurger]

The re-addition of test_timeseries.py is still in the diff, I think

It's a zombie module, won't stay dead. I've removed it for real now.

This run shows the trouble I'm having with appveyor. Dunno why, but the setup of

/pandas.git
  - setup.py
  - conftest.py
  - pandas/  # package

AFAICT, the same layout (running test from the git repo, but having the package installed to site-packages) works fine for me locally. Am I missing something obvious?

[bashtage]

Pytest will run setup but not setUp before each test in a class. Guessing this change in structure was intentional.

[bashtage]

You probably need something like python setup.py build_ext --inplace or python setup.py develop or you need to move to a directory somewhere outside of the one that has pandas below.

[TomAugspurger]

Last commit moved conftest.py back to the package, under pandas/conftest.py. I haven't been able to get appveyor to run the tests correctly. Since it's not an inplace install, we have to be outside the git repo. But AFAICT, you can't provide an explicit path to a conftest file, it has to be an imported module, so we have no way of specifying the --skip-slow option.

Should I change appveyor to be an inplace install?

[TomAugspurger]

Rebased on top of #15330. Only one tiny conflict thankfully.

[jreback]

I would remove this file entirely

[jreback]

i would remove this too

[bashtage]

Should NOSE_ARGS be changed to something neutral, e.g. TEST_ARGS

[bashtage]

I think the 124 instances of _multiprocess_can_split_ are nose-specific.

[jreback]

I think the 124 instances of multiprocess_can_split are nose-specific.

hmm, since we don't actually use parallel nose testing......

will take these out in a separate PR

[TomAugspurger]

Rebased on #15333

[jreback]

maybe add a link here to pytest (like you do in the docs)

[jreback]

I think we removed all of the disabled tests, yes?

[jreback]

same here

[jreback]

-> setup?

[jreback]

@pytest.fixture (as long as you are moving it)

[TomAugspurger]

Was gonna do fixtures as a follow up. Right now it's runnable under nose and pytest.

[jreback]

yep, np. Though I don't think its a big deal to completely drop nose (but sure can do later).

[jreback]

in fact fixture setup / changing to parameterized is actually quite a bit of work :> we have so many loop-y type tests which really should be parametrized.

[TomAugspurger]

One of the indexing tests had over 100 variations in a single test :D

[jreback]

yep, I think when you merge this can pretty much go crazy on params / fixtures.

[jreback]

@TomAugspurger

I have 2 PR's ready to go in (shortly): #15336 (groupby test) and #15324 (timedelta tests)

I don't actually think this will cause a rebase for you or if so should be trivial.

If you are close on this (appears so), then I will wait on future things (that change tests a lot) until we merge this (aside from these 2)?

[TomAugspurger]

AFAIK this is ready to go. The only lingering issue is conftest.py and whether it can / should go under pandas/conftest.py (currently it is).

Rebasing isn't too bad now that I don't touch every test file, so up to you when you want to merge these. I'll just keep rebasing when there are conflicts :)

[jreback]

ok, let me merge these and you rebase. I think let's put this in tomorrow (when you are ready). BTW did you change NOSE_ARGS -> TEST_ARGS? (or something).

yeah I really want conftest to be in the top-level (and not package namespace). But can fix that after.

[jreback]

I would instead recommend using -k, in fact this is one of the main reasons I really like pytest, e.g.

pytest pandas/[tests-module].py -k cool_test IMHO is much easiest then finding class names and the exact names of the methods.

[jreback]

@TomAugspurger all merged. rebase and let's get this in!

[jreback]

prob should take out this tag

[jreback]

on the todo list, let's just make @network an actual marker then this becomes trival

[jreback]

I think you might need to fix TestCase, IOW the setUpClass and such.

[jreback]

can you rebase and we will try for tomorrow for merging?

[TomAugspurger]

Rebased and travis is green. I'm under a deadline for Wednesday, so won't be able to start on followup issues till later next week.

[jreback]

ok thanks @TomAugspurger let me play a bit with this. I think we can just merge and fix anything else later.

[jreback]

thanks @TomAugspurger awesome job!

[jreback]

I made a couple of minor changes: 7713f29 and added a bit to the 'list' of TODOs, but otherwise merged very nicely!