Skip to content

improvements to install / testing [manually merge to master] #3659

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 12 commits into from

Conversation

HolgerPeters
Copy link
Contributor

This pull request addresses the issue #3649.

nose and mock are now listed as tests_require dependencies in the setup.py and automatically installed when tests are executed with python setup.py tests. Most of the code in tests.py has been moved to that test command in the setup.py

- require nose and mock for tests, not for normal installation
- implement a test command in setup.py for invoking tests
@tacaswell tacaswell added this to the v1.4.2 milestone Oct 17, 2014
@mdboom
Copy link
Member

mdboom commented Oct 17, 2014

Thanks for this. This is definitely an improvement.

However, there's a few aspects I'm not crazy about. This creates a yet another way to run the tests, which I'm already not crazy about. We have:

  • nosetests matplotlib.tests
  • python -c "import matplotlib; matplotlib.test()"
  • python tests.py
  • and now: python setup.py tests

Maybe replace tests.py so it simply delegates to setup.py tests? That would maintain backward compatibility without copying-and-pasting code. Ideally, we'd also merge the other duplication of this code in lib/matplotlib/__init__.py, but that's optional as at least not doing that is status quo.

Also, can you update the .travis.yml file to use this rather than tests.py? And drop the explicit installation of nose and mock from the .travis.yml so that we can test that what you've done here works (and stays working).

@HolgerPeters
Copy link
Contributor Author

@mdboom thanks for the feedback. I did not want to be too meddlesome for my first contribution ;) I will incorporate your feedback and amend the PR

@HolgerPeters
Copy link
Contributor Author

For a full replacement of test tests.py some more work seems to be necessary especially in terms of command line arguments (such as for number of processes, etc.). I am currently looking into whether this can be incorporated into the test command.

@jenshnielsen
Copy link
Member

Perhaps we should add pep8 to the test dependencies too?

@WeatherGod
Copy link
Member

yes. Btw, when did "tests_require" become available? I didn't see this
last year when I was reworking the setup.py to pull in numpy as a setup
requirement. Are we going to need to specify a minimum requirement for
setuptools?

On Fri, Oct 17, 2014 at 6:56 PM, Jens Hedegaard Nielsen <
notifications@github.com> wrote:

Perhaps we should add pep8 to the test dependencies too?


Reply to this email directly or view it on GitHub
#3659 (comment)
.

@tacaswell tacaswell changed the title V1.4.x improvements to install / testing Oct 19, 2014
@tacaswell
Copy link
Member

renamed the issue to make it clearer what is being done.

@HolgerPeters
Copy link
Contributor Author

@WeatherGod Apparently it became available in 0.6a9 https://pypi.python.org/pypi/setuptools

@WeatherGod
Copy link
Member

So, that's about what? 2 years ago, I guess?

On Mon, Oct 20, 2014 at 3:35 AM, Holger Peters notifications@github.com
wrote:

@WeatherGod https://github.com/WeatherGod Apparently it became
available in 0.6a9 https://pypi.python.org/pypi/setuptools


Reply to this email directly or view it on GitHub
#3659 (comment)
.

- specifiy parameters properly using user_options
- add parameters used by travis to be passed on to nose
@HolgerPeters
Copy link
Contributor Author

So I reworked the test command to truly parse parameters. So it seems travis failing stems from setup.py being called from an outside directory (and a quick glance at setup.py and setupext.py I think this is an assumption that is made on a few occasions in those files).

I do not know the reasons why the tests are called from an outside directory on travis, but I guess that there are reasons for this (would be great if someone could point them out to me).

I think making setup.py callable from the outside would trigger quite a few changes throughout the setup.py infrastructure of matplotlib.

@mdboom
Copy link
Member

mdboom commented Oct 21, 2014

The tests are called from an outside directory to ensure that what is imported is precisely what is installed (and not anything, accidentally, from within the source tree). It's probably not totally necessary for a project like matplotlib where the top-level of code in the source tree is lib and not matplotlib. It think the "go somewhere else first" comes historically from numpy, where not doing so is a problem. I'm fine with changing .travis.yml however it is necessary to make it work.

@mdboom
Copy link
Member

mdboom commented Nov 18, 2014

Sorry to take so long to get back to this. I think this is in great shape and I'm happy to merge it.

One detail, though. Although we want to get away from the python tests.py method to run the tests, we can't just out-and-out remove it in a bugfix release. We still need to provide a backward compatibility layer to support that (with a warning that it's deprecated) for at least one release.

