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

Make tests pass with latest pip/conda/tornado #167

Merged
merged 11 commits into from
Dec 14, 2018
Merged

Conversation

mcg1969
Copy link
Collaborator

@mcg1969 mcg1969 commented Dec 8, 2018

I'm not seeing any downside to replacing pip list <args> with pip freeze. It greatly simplifies the pip implementation.

But more importantly, the tests were just not working with later versions of conda and pip.

@@ -87,27 +87,10 @@ def installed(prefix):
if not os.path.isdir(prefix):
return dict()

# In pip 9, there's a big ugly deprecation warning by default if
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

By replacing pip list with pip freeze we avoid a ton of painful logic figuring out what --format keyword argument to use. In fact, the current version of this code is broken with newer versions of pip, because --format=legacy is gone.

@@ -21,6 +21,8 @@
from anaconda_project.internal.directory_contains import subdirectory_relative_to_directory
from anaconda_project.internal.py2_compat import is_string

CONDA_EXE = os.environ.get("CONDA_EXE", "conda")
Copy link
Collaborator Author

@mcg1969 mcg1969 Dec 8, 2018

Choose a reason for hiding this comment

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

This is necessary for versions of conda that do not put conda on the path by default, choosing instead to use a shell command. Replacing the bare references to "conda" with CONDA_EXE, when it exists, does the job

@@ -181,39 +182,3 @@ def test_parse_spec_url():

# this was a real-world example with an url and [] after package name
assert 'dask' == pip_api.parse_spec('git+https://github.com/blaze/dask.git#egg=dask[complete]').name


def test_format_flag(monkeypatch):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These tests are no longer necessary because we're not using pip list --format=... anymore

@@ -64,16 +56,11 @@ install:
- if [[ "$TRAVIS_OS_NAME" == "linux" ]]; then
OS_PACKAGES=libffi ;
fi ;
conda create -q -n test-environment python="$TRAVIS_PYTHON_VERSION" coverage tornado keyring pytest pytest-cov pip redis notebook bokeh ruamel_yaml anaconda-client requests psutil $OS_PACKAGES
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A pin to tornado v4 seems to be necessary to get tests to pass. A fix should be created for this

@mcg1969 mcg1969 force-pushed the fix-ci branch 5 times, most recently from 1f8519f to 86e80b6 Compare December 8, 2018 22:14
@@ -22,9 +22,11 @@ def add_variables(project_dir, env_spec_name, vars_to_add, default):
Returns exit code
"""
if len(vars_to_add) > 1 and default is not None:
# yapf on P2.7 vs yapf on P3.6 disagree here.
# yapf: disable
print("It isn't clear which variable your --default option goes with; "
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not clear why, but the same version of yapf gave different results here depending upon whether it was running on Python 2.7 and Python 3.6.

@mcg1969
Copy link
Collaborator Author

mcg1969 commented Dec 9, 2018

@goanpeca @fpliger do you think it's possible to get this reviewed and merged in? This is basically a "get tests passing again" PR, though with fixes required to get it working with later versions of conda and pip (versions that presumably will end up in future versions of AE).

There is more I want to do here, for sure. But making sure we have functioning tests is a necessary thing, even if I don't.

@mcg1969 mcg1969 changed the title Replace pip list with pip freeze; update tests for latest conda Make tests pass again with latest pip and conda Dec 9, 2018
@mcg1969
Copy link
Collaborator Author

mcg1969 commented Dec 10, 2018

That last commit allows tests to pass on Windows.

I personally would like to refactor these tests considerably. In particular, I'd like all functional testing to be performed on the conda package within conda build itself. Thankfully this seems entirely doable, and I have a conda recipe that accomplishes this.

Then the source verification testing---flake8, yapf, pep257, copyright notices, etc---would go into a separate Travis task, run only once. There would still be an expectation that the developer commits any of these textual changes themselves.

@mcg1969
Copy link
Collaborator Author

mcg1969 commented Dec 10, 2018

But for now—this at least gets the tests working again, and the code working with the latest conda and pip. Those additional refactors can be done in a separate PR.

@mcg1969 mcg1969 closed this Dec 10, 2018
@mcg1969 mcg1969 deleted the fix-ci branch December 10, 2018 22:42
@mcg1969
Copy link
Collaborator Author

mcg1969 commented Dec 11, 2018

I didn't mean to close this. I did however fix the tornado issue :-) I'll re-open when I get back to my computer

@mcg1969 mcg1969 restored the fix-ci branch December 11, 2018 00:51
@mcg1969 mcg1969 reopened this Dec 11, 2018
@mcg1969 mcg1969 force-pushed the fix-ci branch 2 times, most recently from b6fbed8 to c2a00d5 Compare December 11, 2018 03:06
@mcg1969 mcg1969 changed the title Make tests pass again with latest pip and conda Make tests pass again with latest pip/conda/tornado Dec 11, 2018
@mcg1969 mcg1969 changed the title Make tests pass again with latest pip/conda/tornado Make tests pass with latest pip/conda/tornado Dec 11, 2018
@mcg1969
Copy link
Collaborator Author

mcg1969 commented Dec 11, 2018

The only reason this isn't passing is because conda is taking so long to install anaconda in some of the tests. I know the conda team is working on this; I'll see if there's a way to reduce test time here.

@mcg1969
Copy link
Collaborator Author

mcg1969 commented Dec 11, 2018

I've reduced test time in a three ways:

  • Monkeypatching the default environment spec for empty projects to contain python 3.7 instead of anaconda, which speeds up solve time considerably.
  • Adding some version pins to a couple of tests that were creating projects with explicit package lists.
  • Bumping up some of the pinned versions of bokeh, conda, and python, so they can be found in pkgs/main instead of pkgs/free. This allows us to remove pkgs/free from our default channel list, if we choose. Unfortunately, this breaks the msys2 test, so we can't do this in CI, but it's useful for local testing (and a good strategy for conda use in general).

@fpliger
Copy link
Contributor

fpliger commented Dec 12, 2018

@mcg1969 The change LGTM.. we can merge as soon as we get CI happy again :)

@mcg1969
Copy link
Collaborator Author

mcg1969 commented Dec 12, 2018

Yep, I'll fix things

@mcg1969 mcg1969 closed this Dec 12, 2018
@mcg1969 mcg1969 reopened this Dec 12, 2018
@mcg1969
Copy link
Collaborator Author

mcg1969 commented Dec 12, 2018

It's going to pass this time.

@mcg1969
Copy link
Collaborator Author

mcg1969 commented Dec 12, 2018

@fpliger ready to merge! I'll do a second PR streamlining the tests and making better use of conda-build once this is merged. No functional changes on that one either.

@fpliger fpliger merged commit b654efe into anaconda:master Dec 14, 2018
@mcg1969 mcg1969 deleted the fix-ci branch December 18, 2018 20:19
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.

None yet

2 participants