-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
Conversation
@@ -153,8 +153,3 @@ def test_long_path(): | |||
points = np.random.rand(70000) | |||
ax.plot(points) | |||
fig.savefig(buff, format='png') | |||
|
|||
|
|||
if __name__ == "__main__": |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
This is based on roughly a month-old |
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): Don't know where they're coming from yet. |
There are probably two problems. Travis is using |
# 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 |
There was a problem hiding this comment.
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.
Thanks for taking on this significant chunk of work. |
um, did I break travis? its not chewing after I fed it some more patches. |
Not sure -- there's nothing in the queue for matplotlib/matplotlib on Travis right now. @tacaswell, any thoughts? |
Not sure, other PRs seem to be building fine... |
When |
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
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 |
There was a problem hiding this comment.
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
Just a few more pointers. There are currently four entry points to running the tests.
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 |
|
||
[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 |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
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
and at least put them all in the one string. Haven't touched matplotlib.test() yet. |
Just a note about something that occurred to me: Another thing we should be able to improve with this change is the |
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) |
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). |
Also, awesome work. Thanks for taking on what looks like a rather nasty task. |
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? |
most likely human error with a long rebase process
human error that got through a long rebase procedure.
are not python!
ValueError is also valid for this test, due to the number of function arguments
"The Travis CI build passed" wohoo! |
It does not look like it is finding all of the tests. From travis:
from my machine running nose
There are like 1.5k tests missing |
but 👍 on progress! |
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
And so on.
There's code here to dynamically generate a lot of tests, but pytest is ignoring almost all of it. and I thought I was close to being done :-) oh well... |
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. |
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 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. |
@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.
Question: once this work is complete, will it affect downstream projects still using My guess is that won't |
It should not. Long term it would probably be a good idea to fork the testing framework code out of mpl it's self? |
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. |
as requested, making a WIP PR