Alternatively, we can move this to the master branch and target it for a 1.5 release.

@tacaswell: Any opinions?

@jenshnielsen
Copy link
Member

Just one small thing. We are also using mock now to build the documentation in addition to the tests so we should probably document that (along with Sphinx and numpydoc) now that it no longer is a install dependency.

@tacaswell
Copy link
Member

This is a pretty big change for a bug-fix release, but it does not touch any user code so I think it is OK.

I changed my mind a couple of times while writing this, but I think leave this where it is + add a back-compatibility shim, and then remove tests.py when it is merged to master is the way to go.

The case we want to avoid is needing one incantation for the maintenance branch and one incantation for the master branch, if we are going to move to python setup.py test , then we should do it all at once everywhere.

@pelson
Copy link
Member

pelson commented Nov 21, 2014

This is a pretty big change for a bug-fix release

I agree. This does not need to go into v1.4.x - we can readily put it straight into master though.

@tacaswell
Copy link
Member

@mdboom @efiring @WeatherGod

We need a decision on this, I am inclined to put this in for 1.4.3, @pelson wants to move this to master.

@efiring
Copy link
Member

efiring commented Jan 1, 2015

@tacaswell, I would move it to master and merge there ASAP, so it gets more usage before a release.

@tacaswell
Copy link
Member

@HolgerPeters Can you also update all of the documentation which refers to tests.py?

It also looks like we have lost the ability to run a single test:

(mpldev_py3k)tcaswell@eowyn:matplotlib$ python setup.py test matplotlib.tests.test_simplification:test_clipping
============================================================================
Edit setup.cfg to change the build options

BUILDING MATPLOTLIB
            matplotlib: yes [1.5.dev1]
                python: yes [3.4.2 |Continuum Analytics, Inc.| (default, Oct
                        21 2014, 17:16:37)  [GCC 4.4.7 20120313 (Red Hat
                        4.4.7-1)]]
              platform: yes [linux]

REQUIRED DEPENDENCIES AND EXTENSIONS
                 numpy: yes [version 1.9.1]
                   six: yes [using six version 1.9.0]
              dateutil: yes [using dateutil version 2.3]
                  pytz: yes [using pytz version 2014.10]
               tornado: yes [using tornado version 4.0.2]
             pyparsing: yes [using pyparsing version 2.0.3]
                libagg: yes [pkg-config information for 'libagg' could not
                        be found. Using local copy.]
              freetype: yes [version 2.4.10]
                   png: yes [version 1.5.13]
                 qhull: yes [Using system Qhull (version unknown, no pkg-
                        config info)]

OPTIONAL SUBPACKAGES
           sample_data: yes [installing]
              toolkits: yes [installing]
                 tests: yes [using nose version 1.3.4 / using unittest.mock]
        toolkits_tests: yes [using nose version 1.3.4 / using unittest.mock]

OPTIONAL BACKEND EXTENSIONS
                macosx: no  [Mac OS-X only]
                qt5agg: no  [PyQt5 not found]
                qt4agg: yes [installing, Qt: 4.8.5, PyQt: 4.8.5; PySide not
                        found]
               gtk3agg: no  [Requires pygobject to be installed.]
             gtk3cairo: no  [Requires cairocffi or pycairo to be installed.]
                gtkagg: no  [Requires pygtk]
                 tkagg: yes [installing, version not identified]
                 wxagg: no  [requires wxPython]
                   gtk: no  [Requires pygtk]
                   agg: yes [installing]
                 cairo: no  [cairocffi or pycairo not found]
             windowing: no  [Microsoft Windows only]

OPTIONAL LATEX DEPENDENCIES
                dvipng: yes [version 1.14]
           ghostscript: yes [version 9.10]
                 latex: yes [version 3.1415926]
               pdftops: yes [version 0.24.5]

invalid command name 'matplotlib.tests.test_simplification:test_clipping'

@tacaswell tacaswell modified the milestones: v1.5.x, v1.4.3 Jan 8, 2015
@tacaswell tacaswell changed the title improvements to install / testing improvements to install / testing [manually merge to master] Jan 8, 2015
@tacaswell tacaswell closed this Feb 17, 2015
@tacaswell
Copy link
Member

@HolgerPeters Can you rebase and re-target this at master?

Replaced by #4110

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

Successfully merging this pull request may close these issues.

7 participants