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

PR: Add testing requirements to setup.py #4185

Merged
merged 8 commits into from
Mar 8, 2017

Conversation

rlaverde
Copy link
Member

Fixes: #4174


I take the packages that I found in the continuum integration recipes.

I'm not sure if numpy should also be added (pandas will install it as a dependency), and if pillow if needed (circle-ci recipe doesn't install it)

@rlaverde rlaverde added this to the v3.1.4 milestone Feb 22, 2017
@rlaverde rlaverde self-assigned this Feb 22, 2017
setup.py Outdated
@@ -288,8 +288,15 @@ def run(self):
'numpydoc',
]

extra_require = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra_require -> extras_require

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The failure in Travis is caused because of this.

setup.py Outdated
@@ -288,8 +288,15 @@ def run(self):
'numpydoc',
]

extra_require = {
'test:python_version == "2.7"': ['mock'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this syntax work? I'm just curious and I'd like to understand how it works :-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only found that documented in the wheels docs

The idea is that you could run:
pip install spyder[test]
and It will install this extra dependencies

@ccordoba12
Copy link
Member

I'm not sure if numpy should also be added (pandas will install it as a dependency), and if pillow if needed (circle-ci recipe doesn't install it)

Ok, no problem about Numpy., i.e. you can remove it.

Pillow is used to generate image representations of arrays in the Variable Explorer. So please add a test for that and leave it as a test dependency ;-)

@rlaverde rlaverde force-pushed the required-test-packages branch from d11acb9 to 4403069 Compare February 23, 2017 14:36
@ccordoba12
Copy link
Member

Ok, good. Now please modify our Travis scripts to install our wheel with [test], then install pyqt5 (i.e. remove our conda install of pyqt5 for pip) and finally run our pytest's there.

The idea is to use our Travis pip slot to test the latest PyQt 5 (which is 5.8 right now) because conda is going to stay at 5.6 for a loong time ;-)

setup.py Outdated
extras_require = {
'test:python_version == "2.7"': ['mock'],
'test': ['pytest', 'pytest-qt', 'pytest-cov', 'mock', 'coveralls',
'ciocheck', 'flaky', 'pandas', 'scipy', 'sympy', 'pillow'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ciocheck is not in PiPy. Please remove it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But Travis is complaining about the lack of a package for ciocheck.

In any case, pytest doesn't run ciocheck, so it's not really a test dependency.

@@ -18,6 +18,7 @@ download_code()
if [ "$PR" != "false" ] ; then
cd $FULL_SPYDER_CLONE
git fetch origin pull/$PR/head:travis_pr_$PR
git checkout travis_pr_$TRAVIS_PULL_REQUEST
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, but if I try to run pip install -e .[test] It'll fail because It was in the master branch

# Install extra packages
EXTRA_PACKAGES="matplotlib pandas sympy pyzmq pillow"
pip install $EXTRA_PACKAGES
pip install -e .[test]
Copy link
Member

@ccordoba12 ccordoba12 Feb 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not needed here. Please do it after the wheel has been built, i.e. here:

https://github.com/spyder-ide/spyder/blob/master/continuous_integration/travis/run_test.sh#L28

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, this will allow to delete the duplicated checkout

@@ -26,6 +26,9 @@ if [ "$USE_CONDA" = true ] ; then
else
cd $FULL_SPYDER_CLONE
pip install dist/spyder-*.whl

# Install testing packages
pip install -e .[test]
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 line necessary? I mean, what I want is to install our wheel and see if it pulls our test dependencies. So something like

 pip install dist/spyder-*.whl[test]

should work (but I'm not sure :-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure neither 🙈

I added a commit to see if this work

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It didn't work because dist/spyder-*.whl is a filename

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix this one before merging this PR ;-)

@rlaverde rlaverde force-pushed the required-test-packages branch from 25d73a3 to 9c5597e Compare March 1, 2017 12:53
@rlaverde
Copy link
Member Author

rlaverde commented Mar 1, 2017

@ccordoba12 I think the installation using pip is partially working, but I'm blocked because the issue with QtWebEngineWidgets not avalaible in debian/ubuntu


# Install extra packages
EXTRA_PACKAGES="matplotlib pandas sympy pyzmq pillow"
pip install $EXTRA_PACKAGES
Copy link
Member

@ccordoba12 ccordoba12 Mar 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert this change for now. I have a plan on how to start using the pyqt5 wheels on CircleCI.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only revert the installation of pyqt (to install it with conda), the other dependencies will be installed by pip install -e .[test]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're totally right :-)

@rlaverde rlaverde force-pushed the required-test-packages branch from 9c5597e to 4c09c75 Compare March 7, 2017 22:56
@ccordoba12
Copy link
Member

@ghisvail, I think this could be useful for you guys if you want to run our tests in the Debian infrastructure.

@ccordoba12 ccordoba12 merged commit 8fa3cd4 into spyder-ide:3.1.x Mar 8, 2017
ccordoba12 added a commit that referenced this pull request Mar 8, 2017
ccordoba12 added a commit that referenced this pull request Mar 8, 2017
@ghisvail
Copy link
Contributor

ghisvail commented Mar 8, 2017

Could be, assuming you guys distribute your tests following the pytest guidelines. The Debian packaging is not currently CI tested on the Debian infrastructure.

Another thing which could help would be to verify whether spyder can be tested with pytest-xvfb instead of manual calls to xvfb / xvfb-run. The latter are known to be occasionally unreliable on parallel execution setups in our experience. Same comment applies to qtpy and qtawesome. It worked for spyder-unittest.

@ccordoba12
Copy link
Member

assuming you guys distribute your tests following the pytest guidelines

I think we are, by "inlining test directories into [our] application package"

Another thing which could help would be to verify whether spyder can be tested with pytest-xvfb

CircleCi always starts a xvfb instance, so we haven't seen the need of using pytest-xvfb. However, I'm not opposed to idea :-) Could you open a PR to see how that could work?

@rlaverde rlaverde deleted the required-test-packages branch March 8, 2017 15:12
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.

3 participants