Skip to content
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

V1.4.x - Remove test dependencies from install_requires #4188

Closed
wants to merge 1 commit into from

Conversation

HolgerPeters
Copy link
Contributor

See issue #4110 and #3659 reviving

Hope the rebase worked out

@tacaswell tacaswell added this to the next point release milestone Mar 4, 2015
@tacaswell
Copy link
Member

Can you also address all of the documentation related concerns from the original PR?

@HolgerPeters
Copy link
Contributor Author

I will update the docs shortly

@HolgerPeters
Copy link
Contributor Author

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 python setup.py test for the general invocation of tests.

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 tests.py.

@tacaswell
Copy link
Member

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 tests.py around as a compatibility layer (even if it just calls the setup.py version under the hood).

Does python setup.py test play nice with python setup.py develop?

Does setup.py get shipped when mpl is built by conda/linux packagers? Does this affect end-users of those packages ability to run the tests?

@HolgerPeters
Copy link
Contributor Author

  • python setup.py test installs test dependencies as eggs into the current directory runs setup.py egg_info, setup.py build_ext and then runs the tests. It does play nice with python setup.py develop in my experience (it is kind of made for the development solution). Key feature is, that it uses the tests_require option of setuptools for spelling out the dependencies of the test.
  • end users can still run matplotlib.test(), they might have to install nose as a dependency manually (with the corresponding conda install command) if it is not part of the base installation. I do not know whether setup.py is shipped with conda packages, Python wheels do not contain the setup.py (see https://www.python.org/dev/peps/pep-0427/#file-contents PEP 427). Keep in mind that this pull request was triggered by my issue Matplotlib Installing Test Dependencies #3649 -- I do not like that matplotlib lists a testing library in its install requirements when its a testing requirement.
  • I will restore tests.py

@@ -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
Copy link
Member

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?

Copy link
Member

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?

@HolgerPeters HolgerPeters changed the title V1.4.x V1.4.x - Remove test dependencies from install_requires Mar 9, 2015
@HolgerPeters
Copy link
Contributor Author

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.
Copy link
Member

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?

@WeatherGod
Copy link
Member

Travis completely failed to build the docs. There isn't even an index.html, it seems.

@jenshnielsen
Copy link
Member

@WeatherGod We are failing hard on Sphinx warnings when building the docs on Travis and it seems like this warning happens early on

@HolgerPeters
Copy link
Contributor Author

Will take a look at failing docs in my branch, however not before next week.

@jenshnielsen
Copy link
Member

@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.

@tacaswell
Copy link
Member

@HolgerPeters This needs a re-base again.

@HolgerPeters
Copy link
Contributor Author

@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.

@tacaswell
Copy link
Member

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

@HolgerPeters
Copy link
Contributor Author

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`::
Copy link
Member

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
@HolgerPeters
Copy link
Contributor Author

I just squashed the pull request as travis jobs have all built. Hope that you will merge it so #3649 is fixed.

@HolgerPeters
Copy link
Contributor Author

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"),
Copy link
Member

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?

Copy link
Contributor Author

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.

@tacaswell
Copy link
Member

Ping @HolgerPeters

@tacaswell
Copy link
Member

@HolgerPeters This was merged in #4668, closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants