-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
V1.4.x - Remove test dependencies from install_requires #4188
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
Can you also address all of the documentation related concerns from the original PR? |
I will update the docs shortly |
Should I restore the tests.py file for invocation of single tests and for the invocation of tests from an outside directory? People would not have to change their workflows and that script does not necessarily hurt. Of course we could parameterize the tests command further, but a setuptools command is limited, as all parameters have to be spelled out. Note that I am still very much in favor of My main interest in this pull requests is to remove the test dependencies from the install dependencies, so whatever you prefer would be fine in terms of |
The ability to run just a sub-set of the tests is a critical ability (given how long it takes to run the full test suite) for developers. It would be great to keep Does Does |
|
@@ -6,8 +6,7 @@ env: | |||
- secure: E7OCdqhZ+PlwJcn+Hd6ns9TDJgEUXiUNEI0wu7xjxB2vBRRIKtZMbuaZjd+iKDqCKuVOJKu0ClBUYxmgmpLicTwi34CfTUYt6D4uhrU+8hBBOn1iiK51cl/aBvlUUrqaRLVhukNEBGZcyqAjXSA/Qsnp2iELEmAfOUa92ZYo1sk= | |||
- secure: "dfjNqGKzQG5bu3FnDNwLG8H/C4QoieFo4PfFmZPdM2RY7WIzukwKFNT6kiDfOrpwt+2bR7FhzjOGlDECGtlGOtYPN8XuXGjhcP4a4IfakdbDfF+D3NPIpf5VlE6776k0VpvcZBTMYJKNFIMc7QPkOwjvNJ2aXyfe3hBuGlKJzQU=" | |||
- BUILD_DOCS=false | |||
- TEST_ARGS=--no-pep8 | |||
- NUMPY=numpy |
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.
Don't we need this for the build matrix to work right?
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.
Or, is line 26 working because something else in there depends on numpy?
I just rebased on latest master |
causing known-failing tests to fail for real. | ||
Running tests by any means other than `matplotlib.test()` does not | ||
load the nose "knownfailureif" (Known failing tests) plugin, causing | ||
known-failing tests to fail for real. |
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.
Is this statement still true?
Travis completely failed to build the docs. There isn't even an index.html, it seems. |
@WeatherGod We are failing hard on Sphinx warnings when building the docs on Travis and it seems like this warning happens early on |
Will take a look at failing docs in my branch, however not before next week. |
@HolgerPeters The build failure of the docs is due to the release of Sphinx 1.3 which broke some things and have nothing to do with your branch AFAIK. In current master we pin Sphinx at 1.2.3 (while we work on resolving the issue). Rebasing on current master is one way of fixing it. |
594c5eb
to
e22f1b0
Compare
@HolgerPeters This needs a re-base again. |
@tacaswell Will you merge this pull request once it has been rebased? I have rebased it before and it wasn't merged so I wouldn't want to rebase it now, without knowing that you have the intention to merge it. |
Sorry this is getting dragged out, everyone who works on MPL is doing it more-or-less on a volunteer basis so things happen at what ever pace they happen at. The delay in review is an issue of resources not malice. If we had no intention of merging this we would have just closed it initially. I suspect the conflict is in .travis.yml which we have been thrashing on |
Hmm, it seems that tests requirements are not installed in https://travis-ci.org/matplotlib/matplotlib/jobs/64324176 will try to fix this. |
|
||
|
||
An alternative implementation that does not look at command line | ||
arguments works from within Python:: | ||
arguments works from within Python is to run the tests from the | ||
matplotlibs library function :func:`matplotlib.test`:: |
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.
s/matplotlibs/matplotlib/
- add setup.py test command for test execution - modify setupext's logic for dependency determination for a test requirements list - add nose to test requirements - update documentation accordingly This reverts commit 3d622fe. install mock for dock building
I just squashed the pull request as travis jobs have all built. Hope that you will merge it so #3649 is fixed. |
Please merge or find another way to fix #3649 . I will not rebase this pull request a third time. |
description = "Invoke unit tests using nose after an in-place build." | ||
user_options = [ | ||
("pep8-only", None, "pep8 checks"), | ||
("omit-pep8", None, "Do not perform pep8 checks"), |
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 standard names? If not can you revert these to the flags we were using before?
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.
I wrote this so long ago I can hardly remember if I changed those names (I presume I did) . Probably I had the impression that the flags from the script runner were ambiguous.
Ping @HolgerPeters |
@HolgerPeters This was merged in #4668, closing. |
See issue #4110 and #3659 reviving
Hope the rebase worked out