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

Jump to bottom

[RDY] Fix requirements file lookup #3800

Merged
merged 13 commits into from Apr 17, 2018
Merged

[RDY] Fix requirements file lookup #3800

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
9731997
Add tests for install core requirements
stsewd Mar 14, 2018
9aecefc
Add tests for user requirements
stsewd Mar 14, 2018
210922b
Fix install_user_requirements
stsewd Mar 14, 2018
028077b
Add tests for conda
stsewd Mar 14, 2018
928ab3e
Reuse the same requirements
stsewd Mar 23, 2018
b765727
Add docstring and comments
stsewd Mar 23, 2018
6446f48
Reuse conda requirements
stsewd Mar 23, 2018
029c078
Reuse pip install args
stsewd Mar 23, 2018
28c8a0b
Merge branch 'master' into fix-requirements-file-lookup
stsewd Mar 23, 2018
507b714
Reset mock last test
stsewd Mar 23, 2018
636d487
Merge branch 'master' into fix-requirements-file-lookup
stsewd Mar 27, 2018
d482c89
Merge branch 'master' into fix-requirements-file-lookup
stsewd Mar 27, 2018
98d47ec
Merge branch 'master' into fix-requirements-file-lookup
stsewd Apr 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 8 additions & 6 deletions readthedocs/doc_builder/python_environments.py
View file
Open in desktop
Expand Up @@ -4,6 +4,7 @@
from __future__ import (
absolute_import, division, print_function, unicode_literals)

import itertools
import json
import logging
import os
Expand Down Expand Up @@ -253,12 +254,13 @@ def install_user_requirements(self):
builder_class = get_builder_class(self.project.documentation_type)
docs_dir = (builder_class(build_env=self.build_env, python_env=self)
.docs_dir())
for path in [docs_dir, '']:
for req_file in ['pip_requirements.txt', 'requirements.txt']:
test_path = os.path.join(self.checkout_path, path, req_file)
if os.path.exists(test_path):
requirements_file_path = test_path
break
paths = [docs_dir, '']
req_files = ['pip_requirements.txt', 'requirements.txt']
for path, req_file in itertools.product(paths, req_files):
Copy link
Member

Choose a reason for hiding this comment

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

I don't see any logic change here, right?

We just had 2 nested for and now we use itertools.products that produces the same result in the end. Am I right?

Copy link
Member

Choose a reason for hiding this comment

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

