Skip to content

WIP: issue #5325, convert from nose to pytest #5405

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.

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

Closed
wants to merge 174 commits into from

Conversation

pizzathief
Copy link
Contributor

as requested, making a WIP PR

@@ -153,8 +153,3 @@ def test_long_path():
points = np.random.rand(70000)
ax.plot(points)
fig.savefig(buff, format='png')


if __name__ == "__main__":
Copy link
Member

Choose a reason for hiding this comment

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

Can these be replaced with a pytest equivalent ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the pytest equivilent is simply "py.test name_of_file" , nose couldn't do that, and needed the extra code, apparently.

@QuLogic
Copy link
Member

QuLogic commented Nov 5, 2015

This is based on roughly a month-old master. Please rebase because there are conflicts and the CIs don't run.

@pizzathief
Copy link
Contributor Author

so, most of test_coding_standards.py can go, and be replaced with the right settings in tox.ini, and installation of pytest.cov , pytest-pep8 , plus other plugins at https://pytest.org/latest/plugins_index/index.html . which ones do you want?

I'm getting a bunch of errors like this:

ERROR: Failure: AttributeError ('module' object has no attribute 'test_axes')

Traceback (most recent call last):
File "/home/travis/build/matplotlib/matplotlib/venv/lib/python2.7/site-packages/nose/loader.py", line 407, in loadTestsFromName
module = resolve_name(addr.module)
File "/home/travis/build/matplotlib/matplotlib/venv/lib/python2.7/site-packages/nose/util.py", line 322, in resolve_name
obj = getattr(obj, part)
AttributeError: 'module' object has no attribute 'test_axes'

Don't know where they're coming from yet.

@QuLogic
Copy link
Member

QuLogic commented Nov 5, 2015

There are probably two problems. Travis is using tests.py which imports nose and uses it to find and run tests, but you should replace that with just running py.test or similar. If you have a syntax error in a file, then it will not import and nose will say it doesn't "exist".

# https://pypi.python.org/pypi/pytest-cov for docs
- pip install pytest-cov
# https://pypi.python.org/pypi/pytest-flake8 for docs
- pip install pytest-flake8
Copy link
Member

Choose a reason for hiding this comment

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

flake8 finds a lot more problems than pep8 does as a tool. And we should carefully consider whether we want to add the burden of checking for those things at this time. That can always be a follow-on to this after some discussion.

Also, with respect to PEP8 checking, ensure that we're checking the same set of files (we don't check all of them) and ignoring the same set of violations.

@mdboom
Copy link
Member

mdboom commented Nov 5, 2015

Thanks for taking on this significant chunk of work.

@pizzathief
Copy link
Contributor Author

um, did I break travis? its not chewing after I fed it some more patches.

@mdboom
Copy link
Member

mdboom commented Nov 6, 2015

Not sure -- there's nothing in the queue for matplotlib/matplotlib on Travis right now. @tacaswell, any thoughts?

@tacaswell
Copy link
Member

Not sure, other PRs seem to be building fine...

@QuLogic
Copy link
Member

QuLogic commented Nov 6, 2015

When .travis.yml is not parseable, Travis will mysteriously ignore your PR.

@pizzathief
Copy link
Contributor Author

So it turned out to be the nose test line that I commented out, but didn't handle the indentation correctly.

py.test is trying to run all the tests now, its just that it can't handle

from matplotlib import _path

properly. _path seems to be a c .so file ? according to searching I've done so far. Any idea what to do to solve this?

@@ -100,7 +109,8 @@ script:
- |
if [[ $BUILD_DOCS == false ]]; then
export MPL_REPO_DIR=$PWD # needed for pep8-conformance test of the examples
gdb -return-child-result -batch -ex r -ex bt --args python tests.py --processes=$NPROC --process-timeout=300 $TEST_ARGS
# gdb -return-child-result -batch -ex r -ex bt --args python tests.py --processes=$NPROC --process-timeout=300 $TEST_ARGS
py.test --pep8 --cov=matplotlib ./lib/matplotlib/tests
Copy link
Member

Choose a reason for hiding this comment

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

In reference to your question about the matplotlib._path C++ extension not loading...

Here, you are running the tests from the source directory, not the installed directory (which usually ends up under site-packages). I think if you give py.test an import path, rather than a file path, it should import matplotlib from the installed directory, i.e.:

py.test matplotlib.tests

@mdboom
Copy link
Member

mdboom commented Nov 7, 2015

Just a few more pointers.

There are currently four entry points to running the tests.

  1. The tests.py script.
  2. import matplotlib, matplotlib.test()
  3. nosetests matplotlib.tests
  4. python setup.py test

There was recent discussion to get rid of (4) -- there's just too many unresolvable problems with it (See #5315). But I think we'll want to keep the others all working. Option (3) has historically been problematic because it doesn't do any of the test setup and sanity checking at the start of the run that the other two do (see matplotlib.__init__.py:test to see what sort of startup stuff we do). With py.test one can include a conftest.py file to include that kind of startup stuff, so we should (I say hand-wavingly) move most of of matplotlib.test does into conftest.py, and then reduce matplotlib.test to the bare minimum required to start up py.test.


[pep8]
ignore=E111,E114,E115,E116,E121,E122,E123,E124,E125,E126,E127,E128,E129,E131,E265,E266,W503
exclude=_delaunay.py,_image.py,_tri.py,_backend_agg.py,_tkagg.py,ft2font.py,_cntr.py,_contour.py,_png.py,_path.py,ttconv.py,_gtkagg.py,_backend_gdk.py,pyparsing*,_qhull.py,_macosx.py
Copy link
Member

Choose a reason for hiding this comment

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

