From 176742ba53edc839093ae1f5862193a8d412bec1 Mon Sep 17 00:00:00 2001 From: Anthony Johnson Date: Tue, 4 Aug 2015 14:34:13 -0700 Subject: [PATCH] Cleanup and fix tests --- .../core/management/commands/run_docker.py | 41 ----------- readthedocs/doc_builder/backends/sphinx.py | 5 +- readthedocs/doc_builder/base.py | 3 +- readthedocs/doc_builder/environments.py | 39 +++++------ readthedocs/rtd_tests/tests/test_builds.py | 28 +++++--- readthedocs/rtd_tests/tests/test_celery.py | 14 ++-- .../rtd_tests/tests/test_core_management.py | 68 ------------------- 7 files changed, 52 insertions(+), 146 deletions(-) delete mode 100644 readthedocs/core/management/commands/run_docker.py delete mode 100644 readthedocs/rtd_tests/tests/test_core_management.py diff --git a/readthedocs/core/management/commands/run_docker.py b/readthedocs/core/management/commands/run_docker.py deleted file mode 100644 index 8f4078811..000000000 --- a/readthedocs/core/management/commands/run_docker.py +++ /dev/null @@ -1,41 +0,0 @@ -import logging -import fileinput -import json - -from django.core.management.base import BaseCommand - -from readthedocs.projects import tasks - -log = logging.getLogger(__name__) - - -class Command(BaseCommand): - """Trigger build for project version""" - - args = '' - - def handle(self, files=None, *args, **options): - - def _return_json(output): - return json.dumps(output) - - output = None - try: - input_data = self._get_input(files) - version_data = json.loads(input_data) - version = tasks.make_api_version(version_data) - log.info('Building %s', version) - output = _return_json(tasks.docker_build(version)) - except Exception as e: - log.exception('Error starting docker build') - output = _return_json( - {'doc_builder': (-1, '', '{0}: {1}'.format(type(e).__name__, - str(e)))}) - finally: - print(output) - - def _get_input(self, files=None): - if files is None: - files = '-' - fh = fileinput.FileInput(files) - return ''.join([line for line in fh]) diff --git a/readthedocs/doc_builder/backends/sphinx.py b/readthedocs/doc_builder/backends/sphinx.py index fcda78a96..a865d1f13 100644 --- a/readthedocs/doc_builder/backends/sphinx.py +++ b/readthedocs/doc_builder/backends/sphinx.py @@ -11,11 +11,14 @@ from django.template.loader import render_to_string from django.conf import settings from readthedocs.builds import utils as version_utils -from readthedocs.doc_builder.base import BaseBuilder, restoring_chdir from readthedocs.projects.utils import safe_write from readthedocs.projects.exceptions import ProjectImportError from readthedocs.restapi.client import api +from ..base import BaseBuilder, restoring_chdir +from ..exceptions import BuildEnvironmentError + + log = logging.getLogger(__name__) TEMPLATE_DIR = '%s/readthedocs/templates/sphinx' % settings.SITE_ROOT diff --git a/readthedocs/doc_builder/base.py b/readthedocs/doc_builder/base.py index 0be94248e..149f8b701 100644 --- a/readthedocs/doc_builder/base.py +++ b/readthedocs/doc_builder/base.py @@ -19,7 +19,6 @@ def restoring_chdir(fn): class BaseBuilder(object): - """ The Base for all Builders. Defines the API for subclasses. @@ -31,9 +30,9 @@ class BaseBuilder(object): # old_artifact_path = .. def __init__(self, version, force=False, build_env=None): - self.project = build_env.project # TODO do we really need to push the version in here? self.version = version + self.project = version.project self._force = force self.target = self.version.project.artifact_path( version=self.version.slug, diff --git a/readthedocs/doc_builder/environments.py b/readthedocs/doc_builder/environments.py index f7aab1214..f6b1f5b7b 100644 --- a/readthedocs/doc_builder/environments.py +++ b/readthedocs/doc_builder/environments.py @@ -20,7 +20,6 @@ from docker.utils import create_host_config from docker.errors import APIError as DockerAPIError, DockerException from readthedocs.builds.constants import BUILD_STATE_FINISHED -from readthedocs.projects.utils import run from readthedocs.restapi.serializers import VersionFullSerializer from readthedocs.projects.constants import LOG_TEMPLATE from readthedocs.api.client import api as api_v1 @@ -139,7 +138,7 @@ class DockerBuildCommand(BuildCommand): Build command to execute in docker container ''' - def run(self, cmd_input=None, combine_output=True): + def run(self): '''Execute command in existing Docker container :param cmd_input: input to pass to command in STDIN @@ -208,9 +207,10 @@ class BuildEnvironment(object): def __exit__(self, exc_type, exc_value, tb): ret = self.handle_exception(exc_type, exc_value, tb) self.update_build(state=BUILD_STATE_FINISHED) - log.info(LOG_TEMPLATE.format(project=self.project.slug, - version=self.version.slug, - msg='Build finished')) + log.info(LOG_TEMPLATE + .format(project=self.project.slug, + version=self.version.slug, + msg='Build finished')) return ret def handle_exception(self, exc_type, exc_value, tb): @@ -225,13 +225,11 @@ class BuildEnvironment(object): exception will bubble up. """ if exc_type is not None: - log.error( - LOG_TEMPLATE.format( - project=self.project.slug, - version=self.version.slug, - msg=exc_value), - exc_info=True - ) + log.error(LOG_TEMPLATE + .format(project=self.project.slug, + version=self.version.slug, + msg=exc_value), + exc_info=True) if issubclass(exc_type, BuildEnvironmentWarning): return True else: @@ -240,7 +238,6 @@ class BuildEnvironment(object): return True return False - def run(self, *cmd, **kwargs): '''Run command''' kwargs['build_env'] = self @@ -258,18 +255,18 @@ class BuildEnvironment(object): @property def successful(self): # TODO should this include a check for finished state? - return (self.failure is None - and all(cmd.successful for cmd in self.commands)) + return (self.failure is None and + all(cmd.successful for cmd in self.commands)) @property def failed(self): - return (self.failure is not None - or any(cmd.failed for cmd in self.commands)) + return (self.failure is not None or + any(cmd.failed for cmd in self.commands)) @property def done(self): - return (self.build is not None - and self.build.state == BUILD_STATE_FINISHED) + return (self.build is not None and + self.build['state'] == BUILD_STATE_FINISHED) def update_build(self, state=None): """ @@ -302,8 +299,8 @@ class BuildEnvironment(object): # TODO Replace this with per-command output tracking in the db self.build['output'] = '\n'.join([str(cmd) - for cmd in self.commands - if cmd is not None]) + for cmd in self.commands + if cmd is not None]) # Attempt to stop unicode errors on build reporting for key, val in self.build.items(): diff --git a/readthedocs/rtd_tests/tests/test_builds.py b/readthedocs/rtd_tests/tests/test_builds.py index 294dadc74..adf73d904 100644 --- a/readthedocs/rtd_tests/tests/test_builds.py +++ b/readthedocs/rtd_tests/tests/test_builds.py @@ -36,6 +36,17 @@ def build_subprocess_side_effect(*args, **kwargs): class BuildTests(TestCase): + def setUp(self): + self.patches = {} + self.mocks = {} + self.patches['update_build'] = mock.patch.object(LocalEnvironment, + 'update_build') + self.mocks['update_build'] = self.patches['update_build'].start() + + def tearDown(self): + for patch in self.patches: + self.patches[patch].stop() + @mock.patch('slumber.Resource') @mock.patch('os.chdir') @mock.patch('readthedocs.projects.models.Project.api_versions') @@ -51,10 +62,12 @@ class BuildTests(TestCase): # subprocess mock logic mock_process = mock.Mock() - process_return_dict = {'communicate.return_value': ('SOMEGITHASH', '')} + process_return_dict = { + 'communicate.return_value': ('SOMEGITHASH', ''), + 'returncode': 0 + } mock_process.configure_mock(**process_return_dict) mock_Popen.return_value = mock_process - mock_Popen.side_effect = build_subprocess_side_effect project = get(Project, slug='project-1', @@ -79,18 +92,15 @@ class BuildTests(TestCase): task.project = project with mock.patch('codecs.open', mock.mock_open(), create=True): with fake_paths_lookup({conf_path: True}): - built_docs = task.build_docs(version, - False, - False) + built_docs = task.build_docs_html(False) - builder_class = get_builder_class(project.documentation_type) - builder = builder_class(version) + with build_env: + builder_class = get_builder_class(project.documentation_type) + builder = builder_class(version) self.assertIn(builder.sphinx_builder, str(mock_Popen.call_args_list[1]) ) - # We are using the comment builder - def test_builder_comments(self): # Normal build diff --git a/readthedocs/rtd_tests/tests/test_celery.py b/readthedocs/rtd_tests/tests/test_celery.py index a232b09f8..17a14230e 100644 --- a/readthedocs/rtd_tests/tests/test_celery.py +++ b/readthedocs/rtd_tests/tests/test_celery.py @@ -1,9 +1,11 @@ import os -from os.path import exists -import shutil -from tempfile import mkdtemp -from django.contrib.auth.models import User import json +import shutil +from os.path import exists +from tempfile import mkdtemp + +from django.contrib.auth.models import User +from mock import patch, MagicMock from readthedocs.projects.models import Project from readthedocs.projects import tasks @@ -59,6 +61,10 @@ class TestCeleryBuilding(RTDTestCase): self.assertTrue(result.successful()) self.assertFalse(exists(directory)) + @patch('readthedocs.projects.tasks.UpdateDocsTask.build_docs', + new=MagicMock) + @patch('readthedocs.projects.tasks.UpdateDocsTask.setup_vcs', + new=MagicMock) def test_update_docs(self): with mock_api(self.repo): update_docs = tasks.UpdateDocsTask() diff --git a/readthedocs/rtd_tests/tests/test_core_management.py b/readthedocs/rtd_tests/tests/test_core_management.py deleted file mode 100644 index 8a572e7ba..000000000 --- a/readthedocs/rtd_tests/tests/test_core_management.py +++ /dev/null @@ -1,68 +0,0 @@ -from StringIO import StringIO - -from django.test import TestCase -from mock import patch - -from readthedocs.core.management.commands import run_docker -from readthedocs.projects.models import Project -from readthedocs.builds.models import Version - - -class TestRunDocker(TestCase): - '''Test run_docker command with good input and output''' - - fixtures = ['test_data'] - - def setUp(self): - self.project = Project.objects.get(slug='pip') - self.version = Version(slug='foo', verbose_name='foobar') - self.project.versions.add(self.version) - - def _get_input(self, files=None): - return ('{"project": {"id": 6, "name": "Pip", "slug": "pip"},' - '"id": 71, "type": "tag", "identifier": "437fb316fbbdba1acdd22e07dbe7c4809ffd97e6",' - '"verbose_name": "stable", "slug": "stable"}') - - def _docker_build(data): - if isinstance(data, Version): - return {'html': (0, 'DOCKER PASS', '')} - else: - return {'html': (1, '', 'DOCKER FAIL')} - - def test_stdin(self): - '''Test docker build command''' - - def _input(_, files=None): - return '{"test": "foobar"}' - - with patch.object(run_docker.Command, '_get_input', _input): - cmd = run_docker.Command() - assert cmd._get_input() == '{"test": "foobar"}' - - @patch.object(run_docker.Command, '_get_input', _get_input) - @patch('readthedocs.projects.tasks.docker_build', _docker_build) - @patch('sys.stdout', new_callable=StringIO) - def test_good_input(self, mock_output): - '''Test docker build command''' - cmd = run_docker.Command() - self.assertEqual(cmd._get_input(), self._get_input()) - cmd.handle() - self.assertEqual( - mock_output.getvalue(), - '{"html": [0, "DOCKER PASS", ""]}\n' - ) - - @patch('readthedocs.projects.tasks.docker_build', _docker_build) - def test_bad_input(self): - '''Test docker build command''' - with patch.object(run_docker.Command, '_get_input') as mock_input: - with patch('sys.stdout', new_callable=StringIO) as mock_output: - mock_input.return_value = 'BAD JSON' - cmd = run_docker.Command() - cmd.handle() - self.assertEqual( - mock_output.getvalue(), - ('{"doc_builder": ' - '[-1, "", "ValueError: No JSON object could be decoded"]}' - '\n') - )