>>> import itertools
>>> d = ['a', 'b']
>>> r = [1, 2]
>>> iter
iter(      itertools  
>>> list(itertools.product(d, r))
[('a', 1), ('a', 2), ('b', 1), ('b', 2)]
>>> for _d in d:
...     for _r in r:
...         print(_d, _r)
... 
a 1
a 2
b 1
b 2
>>>

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 is the same result, but the break now exists the loop #2812 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Oh boy! Great catch! :)

test_path = os.path.join(self.checkout_path, path, req_file)
if os.path.exists(test_path):
requirements_file_path = test_path
break

if requirements_file_path:
args = [
Expand Down
227 changes: 226 additions & 1 deletion readthedocs/rtd_tests/tests/test_doc_building.py
View file
Open in desktop
Expand Up @@ -19,16 +19,18 @@
from docker.errors import APIError as DockerAPIError
from docker.errors import DockerException
from mock import Mock, PropertyMock, mock_open, patch
from django_dynamic_fixture import get

from readthedocs.builds.constants import BUILD_STATE_CLONING
from readthedocs.builds.models import Version
from readthedocs.doc_builder.config import ConfigWrapper
from readthedocs.doc_builder.environments import (
BuildCommand, DockerBuildCommand, DockerBuildEnvironment, LocalBuildEnvironment)
from readthedocs.doc_builder.exceptions import BuildEnvironmentError
from readthedocs.doc_builder.python_environments import Virtualenv
from readthedocs.doc_builder.python_environments import Conda, Virtualenv
from readthedocs.projects.models import Project
from readthedocs.rtd_tests.mocks.environment import EnvironmentMockGroup
from readthedocs.rtd_tests.mocks.paths import fake_paths_lookup
from readthedocs.rtd_tests.tests.test_config_wrapper import create_load

DUMMY_BUILD_ID = 123
Expand Down Expand Up @@ -837,6 +839,229 @@ def test_command_oom_kill(self):
u'Command killed due to excessive memory consumption\n')


class TestPythonEnvironment(TestCase):

def setUp(self):
self.project_sphinx = get(Project, documentation_type='sphinx')
self.version_sphinx = get(Version, project=self.project_sphinx)

self.project_mkdocs = get(Project, documentation_type='mkdocs')
self.version_mkdocs = get(Version, project=self.project_mkdocs)

self.build_env_mock = Mock()

def test_install_core_requirements_sphinx(self):
python_env = Virtualenv(
version=self.version_sphinx,
build_env=self.build_env_mock,
)
python_env.install_core_requirements()
requirements = [
Copy link
Member

Choose a reason for hiding this comment

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

Use the same list of reqs (common) for both project (created in setUp) and append the ones that are specific for documentation type in each test.

This will be easier to follow and to change when we upgrade the reqs also.

'Pygments==2.2.0',
'setuptools==37.0.0',
'docutils==0.13.1',
'mock==1.0.1',
'pillow==2.6.1',
'alabaster>=0.7,<0.8,!=0.7.5',
'commonmark==0.5.4',
'recommonmark==0.4.0',
'sphinx==1.6.5',
'sphinx-rtd-theme<0.3',
'readthedocs-sphinx-ext<0.6',
]
args = [
'python',
mock.ANY, # pip path
'install',
'--use-wheel',
'--upgrade',
'--cache-dir',
mock.ANY, # cache path
]
args.extend(requirements)
self.build_env_mock.run.assert_called_once_with(
*args, bin_path=mock.ANY
)

def test_install_core_requirements_mkdocs(self):
python_env = Virtualenv(
version=self.version_mkdocs,
build_env=self.build_env_mock
)
python_env.install_core_requirements()
requirements = [
'Pygments==2.2.0',
'setuptools==37.0.0',
'docutils==0.13.1',
'mock==1.0.1',
'pillow==2.6.1',
'alabaster>=0.7,<0.8,!=0.7.5',
'commonmark==0.5.4',
'recommonmark==0.4.0',
'mkdocs==0.15.0',
]
args = [
'python',
mock.ANY, # pip path
'install',
'--use-wheel',
'--upgrade',
'--cache-dir',
mock.ANY, # cache path
]
args.extend(requirements)
self.build_env_mock.run.assert_called_once_with(
*args, bin_path=mock.ANY
)

def test_install_user_requirements(self):
self.build_env_mock.project = self.project_sphinx
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a docstring here explaining the test case in general?

self.build_env_mock.version = self.version_sphinx
python_env = Virtualenv(
version=self.version_sphinx,
build_env=self.build_env_mock
)

checkout_path = python_env.checkout_path
docs_requirements = os.path.join(
checkout_path, 'docs', 'requirements.txt'
)
root_requirements = os.path.join(
checkout_path, 'requirements.txt'
)
paths = {
os.path.join(checkout_path, 'docs'): True,
}
args = [
'python',
mock.ANY, # pip path
'install',
'--exists-action=w',
'--cache-dir',
mock.ANY, # cache path
'requirements_file'
]

paths[docs_requirements] = True
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a simple comment in each of the cases saying what's the scenario and what we expect?

paths[root_requirements] = False
with fake_paths_lookup(paths):
python_env.install_user_requirements()
args[-1] = '-r{}'.format(docs_requirements)
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this modification of the list, but I don't have a cleaner way of doing 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.

I didn't like it either, but it was the only way I thought to reuse the previous list p:

self.build_env_mock.run.assert_called_with(
*args, cwd=mock.ANY, bin_path=mock.ANY
)

paths[docs_requirements] = False
paths[root_requirements] = True
with fake_paths_lookup(paths):
python_env.install_user_requirements()
args[-1] = '-r{}'.format(root_requirements)
self.build_env_mock.run.assert_called_with(
*args, cwd=mock.ANY, bin_path=mock.ANY
)

paths[docs_requirements] = True
paths[root_requirements] = True
with fake_paths_lookup(paths):
python_env.install_user_requirements()
args[-1] = '-r{}'.format(docs_requirements)
self.build_env_mock.run.assert_called_with(
*args, cwd=mock.ANY, bin_path=mock.ANY
)

paths[docs_requirements] = False
paths[root_requirements] = False
with fake_paths_lookup(paths):
python_env.install_user_requirements()
self.build_env_mock.run.assert_not_called()

def test_install_core_requirements_sphinx_conda(self):
python_env = Conda(
version=self.version_sphinx,
build_env=self.build_env_mock,
)
python_env.install_core_requirements()
conda_requirements = [
Copy link
Member

Choose a reason for hiding this comment

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

Same here, try to use a shared list.

'mock',
'pillow',
'sphinx',
'sphinx_rtd_theme',
]
pip_requirements = [
'recommonmark',
'readthedocs-sphinx-ext',
]

args_pip = [
'python',
mock.ANY, # pip path
'install',
'-U',
Copy link
Member

Choose a reason for hiding this comment

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

You can also use a shared args_pip for all these tests. You will need to change the -U by --upgrade but I think that's OK and it's better to use long attributes instead of short. So, 👍

Copy link
Member Author

@stsewd stsewd Mar 23, 2018 •
edited

Choose a reason for hiding this comment

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

There is two types of the arg_pip, on conda the --use-wheel isn't used. Is that on purpose? or perhaps we can unify both :)?

'--cache-dir',
mock.ANY, # cache path
]
args_pip.extend(pip_requirements)

args_conda = [
'conda',
'install',
'--yes',
'--name',
self.version_sphinx.slug,
]
args_conda.extend(conda_requirements)

self.build_env_mock.run.assert_has_calls([
mock.call(*args_conda),
mock.call(*args_pip, bin_path=mock.ANY)
])

def test_install_core_requirements_mkdocs_conda(self):
python_env = Conda(
version=self.version_mkdocs,
build_env=self.build_env_mock,
)
python_env.install_core_requirements()
conda_requirements = [
'mock',
'pillow',
]
pip_requirements = [
'recommonmark',
'mkdocs',
]

args_pip = [
'python',
mock.ANY, # pip path
'install',
'-U',
'--cache-dir',
mock.ANY, # cache path
]
args_pip.extend(pip_requirements)

args_conda = [
'conda',
'install',
'--yes',
'--name',
self.version_mkdocs.slug,
]
args_conda.extend(conda_requirements)

self.build_env_mock.run.assert_has_calls([
mock.call(*args_conda),
mock.call(*args_pip, bin_path=mock.ANY)
])

def test_install_user_requirements_conda(self):
python_env = Conda(
version=self.version_sphinx,
build_env=self.build_env_mock,
)
python_env.install_user_requirements()
self.build_env_mock.run.assert_not_called()


class TestAutoWipeEnvironment(TestCase):
Expand Down