-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
#2141 fix travis #1255
#2141 fix travis #1255
Conversation
Looks like tests are still failing. 😢 |
Yes, the plan was to get the tests running and not fixing them. |
@brettcannon |
@@ -4,7 +4,16 @@ dist: trusty | |||
|
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.
Add cache: pip
to cache pip-installed dependencies.
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.
Fixed
.travis.yml
Outdated
@@ -45,18 +54,21 @@ before_install: | | |||
pyenv install $PYTHON | |||
pyenv virtualenv $PYTHON MYVERSION | |||
source ~/.pyenv/versions/MYVERSION/bin/activate | |||
pyenv global $PYTHON |
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.
What is pyenv for? If you need to run tests under different Python versions you could use tox and then list different Python versions in the text matrix.
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.
This was the only way I knew I could get multiple versions of Python running on Travis for OSX. Will check tox.
.travis.yml
Outdated
python --version | ||
python -c 'import sys;print(sys.version)' | ||
python -c 'import sys;print(sys.executable)' | ||
fi | ||
install: | ||
# we have this here so we can see where python is installed and hardcode in our tests | ||
# else when running npm test, the python version picked is completely different :( | ||
# python-dev is to ensure jupyter installs properly | ||
- sudo apt-get install python-dev |
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.
Can you not specify python-dev
under packages
so you can drop the sudo
requirement? I'm actually surprised it isn't included by default.
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.
For some reason this is required, else installing jupyter falls over.
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.
Hrm, that's unfortunate/weird. Another reason to eventually rip that code out! 😉
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.
Yes agreed.
However I would like to remove Jupyter tests and all of the related code/settings in a separate PR. I'd like to track that separately. what do you think?
I.e. leave this for now, and it will get removed in another PR.
.travis.yml
Outdated
python --version | ||
python -c 'import sys;print(sys.version)' | ||
python -c 'import sys;print(sys.executable)' | ||
fi | ||
install: | ||
# we have this here so we can see where python is installed and hardcode in our tests | ||
# else when running npm test, the python version picked is completely different :( | ||
# python-dev is to ensure jupyter installs properly | ||
- sudo apt-get install python-dev | ||
- python --version | ||
- python -c 'import sys;print(sys.executable)' | ||
- pip install -r requirements.txt |
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.
You can also use python -m pip
to guarantee what version of Python you're using.
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.
how do i do that?
.travis.yml
Outdated
python --version | ||
python -c 'import sys;print(sys.version)' | ||
python -c 'import sys;print(sys.executable)' | ||
fi | ||
install: | ||
# we have this here so we can see where python is installed and hardcode in our tests | ||
# else when running npm test, the python version picked is completely different :( | ||
# python-dev is to ensure jupyter installs properly | ||
- sudo apt-get install python-dev | ||
- python --version |
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.
This should be in the Travis output based on what version of Python you specified.
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.
?
.travis.yml
Outdated
@@ -45,18 +54,21 @@ before_install: | | |||
pyenv install $PYTHON | |||
pyenv virtualenv $PYTHON MYVERSION | |||
source ~/.pyenv/versions/MYVERSION/bin/activate | |||
pyenv global $PYTHON | |||
python --version | |||
python -c 'import sys;print(sys.version)' | |||
python -c 'import sys;print(sys.executable)' | |||
fi | |||
install: | |||
# we have this here so we can see where python is installed and hardcode in our tests |
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.
Hardcode how? Can you just pass it in through an environment variable or command-line option? E.g.
PYTHON_EXE=`which python`
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.
This was for OSX tests. I need this to be the global setting so the unit tests can pick the standard python path. I could introduce this env variable as well.
@brettcannon done |
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.
One minor change, but otherwise this LGTM!
.travis.yml
Outdated
# we have this here so we can see where python is installed and hardcode in our tests | ||
# else when running npm test, the python version picked is completely different :( | ||
- python --version | ||
- python -c 'import sys;print(sys.executable)' | ||
- pip install -r requirements.txt |
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.
Probably want pip install --upgrade -r requirements.txt
for when unpinned dependencies change.
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.
fixed
The fix was scoped to
.travis.yml
However I couldn't help but clean some of the code (not much, but a little).
Also fixed the unittest tests.