Are these exclusions picked up when running the tests outside of tox?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PEP8-check(ignoring E111 E114 E115 E116 E121 E122 E123 E124 E125 E126 E127 E128 E129 E131 E265 E266 W503)

they are now. output log is over 4M, so I'm looking at means of making the log smaller.

@pizzathief
Copy link
Contributor Author

tests.py now has py.test() go through each file in matplotlib.default_test_modules. its testing each entry seperately, I'm seeing if I can specify

pytest.main(" --pyargs matplotlib.tests.test_animation matplotlib.tests.test_basic")

and at least put them all in the one string.

Haven't touched matplotlib.test() yet.

@mdboom mdboom changed the title WIP: issue 5325, convert from nose to pytest WIP: issue #5325, convert from nose to pytest Nov 12, 2015
@mdboom
Copy link
Member

mdboom commented Nov 12, 2015

Just a note about something that occurred to me: Another thing we should be able to improve with this change is the @cleanup decorator. We currently put that (or @image_comparison, which calls cleanup internally) on all tests to make sure that the tests' state doesn't impact other tests run subsequently. py.test has a hook to run a snippet of code before every test, so we should be able to add cleanup there and then not requiring the explicit @cleanup decorator on every test. That would solve a whole class of bugs where someone forgets to put @cleanup on and things work sometimes but not others.

@pizzathief
Copy link
Contributor Author

This https://pytest.org/latest/xunit_setup.html seems to cover the ways to replace @cleanup with a more pytestified (yes, its a word I just made up) way of doing things. Given that the cleanup decorator seems to store state (the settings registry copy) and then restore it again after its finished, I'm not sure if its possible to replicate this with setup_function() and teardown_function(). (at least , without a global somewhere, and that would get ugly if you tried to multithread tests, etc)

In other news, rebasing without updating master first was not my best move, took ages, but rebasing eventually worked, and I seem to get nice output from using tests.py now.

Do you have a list of all the things that you'd expect all this to do before merging it ? (to make sure I don't miss anything)

@tacaswell
Copy link
Member

It looks like the pep8 restrictions are not getting passed through (as the 3.6 test is failing on pep8 not being 3.6 compatible).

@tacaswell
Copy link
Member

Also, awesome work.

Thanks for taking on what looks like a rather nasty task.

@pizzathief
Copy link
Contributor Author

I'm thinking of things like removal of matplotlib.tests.test_coding_standards, if all its doing is running pep8 over the default modules, then its probably not needed any more.

Did all the tests pass before? If they did then I'll need to fix that up, as a few of them are failing.

Can the log length be increased so that it doesn't force me to download the log to see it all?

@pizzathief
Copy link
Contributor Author

"The Travis CI build passed" wohoo!

@tacaswell
Copy link
Member

It does not look like it is finding all of the tests. From travis:

============= 3574 passed, 6 skipped, 28 xfailed in 84.13 seconds ==============
[Inferior 1 (process 16887) exited normally]
No stack.

from my machine running nose

----------------------------------------------------------------------
Ran 5166 tests in 83.836s

FAILED (KNOWNFAIL=19, failures=1)

There are like 1.5k tests missing

@tacaswell
Copy link
Member

but 👍 on progress!

@pizzathief
Copy link
Contributor Author

ok, a good example of whats going on here is with test_mathtest.py

If I run tests from the master branch (ie nose), I get

FAIL: matplotlib.tests.test_mathtext.test_mathtext_stix_77.test
FAIL: matplotlib.tests.test_mathtext.test_mathtext_stix_78.test
FAIL: matplotlib.tests.test_mathtext.test_mathtext_stix_80.test
...
FAIL: matplotlib.tests.test_mathtext.test_mathtext_stixsans_77.test
FAIL: matplotlib.tests.test_mathtext.test_mathtext_stixsans_78.test
FAIL: matplotlib.tests.test_mathtext.test_mathtext_stixsans_80.test

And so on.
but with pytest I get

collected 3 items 
. ...
 3 passed, 1 pytest-warnings in 0.60 seconds

There's code here to dynamically generate a lot of tests, but pytest is ignoring almost all of it.
So I'll either have to @pytest.parametrize things, or use pytest_generate_tests

and I thought I was close to being done :-) oh well...

@tacaswell
Copy link
Member

Unfortunately this is not a project where we can merge merge it in pieces and you are going to continuously get conflicts from us adding tests.

@mdboom
Copy link
Member

mdboom commented Jan 25, 2016

There's code here to dynamically generate a lot of tests, but pytest is ignoring almost all of it.
So I'll either have to @pytest.parametrize things, or use pytest_generate_tests

py.test also allows a generator to generate tests (just like nose), but I suspect there is just some difference in the details here. I don't think @pytest.parameterize will be necessary here -- or at least shouldn't be tried until the generator approach has been exhausted.

Sorry this has been such a long slog. Your work is very appreciated. I think you probably understand now why most of the other devs have avoided this for so long ;) But it will be really nice when it's done.

@jankatins
Copy link
Contributor

@pizzathief Could you add a small commit so that this is tested on the new appveyor windows infrastructure?

functions that have the image_comparison decorator.
After asking on irc, seems this is the way to do it.
Just checking in what I have so I can rebase again.
@phobson
Copy link
Member

phobson commented May 13, 2016

Question: once this work is complete, will it affect downstream projects still using @image_comparison and nose?

My guess is that won't

@tacaswell
Copy link
Member

It should not. Long term it would probably be a good idea to fork the testing framework code out of mpl it's self?

@QuLogic
Copy link
Member

QuLogic commented Feb 5, 2017

This seems to have fallen quite out of date, and is rather too large to even review as a whole. We've mostly implemented all of these changes in smaller PRs. Sorry, @pizzathief, I'm going to have to close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants