Add ``test`` and ``test-coverage`` to Makefile #1470

Merged
merged 3 commits into from Nov 19, 2012

3 participants

@NelleV

I run the tests very often, and I find it useful to have shortcuts in the makefile to run them. I've also added a shortcut to run the test coverage.

Thanks,
N

@pelson
Matplotlib Developers member

Wow, didn't even know there was a make file! I can't see any harm in adding these additions, though I guess a similar thing should be done for python setup.py tests or python setup.py check in the future (perhaps without those bespoke arguments to nosetest).

👍

@NelleV

Before merging, I should probably update the developper documentation to mention the makefile.

The two commands you mention here fail on my computer, with the traceback error: invalid command 'check'
What are these suppose to do?

@pelson
Matplotlib Developers member

What are these suppose to do?

Nothing just yet.

The "test" command is fairly standard in distribute and setuptools (http://packages.python.org/distribute/setuptools.html).

The "check" command seems to be a standard thing and is defined for mpl's setup.py (as seen with python setup.py --help-commands), but hasn't been defined to run the tests.

HTH,

@NelleV

I've added the commands to the documentation and the README.

@dmcdougall
Matplotlib Developers member

Wow, didn't even know there was a make file! I can't see any harm in adding these additions, though I guess a similar thing should be done for python setup.py tests or python setup.py check in the future (perhaps without those bespoke arguments to nosetest).

+1e100

@pelson pelson commented on an outdated diff Nov 13, 2012
README.txt
For installation instructions and requirements, see the INSTALL file.
+
+Testing
+=======
+
+After installation, you can launch the test suite::
+
+ $ make test
@pelson
Matplotlib Developers member
pelson added a line comment Nov 13, 2012

For me, this is not the preferred way of running the tests (in the same way the Makefile is not the preferred way of building the source).
I don't have a problem with adding the "test" makefile target, but I think I would prefer this line to be python tests.py.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pelson pelson commented on an outdated diff Nov 13, 2012
doc/devel/coding_guide.rst
@@ -384,35 +384,39 @@ The following software is required to run the tests:
Running the tests
-----------------
-Running the tests is simple. Make sure you have nose installed and run
-the script :file:`tests.py` in the root directory of the distribution.
+Running the tests is simple. Make sure you have nose installed. There are two
+options to run tests in matplotlib:
+
+- use the makefile, and type ``make test`` or ``make test-coverage``
@pelson
Matplotlib Developers member
pelson added a line comment Nov 13, 2012

I'm not sure this change (i.e. 5 lines) is necessary. It confuses the issue a little (i.e. there is more than one way to do something).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pelson
Matplotlib Developers member

Hi @NelleV : if adding a test / test-coverage target to the makefile makes things easier for some people to work with mpl, then I am all for that. My only concern (as indicated inline) is that it means there are two ways to run the tests, and therefore the documentation starts to say things like: "to run the tests, you can either do X or you can do Y." rather than "to run the tests, do X". For me, the latter is far clearer, as when I see a choice I wonder what the pros and cons of that choice are...

Cheers,

@NelleV

I can revert the changes in the documentation. I don't have a strong opinion on that.

@pelson
Matplotlib Developers member

I can revert the changes in the documentation

Perhaps just the changes to coding_guide.rst and removing the make test line from readme.rst? Thanks @NelleV

@NelleV

I've reverted those, and rebased master.

@pelson
Matplotlib Developers member

@NelleV : Your patience with me is incredible - thank you and good stuff! 😄

@pelson pelson merged commit d0cf23d into matplotlib:master Nov 19, 2012

1 check passed

Details default The Travis build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment