/matplotlib

Pytest documentation + build tweaks #8026

Merged
merged 14 commits into from Feb 20, 2017
+200 −230

Conversation

Reviewers

@jenshnielsen jenshnielsen

@phobson phobson

@anntzer anntzer

@NelleV NelleV

@dstansby dstansby

@tacaswell tacaswell

Assignees
No one assigned
Labels
Documentation Testing
Projects

Done in Migrate to `pytest`

Milestone
  2.1 (next point r...
7 participants
@QuLogic @anntzer @madphysicist @NelleV @dstansby @jenshnielsen @phobson
@QuLogic
Member

QuLogic commented Feb 5, 2017 • edited

I've added the developer documentation for setting up pytest and removed a few more cases of nose in the setup, package metadata, etc.

For the tweaks:

  • Fix tests.py when run with a module path instead of a file path.
  • Close figures before running image_comparison because that decorator expects all figures when it's done to correspond to figures from the test, but if never checks for/ignores any already open figures.
  • Delete test_mouseclicks.py example because it's getting collected and run by pytest and since it's not really written as a test it causes an extra figure to be opened which breaks any image tests if run directly (modulo the previous fix.) Also copy the use of event.dblclick into the documentation so it's not gone completely.
  • Use importorskip everywhere so that the skip reasons are always the same, since it's used in some places and not others.

Fixes #8015.

@QuLogic QuLogic added Documentation Testing labels Feb 5, 2017

QuLogic added this to the 2.1 (next point release) milestone Feb 5, 2017

@QuLogic
Member

QuLogic commented Feb 5, 2017

Added one more tweak to use importorskip everywhere so that the skip reasons are always the same, since it's used in some places and not others.

QuLogic changed the title from Pytest tweaks to Pytest documentation + build tweaks Feb 6, 2017

@QuLogic
Member

QuLogic commented Feb 6, 2017

I've added the developer documentation for setting up pytest and removed a few more cases of nose in the setup, package metadata, etc.

QuLogic added to Needs Review in Migrate to `pytest` Feb 6, 2017

@anntzer
Contributor

anntzer commented Feb 6, 2017

Could we document the pytest interface rather than tests.py? I think it basically comes down to documenting -m "not network" instead of --no-network, the rest is the same.

After a quick look at tests.py, it also seems that there are some customizations (warnings filters, recursion limit) which should perhaps be moved to conftest.py?
tox.ini
@@ -12,7 +12,6 @@ commands =
sh -c 'rm -f $HOME/.matplotlib/fontList*'
@dstansby

dstansby Feb 6, 2017

Contributor

Add python version 3.6 to envlist above

@QuLogic

QuLogic Feb 18, 2017

Member

Added.

lib/matplotlib/tests/test_axes.py
- import pandas as pd
- except ImportError:
- pytest.skip("Pandas not installed")
+ pd = pytest.importorskip('pandas')
@dstansby

dstansby Feb 6, 2017

Contributor

Is there a minimum required version of pandas needed for the pandas tests to work?

@phobson

phobson Feb 6, 2017

Member

It doesn't appear that there is. Creating a Series from a list and DataFrame a from a dictionary of lists/arrays should work with versions of pandas several years old at this point.

examples/event_handling/test_mouseclicks.py
@@ -1,32 +0,0 @@
-from __future__ import print_function
@dstansby

dstansby Feb 6, 2017 • edited

Contributor

Also need to get rid of exammples/event_handling/test_mouseclicks.rst

@jenshnielsen

jenshnielsen Feb 6, 2017

Owner

Isn't that file autogenerated from the example? It's not in version control as far as I can see

@dstansby

dstansby Feb 6, 2017

Contributor

🤦‍♂️ so it is

doc/devel/testing.rst
-documentation for supported arguments. Some of the more important ones are given
-here:
+Additional arguments are passed on to pytest. See the pytest documentation for
+supported arguments. Some of the more important ones are given here:
@dstansby

dstansby Feb 6, 2017

Contributor

Might be good to put a link to the pytest documents that list supported arguments

doc/devel/testing.rst
- python tests.py matplotlib.tests.test_simplification:test_clipping
+ python tests.py matplotlib.tests.test_simplification::test_clipping
@dstansby

dstansby Feb 6, 2017

Contributor

Is there any point documenting the method where tests have to be installed, if the method below (which doesn't require installed tests) works?

@QuLogic

QuLogic Feb 6, 2017

Member

I don't know; does anyone use this method?

On further though, using non-installed tests might be broken when trying mpl_toolkits tests because they import stuff from matplotlib.tests. I can move things around to get that working again.

doc/devel/testing.rst
import matplotlib
matplotlib.test()
.. hint::
- To run the tests you need to install nose and mock if using python 2.7::
+ To run the tests you need to install pytest and mock if using python 2.7::
@dstansby

dstansby Feb 6, 2017

Contributor

Everything in this hint is covered above, so I think it can just be deleted

QuLogic added some commits Feb 3, 2017

@QuLogic QuLogic Document markers used by pytest. 
This may or may not become a requirement in the future.
 9d36e98
@QuLogic QuLogic TST: Close figures before image comparison. 
Since this decorator assumes that the figures available after are the
ones to check, it should close any existing figures or it will get the
wrong count.
 bdb8682
@QuLogic QuLogic Remove test_mouseclicks and copy some bits to docs. 
The name is getting collected as a test by pytest when it isn't one, and
it isn't much of an example anyway.
 b71447f
@QuLogic QuLogic Remove nose from tox environment. 113d079
@QuLogic QuLogic Use pytest.importorskip for consistent skip message. 
It's used in some places, but not all.
 c632e41
@QuLogic QuLogic Fix tests.py when passing modules instead of paths. d9fbe8d
@QuLogic QuLogic Update documentation to use pytest for testing. 244806d
@QuLogic QuLogic Change setup to require pytest instead of nose. 25192ea
@QuLogic QuLogic Remove multiprocess nose stuff. 50eaad6
@QuLogic QuLogic TST: Add 3.6 to tox.ini. 4529002
@QuLogic QuLogic Move conftest.py so it works with uninstalled tests. 5bc91c7
@QuLogic QuLogic Fix running of tests without installation. 
* Check for installed test data only if testing installed package.
* Remove relative import out of tests.
* Replace assert_str_equal with plain assert (pytest does diffing
  already).
 a4427a1
@QuLogic
Member

QuLogic commented Feb 16, 2017

I updated the documentation to follow the requests here and also moved some bits around so that tests work without being installed. Everything but the determinism test in the SVG backend works (because that one imports itself, which doesn't exist when not installed); not too sure how to fix that one, but it's not a high priority since not-installed tests never worked before.
@NelleV

NelleV reviewed Feb 16, 2017
View changes

Here are general comments on the documentation part.
This are minor comments and should not prevent the pull requests from being merged. These are just overall suggestions of improvements.

I'll review the rest of the PR.

INSTALL
@@ -106,12 +106,10 @@ or example code.
If you want to try the many demos that come in the matplotlib source
distribution, download the :file:`*.tar.gz` file and look in the
:file:`examples` subdirectory.
-To run the test suite, copy the :file:`lib\\matplotlib\\tests` and
+To run the test suite, extract the :file:`lib\\matplotlib\\tests` or
@NelleV

NelleV Feb 16, 2017

Contributor

I don't understand this sentence.

@QuLogic

QuLogic Feb 18, 2017

Member

I split things into bullet points; hopefully that's clearer.

README.rst
-more information. Note that the test suite requires nose and on Python 2.7 mock
-which are not installed by default. Please install with pip or your package
-manager of choice.
+more information. Note that the test suite requires pytest and on Python 2.7
@NelleV

NelleV Feb 16, 2017

Contributor

I think this sentence would be better with comas (for the understanding):
Note that the test suite requires pytest and, on Python 2.7, mock.
The "which are not installed by default seems redundant.

I would be happy to push a fix to your branch. Let me know if you prefer this option.

@QuLogic

QuLogic Feb 18, 2017

Member

Changed.

ci/conda_recipe/meta.yaml
@@ -30,7 +30,7 @@ requirements:
- freetype 2.6*
- msinttypes # [win]
- cycler >=0.10
- - nose
+ - pytest >=3.0.0
@NelleV

NelleV Feb 16, 2017

Contributor

👍

doc/devel/contributing.rst
- <https://docs.python.org/dev/library/unittest.mock.html>`_ (if python < 3.3), `Ghostscript
- <https://www.ghostscript.com/>`_, `Inkscape <https://inkscape.org>`_
+ **Additional dependencies for testing**: pytest_ (version 3.0 or later),
+ mock_ (if python < 3.3), Ghostscript_, Inkscape_
@NelleV

NelleV Feb 16, 2017

Contributor

👍

@dstansby

dstansby Feb 19, 2017

Contributor

Python 3 < 3.3 isn't supported but matplotlib, so might make more sense to change this to "if python 2.7"

doc/devel/testing.rst
-please ignore it while we consolidate our testing to these locations.)
-
-.. _nose: https://nose.readthedocs.io/en/latest/
+Matplotlib has a testing infrastructure based on pytest_, making it easy to
@NelleV

NelleV Feb 16, 2017

Contributor

Here are suggestions:

Matplotlib's testing infrastucture relies on pytest_. Test files are place in [path to the folder], and testing utility functions are in :mod:`matplotlib.testing`.

I'd drop the "making it easy to write new tests", as this is a judgement call that I also totally disagree with.
I'd also remove the "There is other old testing cruft around…"

@QuLogic

QuLogic Feb 18, 2017

Member

Changed; also switched to the file paths instead of modules since we're talking about developers who probably want to edit the existing files.

doc/devel/testing.rst
- information
- - `pep8 <http://pep8.readthedocs.io/en/latest>`_ to test coding standards
+ - pytest-cov_ to collect coverage information
+ - pytest-pep8_ to test coding standards
@NelleV

NelleV Feb 16, 2017

Contributor

I would add all the optional dependencies here:

  • pytest-xdist
  • pytest-timeout_
@QuLogic

QuLogic Feb 18, 2017

Member

Added both.

doc/devel/testing.rst
-Running the tests is simple. Make sure you have nose installed and run::
+Running the tests is simple. Make sure you have pytest installed and run::
+
+ py.test
@NelleV

NelleV Feb 16, 2017

Contributor

👍

@dstansby

dstansby Feb 19, 2017

Contributor

See above comment about py.test vs. pytest

doc/devel/testing.rst
+
+ py.test
+
+or::
python tests.py
@NelleV

NelleV Feb 16, 2017

Contributor

@tacaswell can we remove this from our documentation? :(

@anntzer

anntzer Feb 19, 2017

Contributor

I agree, attn @tacaswell.

doc/devel/testing.rst
- python tests.py matplotlib.tests.test_simplification:test_clipping
+ py.test lib/matplotlib/tests/test_simplification.py::test_clipping
@NelleV

NelleV Feb 16, 2017

Contributor

👍

doc/devel/testing.rst
- python tests.py matplotlib.tests.test_simplification:test_clipping
+ py.test lib/matplotlib/tests/test_simplification.py::test_clipping
+
+or, if tests are installed, a dot-separated path to the module, optionally
@NelleV

NelleV Feb 16, 2017

Contributor

I would remove this section. We only need to document one way to do this.

doc/devel/testing.rst
+
+An alternative implementation that does not look at command line arguments
+and works from within Python is to run the tests from the Matplotlib library
+function :func:`matplotlib.test`::
import matplotlib
matplotlib.test()
@NelleV

NelleV Feb 16, 2017

Contributor

I'd remove this section from our documentation. I think we should document (and have) only one way to run the tests.

attn @tacaswell (as I believe Thomas will disagree with me on that one).

doc/devel/testing.rst
- """
- fig = figure()
- ...
+Tests that have side effects that need to be cleaned up, such as created
@NelleV

NelleV Feb 16, 2017

Contributor

That paragraph is very unclear to me. Here is a suggestions.

"Some tests have side effects that need to be cleaned up after their execution (such as created figures or modifiec rc params). The pytest fixture XXX will automatically clean these up:

add short example showing how to use the fixture"

@QuLogic

QuLogic Feb 18, 2017

Member

There's no need for an example because the fixture gets used automatically; I've swapped the text around as suggested.

doc/users/event_handling.rst
@@ -29,13 +29,13 @@ connect your function to the event manager, which is part of the
example that prints the location of the mouse click and which button
was pressed::
- fig = plt.figure()
- ax = fig.add_subplot(111)
+ fig, ax = plt.subplots()
@NelleV

NelleV Feb 16, 2017

Contributor

👍

@madphysicist
Contributor

madphysicist commented Feb 16, 2017

Thanks for doing this. This is very helpful. Both the docs and switching to pytest. There is a bit of a learning curve with it, but it seems to be much more flexible than nose.

QuLogic added some commits Feb 18, 2017

@QuLogic QuLogic Update pytest documentation based on review. 
* Prefer py.test over tests.py
* Remove redundant hint
* Add link to further pytest documentation
* Tweak some sentence structure
 6a0c3ee
@QuLogic QuLogic TST: Remove extra Travis build. 
The only difference between the two 3.5 builds is one uses tests.py and
the other uses py.test. But the 2.7, 3.4, and macOS builds all use
tests.py, to this is a waste of resources.

Some checks were not successful

1 failing and 4 successful checks
codecov/patch — 72.13% of diff hit (target 80%)
Details
codecov/project/library — 62.29% (+<.01%) compared to 5e825bd
Details
codecov/project/tests — 97.96% (target 97.7%)
Details
continuous-integration/appveyor/pr — AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
29c1308
@QuLogic
Member

QuLogic commented Feb 18, 2017

I've removed an extra 3.5 build on Travis because it's pretty redundant; hopefully that will get us to finished CI just a little bit faster.

NelleV changed the title from Pytest documentation + build tweaks to [MRG+1] Pytest documentation + build tweaks Feb 18, 2017

@NelleV

NelleV approved these changes Feb 18, 2017
View changes

@NelleV
Contributor

NelleV commented Feb 18, 2017

LGTM 👍
doc/devel/testing.rst
-An alternative implementation that does not look at command line
-arguments works from within Python is to run the tests from the
-matplotlib library function :func:`matplotlib.test`::
+ PYTHONHASHSEED=0 py.test --verbose -n 5
@anntzer

anntzer Feb 19, 2017

Contributor

Can we just specify a minimum version of pytest-xdist where this is no longer an issue? It looks like it is related to https://bitbucket.org/pytest-dev/pytest/issues/346/pytest-xdist-and-python-33-is-sort-of, which has been resolved in 2013.

@QuLogic

QuLogic Feb 19, 2017

Member

Unfortunately, it's not fixed yet: pytest-dev/pytest#920

I just PR'd the same fix to the pandas build, so I know that recent versions still don't work.

QuLogic requested a review from tacaswell Feb 19, 2017

@dstansby

dstansby approved these changes Feb 19, 2017
View changes

I think this looks ready to go! I've just left a couple of comments for things that might need tweaking in the docs; let me know once you've read and/or changed them, and I will merge.

INSTALL
+ `mock <https://pypi.python.org/pypi/mock>`_, Pillow, MiKTeX, GhostScript,
+ ffmpeg, avconv, mencoder, ImageMagick, and `Inkscape
+ <https://inkscape.org/>`_;
+ * run ``py.test path\\to\\tests\\directory``.
@dstansby

dstansby Feb 19, 2017

Contributor
  • According to pytest-dev/pytest#1629 (comment) , pytest is preferred over py.test as of 3.0
  • I don't think you need the extra path\\to\\tests\\directory bit; if you do then line 23 of README.rst should be made consistent
@QuLogic

QuLogic Feb 20, 2017

Member

This is about separate uninstalled tests; line 23 in README.rst is about running from the full source directory.

README.rst
@@ -20,17 +20,16 @@ Testing
After installation, you can launch the test suite::
- python tests.py
+ py.test
@dstansby

dstansby Feb 19, 2017

Contributor

See previous comment about py.test vs. pytest

doc/devel/contributing.rst
- <https://docs.python.org/dev/library/unittest.mock.html>`_ (if python < 3.3), `Ghostscript
- <https://www.ghostscript.com/>`_, `Inkscape <https://inkscape.org>`_
+ **Additional dependencies for testing**: pytest_ (version 3.0 or later),
+ mock_ (if python < 3.3), Ghostscript_, Inkscape_
@dstansby

dstansby Feb 19, 2017

Contributor

Python 3 < 3.3 isn't supported but matplotlib, so might make more sense to change this to "if python 2.7"

doc/devel/testing.rst
-Running the tests is simple. Make sure you have nose installed and run::
+Running the tests is simple. Make sure you have pytest installed and run::
+
+ py.test
@dstansby

dstansby Feb 19, 2017

Contributor

See above comment about py.test vs. pytest

doc/devel/testing.rst
+or, if tests are installed, a dot-separated path to the module, optionally
+followed by the function separated by two colons, such as::
+
+ py.test --pyargs matplotlib.tests.test_simplification::test_clipping
If you want to run the full test suite, but want to save wall time try
@dstansby

dstansby Feb 19, 2017

Contributor

What is "wall time"?

@QuLogic

QuLogic Feb 20, 2017

Member

Time on a clock on the wall, as opposed to cpu time.

dstansby changed the title from [MRG+1] Pytest documentation + build tweaks to Pytest documentation + build tweaks Feb 20, 2017

@dstansby
Contributor

dstansby commented Feb 20, 2017

Since all the lines I commented on are technically true and easily changed later, I'm going to merge (finally!), thanks for all the work on this @QuLogic 👍

@dstansby dstansby merged commit 0d5a02a into matplotlib:master Feb 20, 2017
4 of 5 checks passed

4 of 5 checks passed

codecov/patch 72.13% of diff hit (target 80%)
Details
codecov/project/library 62.29% (+<.01%) compared to 5e825bd
Details
codecov/project/tests 97.96% (target 97.7%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

QuLogic deleted the QuLogic:pytest-tweaks branch Feb 20, 2017

anntzer referenced this pull request Feb 21, 2017

Closed

Deprecate assert_str_equal #8112

dstansby moved from Needs Review to Done in Migrate to `pytest` Mar 8, 2017

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