From 6cac359fa887fe9b60602bc419b5aad36aff343c Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Sun, 11 Mar 2018 04:14:08 -0500 Subject: [PATCH 01/20] Expose wrong tests --- readthedocs/doc_builder/environments.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/doc_builder/environments.py b/readthedocs/doc_builder/environments.py index e3a74e2f7a4..cca0a3cd9a6 100644 --- a/readthedocs/doc_builder/environments.py +++ b/readthedocs/doc_builder/environments.py @@ -455,7 +455,7 @@ def handle_exception(self, exc_type, exc_value, _): return True def record_command(self, command): - command.save() + pass def _log_warning(self, msg): # :'( From 1c1e60e047bb4235cfbb13f854b8cb3860137583 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Sun, 11 Mar 2018 04:19:32 -0500 Subject: [PATCH 02/20] Add additional asserts --- .../rtd_tests/tests/test_doc_building.py | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/readthedocs/rtd_tests/tests/test_doc_building.py b/readthedocs/rtd_tests/tests/test_doc_building.py index 5c65c21e2a3..deea1c8df3d 100644 --- a/readthedocs/rtd_tests/tests/test_doc_building.py +++ b/readthedocs/rtd_tests/tests/test_doc_building.py @@ -78,6 +78,17 @@ def test_normal_execution(self): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) + # The command was saved + command = build_env.commands[0] + self.mocks.mocks['api_v2.command'].post.assert_called_once_with({ + 'build': DUMMY_BUILD_ID, + 'command': command.get_command(), + 'description': command.description, + 'output': command.output, + 'exit_code': 0, + 'start_time': command.start_time, + 'end_time': command.end_time, + }) self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, 'version': self.version.pk, @@ -151,6 +162,17 @@ def test_failing_execution(self): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) + # The command was saved + command = build_env.commands[0] + self.mocks.mocks['api_v2.command'].post.assert_called_once_with({ + 'build': DUMMY_BUILD_ID, + 'command': command.get_command(), + 'description': command.description, + 'output': command.output, + 'exit_code': 1, + 'start_time': command.start_time, + 'end_time': command.end_time, + }) self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, 'version': self.version.pk, @@ -184,6 +206,8 @@ def test_failing_execution_with_caught_exception(self): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) + # The command was not saved + self.mocks.mocks['api_v2.command'].post.assert_not_called() self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, 'version': self.version.pk, @@ -216,6 +240,8 @@ def test_failing_execution_with_unexpected_exception(self): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) + # The command was not saved + self.mocks.mocks['api_v2.command'].post.assert_not_called() self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, 'version': self.version.pk, From 83130786318547dcf7662e923cc5e0c0ed3e1fcb Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Sun, 11 Mar 2018 04:28:23 -0500 Subject: [PATCH 03/20] Fix tests --- readthedocs/doc_builder/environments.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/doc_builder/environments.py b/readthedocs/doc_builder/environments.py index cca0a3cd9a6..e3a74e2f7a4 100644 --- a/readthedocs/doc_builder/environments.py +++ b/readthedocs/doc_builder/environments.py @@ -455,7 +455,7 @@ def handle_exception(self, exc_type, exc_value, _): return True def record_command(self, command): - pass + command.save() def _log_warning(self, msg): # :'( From 4dafcf618f6f70e02f624d00b2e877489d9cd710 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Sun, 11 Mar 2018 04:30:38 -0500 Subject: [PATCH 04/20] Expose falling assert --- readthedocs/rtd_tests/tests/test_doc_building.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/readthedocs/rtd_tests/tests/test_doc_building.py b/readthedocs/rtd_tests/tests/test_doc_building.py index deea1c8df3d..2e2d98bda21 100644 --- a/readthedocs/rtd_tests/tests/test_doc_building.py +++ b/readthedocs/rtd_tests/tests/test_doc_building.py @@ -121,10 +121,13 @@ def test_incremental_state_update_with_no_update(self): ] for build_env in build_envs: + # __import__('pdb').set_trace() with build_env: build_env.update_build(BUILD_STATE_CLONING) + # FIXME: This method raise an assert exception, + # but is captured by the context manager self.mocks.mocks['api_v2.build']().put.assert_called_with({ - 'id': DUMMY_BUILD_ID, + 'id': 'Just for testing', 'version': self.version.pk, 'success': True, 'project': self.project.pk, From 2d1df1317eba2b8aea36cbdacbc9755b08ab1918 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Sun, 11 Mar 2018 14:51:24 -0500 Subject: [PATCH 05/20] Add tests for DockerBuildEnvironment --- .../rtd_tests/tests/test_doc_building.py | 51 +++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/readthedocs/rtd_tests/tests/test_doc_building.py b/readthedocs/rtd_tests/tests/test_doc_building.py index 2e2d98bda21..5f83b358c6f 100644 --- a/readthedocs/rtd_tests/tests/test_doc_building.py +++ b/readthedocs/rtd_tests/tests/test_doc_building.py @@ -303,6 +303,8 @@ def test_environment_successful_build(self): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) + # The command was not saved + self.mocks.mocks['api_v2.command'].post.assert_not_called() self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, 'version': self.version.pk, @@ -332,6 +334,8 @@ def test_environment_successful_build_without_update(self): self.assertTrue(build_env.successful) # api() is not called anymore, we use api_v2 instead + # The command was not saved + self.mocks.mocks['api_v2.command'].post.assert_not_called() self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) self.assertFalse(self.mocks.mocks['api_v2.build']().put.called) @@ -351,6 +355,8 @@ def test_environment_failed_build_without_update_but_with_error(self): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) + # The command was not saved + self.mocks.mocks['api_v2.command'].post.assert_not_called() self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, 'version': self.version.pk, @@ -383,6 +389,8 @@ def _inner(): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) + # The command was not saved + self.mocks.mocks['api_v2.command'].post.assert_not_called() self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, 'version': self.version.pk, @@ -426,6 +434,8 @@ def _inner(): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) + # The command was not saved + self.mocks.mocks['api_v2.command'].post.assert_not_called() self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, 'version': self.version.pk, @@ -466,6 +476,17 @@ def test_api_failure_on_docker_memory_limit(self): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) + # The command was saved + command = build_env.commands[0] + self.mocks.mocks['api_v2.command'].post.assert_called_once_with({ + 'build': DUMMY_BUILD_ID, + 'command': command.get_command(), + 'description': command.description, + 'output': command.output, + 'exit_code': -1, + 'start_time': command.start_time, + 'end_time': command.end_time, + }) self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, 'version': self.version.pk, @@ -498,6 +519,8 @@ def test_api_failure_on_error_in_exit(self): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) + # The command was not saved + self.mocks.mocks['api_v2.command'].post.assert_not_called() self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, 'version': self.version.pk, @@ -536,6 +559,8 @@ def test_api_failure_returns_previous_error_on_error_in_exit(self): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) + # The command was not saved + self.mocks.mocks['api_v2.command'].post.assert_not_called() self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, 'version': self.version.pk, @@ -579,6 +604,17 @@ def test_command_execution(self): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) + # The command was saved + command = build_env.commands[0] + self.mocks.mocks['api_v2.command'].post.assert_called_once_with({ + 'build': DUMMY_BUILD_ID, + 'command': command.get_command(), + 'description': command.description, + 'output': command.output, + 'exit_code': 1, + 'start_time': command.start_time, + 'end_time': command.end_time, + }) self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, 'version': self.version.pk, @@ -623,6 +659,8 @@ def test_command_execution_cleanup_exception(self): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) + # The command was not saved + self.mocks.mocks['api_v2.command'].post.assert_not_called() self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, 'version': self.version.pk, @@ -667,6 +705,8 @@ def _inner(): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) + # The command was not saved + self.mocks.mocks['api_v2.command'].post.assert_not_called() self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, 'version': self.version.pk, @@ -714,6 +754,17 @@ def test_container_timeout(self): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) + # The command was saved + command = build_env.commands[0] + self.mocks.mocks['api_v2.command'].post.assert_called_once_with({ + 'build': DUMMY_BUILD_ID, + 'command': command.get_command(), + 'description': command.description, + 'output': command.output, + 'exit_code': 0, + 'start_time': command.start_time, + 'end_time': command.end_time, + }) self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, 'version': self.version.pk, From 49df30ef3c4570590990d91d721f082496e02a39 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Sun, 11 Mar 2018 15:38:11 -0500 Subject: [PATCH 06/20] Fix tests --- .../rtd_tests/tests/test_doc_building.py | 35 ++++++++++++------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_doc_building.py b/readthedocs/rtd_tests/tests/test_doc_building.py index 5f83b358c6f..582db354a48 100644 --- a/readthedocs/rtd_tests/tests/test_doc_building.py +++ b/readthedocs/rtd_tests/tests/test_doc_building.py @@ -210,7 +210,7 @@ def test_failing_execution_with_caught_exception(self): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) # The command was not saved - self.mocks.mocks['api_v2.command'].post.assert_not_called() + self.assertFalse(self.mocks.mocks['api_v2.command'].post.called) self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, 'version': self.version.pk, @@ -244,7 +244,7 @@ def test_failing_execution_with_unexpected_exception(self): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) # The command was not saved - self.mocks.mocks['api_v2.command'].post.assert_not_called() + self.assertFalse(self.mocks.mocks['api_v2.command'].post.called) self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, 'version': self.version.pk, @@ -304,7 +304,7 @@ def test_environment_successful_build(self): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) # The command was not saved - self.mocks.mocks['api_v2.command'].post.assert_not_called() + self.assertFalse(self.mocks.mocks['api_v2.command'].post.called) self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, 'version': self.version.pk, @@ -334,9 +334,9 @@ def test_environment_successful_build_without_update(self): self.assertTrue(build_env.successful) # api() is not called anymore, we use api_v2 instead - # The command was not saved - self.mocks.mocks['api_v2.command'].post.assert_not_called() self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) + # The command was not saved + self.assertFalse(self.mocks.mocks['api_v2.command'].post.called) self.assertFalse(self.mocks.mocks['api_v2.build']().put.called) def test_environment_failed_build_without_update_but_with_error(self): @@ -356,7 +356,7 @@ def test_environment_failed_build_without_update_but_with_error(self): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) # The command was not saved - self.mocks.mocks['api_v2.command'].post.assert_not_called() + self.assertFalse(self.mocks.mocks['api_v2.command'].post.called) self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, 'version': self.version.pk, @@ -390,7 +390,7 @@ def _inner(): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) # The command was not saved - self.mocks.mocks['api_v2.command'].post.assert_not_called() + self.assertFalse(self.mocks.mocks['api_v2.command'].post.called) self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, 'version': self.version.pk, @@ -435,7 +435,7 @@ def _inner(): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) # The command was not saved - self.mocks.mocks['api_v2.command'].post.assert_not_called() + self.assertFalse(self.mocks.mocks['api_v2.command'].post.called) self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, 'version': self.version.pk, @@ -520,7 +520,7 @@ def test_api_failure_on_error_in_exit(self): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) # The command was not saved - self.mocks.mocks['api_v2.command'].post.assert_not_called() + self.assertFalse(self.mocks.mocks['api_v2.command'].post.called) self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, 'version': self.version.pk, @@ -560,7 +560,7 @@ def test_api_failure_returns_previous_error_on_error_in_exit(self): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) # The command was not saved - self.mocks.mocks['api_v2.command'].post.assert_not_called() + self.assertFalse(self.mocks.mocks['api_v2.command'].post.called) self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, 'version': self.version.pk, @@ -659,8 +659,17 @@ def test_command_execution_cleanup_exception(self): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) - # The command was not saved - self.mocks.mocks['api_v2.command'].post.assert_not_called() + # The command was saved + command = build_env.commands[0] + self.mocks.mocks['api_v2.command'].post.assert_called_once_with({ + 'build': DUMMY_BUILD_ID, + 'command': command.get_command(), + 'description': command.description, + 'output': command.output, + 'exit_code': 0, + 'start_time': command.start_time, + 'end_time': command.end_time, + }) self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, 'version': self.version.pk, @@ -706,7 +715,7 @@ def _inner(): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) # The command was not saved - self.mocks.mocks['api_v2.command'].post.assert_not_called() + self.assertFalse(self.mocks.mocks['api_v2.command'].post.called) self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, 'version': self.version.pk, From 0739ec66beef845c4eead2a4f06b77273f6edbb5 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Sun, 11 Mar 2018 15:42:56 -0500 Subject: [PATCH 07/20] Add test for record=False --- .../rtd_tests/tests/test_doc_building.py | 85 +++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/readthedocs/rtd_tests/tests/test_doc_building.py b/readthedocs/rtd_tests/tests/test_doc_building.py index 582db354a48..d931b62c12d 100644 --- a/readthedocs/rtd_tests/tests/test_doc_building.py +++ b/readthedocs/rtd_tests/tests/test_doc_building.py @@ -104,6 +104,46 @@ def test_normal_execution(self): 'exit_code': 0, }) + def test_command_not_recorded(self): + """Normal build in passing state with no command recorded.""" + self.mocks.configure_mock('process', { + 'communicate.return_value': (b'This is okay', '') + }) + type(self.mocks.process).returncode = PropertyMock(return_value=0) + + build_env = LocalBuildEnvironment( + version=self.version, + project=self.project, + build={'id': DUMMY_BUILD_ID}, + ) + + with build_env: + build_env.run('echo', 'test', record=False) + self.assertTrue(self.mocks.process.communicate.called) + self.assertTrue(build_env.done) + self.assertTrue(build_env.successful) + self.assertEqual(len(build_env.commands), 1) + self.assertEqual(build_env.commands[0].output, u'This is okay') + + # api() is not called anymore, we use api_v2 instead + self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) + # The command was not saved + self.assertFalse(self.mocks.mocks['api_v2.command'].post.called) + self.mocks.mocks['api_v2.build']().put.assert_called_with({ + 'id': DUMMY_BUILD_ID, + 'version': self.version.pk, + 'success': True, + 'project': self.project.pk, + 'setup_error': u'', + 'length': mock.ANY, + 'error': '', + 'setup': u'', + 'output': u'', + 'state': u'finished', + 'builder': mock.ANY, + 'exit_code': 0, + }) + def test_incremental_state_update_with_no_update(self): """Build updates to a non-finished state when update_on_success=True.""" build_envs = [ @@ -630,6 +670,51 @@ def test_command_execution(self): 'builder': mock.ANY, }) + def test_command_not_recorded(self): + """Command execution through Docker without record the command.""" + self.mocks.configure_mock( + 'docker_client', { + 'exec_create.return_value': {'Id': b'container-foobar'}, + 'exec_start.return_value': b'This is the return', + 'exec_inspect.return_value': {'ExitCode': 1}, + }) + + build_env = DockerBuildEnvironment( + version=self.version, + project=self.project, + build={'id': DUMMY_BUILD_ID}, + ) + + with build_env: + build_env.run('echo test', cwd='/tmp', record=False) + + self.mocks.docker_client.exec_create.assert_called_with( + container='build-123-project-6-pip', + cmd="/bin/sh -c 'cd /tmp && echo\\ test'", stderr=True, stdout=True) + self.assertEqual(build_env.commands[0].exit_code, 1) + self.assertEqual(build_env.commands[0].output, u'This is the return') + self.assertEqual(build_env.commands[0].error, None) + self.assertTrue(build_env.failed) + + # api() is not called anymore, we use api_v2 instead + self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) + # The command was not saved + self.assertFalse(self.mocks.mocks['api_v2.command'].post.called) + self.mocks.mocks['api_v2.build']().put.assert_called_with({ + 'id': DUMMY_BUILD_ID, + 'version': self.version.pk, + 'success': False, + 'project': self.project.pk, + 'setup_error': u'', + 'exit_code': 1, + 'length': 0, + 'error': '', + 'setup': u'', + 'output': u'', + 'state': u'finished', + 'builder': mock.ANY, + }) + def test_command_execution_cleanup_exception(self): """Command execution through Docker, catch exception during cleanup.""" response = Mock(status_code=500, reason='Because') From bf4a181c3ea017e30b151bb393bfec932936f20c Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Sun, 11 Mar 2018 15:57:00 -0500 Subject: [PATCH 08/20] Add test record_as_success --- .../rtd_tests/tests/test_doc_building.py | 101 ++++++++++++++++++ 1 file changed, 101 insertions(+) diff --git a/readthedocs/rtd_tests/tests/test_doc_building.py b/readthedocs/rtd_tests/tests/test_doc_building.py index d931b62c12d..0ccb958dca6 100644 --- a/readthedocs/rtd_tests/tests/test_doc_building.py +++ b/readthedocs/rtd_tests/tests/test_doc_building.py @@ -144,6 +144,54 @@ def test_command_not_recorded(self): 'exit_code': 0, }) + def test_record_command_as_success(self): + self.mocks.configure_mock('process', { + 'communicate.return_value': (b'This is okay', '') + }) + type(self.mocks.process).returncode = PropertyMock(return_value=1) + + build_env = LocalBuildEnvironment( + version=self.version, + project=self.project, + build={'id': DUMMY_BUILD_ID}, + ) + + with build_env: + build_env.run('echo', 'test', record_as_success=True) + self.assertTrue(self.mocks.process.communicate.called) + self.assertTrue(build_env.done) + self.assertFalse(build_env.successful) + self.assertEqual(len(build_env.commands), 1) + self.assertEqual(build_env.commands[0].output, u'This is okay') + + # api() is not called anymore, we use api_v2 instead + self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) + # The command was saved + command = build_env.commands[0] + self.mocks.mocks['api_v2.command'].post.assert_called_once_with({ + 'build': DUMMY_BUILD_ID, + 'command': command.get_command(), + 'description': command.description, + 'output': command.output, + 'exit_code': 0, + 'start_time': command.start_time, + 'end_time': command.end_time, + }) + self.mocks.mocks['api_v2.build']().put.assert_called_with({ + 'id': DUMMY_BUILD_ID, + 'version': self.version.pk, + 'success': False, + 'project': self.project.pk, + 'setup_error': u'', + 'length': mock.ANY, + 'error': '', + 'setup': u'', + 'output': u'', + 'state': u'finished', + 'builder': mock.ANY, + 'exit_code': 1, + }) + def test_incremental_state_update_with_no_update(self): """Build updates to a non-finished state when update_on_success=True.""" build_envs = [ @@ -715,6 +763,59 @@ def test_command_not_recorded(self): 'builder': mock.ANY, }) + def test_record_command_as_success(self): + self.mocks.configure_mock( + 'docker_client', { + 'exec_create.return_value': {'Id': b'container-foobar'}, + 'exec_start.return_value': b'This is the return', + 'exec_inspect.return_value': {'ExitCode': 1}, + }) + + build_env = DockerBuildEnvironment( + version=self.version, + project=self.project, + build={'id': DUMMY_BUILD_ID}, + ) + + with build_env: + build_env.run('echo test', cwd='/tmp', record_as_success=True) + + self.mocks.docker_client.exec_create.assert_called_with( + container='build-123-project-6-pip', + cmd="/bin/sh -c 'cd /tmp && echo\\ test'", stderr=True, stdout=True) + self.assertEqual(build_env.commands[0].exit_code, 1) + self.assertEqual(build_env.commands[0].output, u'This is the return') + self.assertEqual(build_env.commands[0].error, None) + self.assertTrue(build_env.failed) + + # api() is not called anymore, we use api_v2 instead + self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) + # The command was saved + command = build_env.commands[0] + self.mocks.mocks['api_v2.command'].post.assert_called_once_with({ + 'build': DUMMY_BUILD_ID, + 'command': command.get_command(), + 'description': command.description, + 'output': command.output, + 'exit_code': 0, + 'start_time': command.start_time, + 'end_time': command.end_time, + }) + self.mocks.mocks['api_v2.build']().put.assert_called_with({ + 'id': DUMMY_BUILD_ID, + 'version': self.version.pk, + 'success': False, + 'project': self.project.pk, + 'setup_error': u'', + 'exit_code': 1, + 'length': 0, + 'error': '', + 'setup': u'', + 'output': u'', + 'state': u'finished', + 'builder': mock.ANY, + }) + def test_command_execution_cleanup_exception(self): """Command execution through Docker, catch exception during cleanup.""" response = Mock(status_code=500, reason='Because') From 35fdca4a5259ee061bcee32d37d95da28ca94a24 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Sun, 11 Mar 2018 16:24:51 -0500 Subject: [PATCH 09/20] Expose failed test --- readthedocs/rtd_tests/tests/test_doc_building.py | 1 + 1 file changed, 1 insertion(+) diff --git a/readthedocs/rtd_tests/tests/test_doc_building.py b/readthedocs/rtd_tests/tests/test_doc_building.py index 0ccb958dca6..b7bc02958a8 100644 --- a/readthedocs/rtd_tests/tests/test_doc_building.py +++ b/readthedocs/rtd_tests/tests/test_doc_building.py @@ -228,6 +228,7 @@ def test_incremental_state_update_with_no_update(self): 'builder': mock.ANY, 'exit_code': 0, }) + self.assertIsNone(build_env.failure) def test_failing_execution(self): """Build in failing state.""" From cdb15c063fa89fb7dd3701a60d514c05bd4e0781 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Sun, 11 Mar 2018 16:29:10 -0500 Subject: [PATCH 10/20] Fix falling test --- readthedocs/rtd_tests/tests/test_doc_building.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_doc_building.py b/readthedocs/rtd_tests/tests/test_doc_building.py index b7bc02958a8..d3faf244bc5 100644 --- a/readthedocs/rtd_tests/tests/test_doc_building.py +++ b/readthedocs/rtd_tests/tests/test_doc_building.py @@ -209,15 +209,11 @@ def test_incremental_state_update_with_no_update(self): ] for build_env in build_envs: - # __import__('pdb').set_trace() with build_env: build_env.update_build(BUILD_STATE_CLONING) - # FIXME: This method raise an assert exception, - # but is captured by the context manager self.mocks.mocks['api_v2.build']().put.assert_called_with({ - 'id': 'Just for testing', + 'id': DUMMY_BUILD_ID, 'version': self.version.pk, - 'success': True, 'project': self.project.pk, 'setup_error': u'', 'length': mock.ANY, @@ -226,9 +222,10 @@ def test_incremental_state_update_with_no_update(self): 'output': u'', 'state': BUILD_STATE_CLONING, 'builder': mock.ANY, - 'exit_code': 0, }) self.assertIsNone(build_env.failure) + # No commands were saved + self.assertFalse(self.mocks.mocks['api_v2.build'].post.called) def test_failing_execution(self): """Build in failing state.""" From 5c158266f08d44e018e143fe7d0743f8e8a16651 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Fri, 23 Mar 2018 17:45:33 -0500 Subject: [PATCH 11/20] Better comments --- .../rtd_tests/tests/test_doc_building.py | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_doc_building.py b/readthedocs/rtd_tests/tests/test_doc_building.py index d3faf244bc5..ea1716031c9 100644 --- a/readthedocs/rtd_tests/tests/test_doc_building.py +++ b/readthedocs/rtd_tests/tests/test_doc_building.py @@ -224,8 +224,8 @@ def test_incremental_state_update_with_no_update(self): 'builder': mock.ANY, }) self.assertIsNone(build_env.failure) - # No commands were saved - self.assertFalse(self.mocks.mocks['api_v2.build'].post.called) + # The build failed before executing any command + self.assertFalse(self.mocks.mocks['api_v2.command'].post.called) def test_failing_execution(self): """Build in failing state.""" @@ -295,7 +295,7 @@ def test_failing_execution_with_caught_exception(self): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) - # The command was not saved + # The build failed before executing any command self.assertFalse(self.mocks.mocks['api_v2.command'].post.called) self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, @@ -329,7 +329,7 @@ def test_failing_execution_with_unexpected_exception(self): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) - # The command was not saved + # The build failed before executing any command self.assertFalse(self.mocks.mocks['api_v2.command'].post.called) self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, @@ -389,7 +389,7 @@ def test_environment_successful_build(self): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) - # The command was not saved + # No command were executed self.assertFalse(self.mocks.mocks['api_v2.command'].post.called) self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, @@ -421,7 +421,7 @@ def test_environment_successful_build_without_update(self): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) - # The command was not saved + # No command were executed self.assertFalse(self.mocks.mocks['api_v2.command'].post.called) self.assertFalse(self.mocks.mocks['api_v2.build']().put.called) @@ -441,7 +441,7 @@ def test_environment_failed_build_without_update_but_with_error(self): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) - # The command was not saved + # No commands were executed self.assertFalse(self.mocks.mocks['api_v2.command'].post.called) self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, @@ -520,7 +520,7 @@ def _inner(): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) - # The command was not saved + # No commands were executed self.assertFalse(self.mocks.mocks['api_v2.command'].post.called) self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, @@ -605,7 +605,7 @@ def test_api_failure_on_error_in_exit(self): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) - # The command was not saved + # No command were executed self.assertFalse(self.mocks.mocks['api_v2.command'].post.called) self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, @@ -645,7 +645,7 @@ def test_api_failure_returns_previous_error_on_error_in_exit(self): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) - # The command was not saved + # No commands were executed self.assertFalse(self.mocks.mocks['api_v2.command'].post.called) self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, @@ -898,7 +898,7 @@ def _inner(): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) - # The command was not saved + # The build failed before executing any command self.assertFalse(self.mocks.mocks['api_v2.command'].post.called) self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, From 880a8634150e02f09dc0f89ea56daa77fc2064be Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Fri, 23 Mar 2018 17:46:25 -0500 Subject: [PATCH 12/20] Use literal unicode strings --- .../rtd_tests/tests/test_doc_building.py | 120 +++++++++--------- 1 file changed, 60 insertions(+), 60 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_doc_building.py b/readthedocs/rtd_tests/tests/test_doc_building.py index ea1716031c9..52c8312e20d 100644 --- a/readthedocs/rtd_tests/tests/test_doc_building.py +++ b/readthedocs/rtd_tests/tests/test_doc_building.py @@ -97,9 +97,9 @@ def test_normal_execution(self): 'setup_error': u'', 'length': mock.ANY, 'error': '', - 'setup': u'', - 'output': u'', - 'state': u'finished', + 'setup': '', + 'output': '', + 'state': 'finished', 'builder': mock.ANY, 'exit_code': 0, }) @@ -134,12 +134,12 @@ def test_command_not_recorded(self): 'version': self.version.pk, 'success': True, 'project': self.project.pk, - 'setup_error': u'', + 'setup_error': '', 'length': mock.ANY, 'error': '', - 'setup': u'', - 'output': u'', - 'state': u'finished', + 'setup': '', + 'output': '', + 'state': 'finished', 'builder': mock.ANY, 'exit_code': 0, }) @@ -185,9 +185,9 @@ def test_record_command_as_success(self): 'setup_error': u'', 'length': mock.ANY, 'error': '', - 'setup': u'', - 'output': u'', - 'state': u'finished', + 'setup': '', + 'output': '', + 'state': 'finished', 'builder': mock.ANY, 'exit_code': 1, }) @@ -215,11 +215,11 @@ def test_incremental_state_update_with_no_update(self): 'id': DUMMY_BUILD_ID, 'version': self.version.pk, 'project': self.project.pk, - 'setup_error': u'', + 'setup_error': '', 'length': mock.ANY, 'error': '', - 'setup': u'', - 'output': u'', + 'setup': '', + 'output': '', 'state': BUILD_STATE_CLONING, 'builder': mock.ANY, }) @@ -270,9 +270,9 @@ def test_failing_execution(self): 'setup_error': u'', 'length': mock.ANY, 'error': '', - 'setup': u'', - 'output': u'', - 'state': u'finished', + 'setup': '', + 'output': '', + 'state': 'finished', 'builder': mock.ANY, 'exit_code': 1, }) @@ -302,12 +302,12 @@ def test_failing_execution_with_caught_exception(self): 'version': self.version.pk, 'success': False, 'project': self.project.pk, - 'setup_error': u'', + 'setup_error': '', 'length': mock.ANY, 'error': 'Foobar', - 'setup': u'', - 'output': u'', - 'state': u'finished', + 'setup': '', + 'output': '', + 'state': 'finished', 'builder': mock.ANY, 'exit_code': 1, }) @@ -336,15 +336,15 @@ def test_failing_execution_with_unexpected_exception(self): 'version': self.version.pk, 'success': False, 'project': self.project.pk, - 'setup_error': u'', + 'setup_error': '', 'length': mock.ANY, 'error': ( 'There was a problem with Read the Docs while building your ' 'documentation. Please report this to us with your build id (123).' ), - 'setup': u'', - 'output': u'', - 'state': u'finished', + 'setup': '', + 'output': '', + 'state': 'finished', 'builder': mock.ANY, }) @@ -396,12 +396,12 @@ def test_environment_successful_build(self): 'version': self.version.pk, 'success': True, 'project': self.project.pk, - 'setup_error': u'', + 'setup_error': '', 'length': 0, 'error': '', - 'setup': u'', - 'output': u'', - 'state': u'finished', + 'setup': '', + 'output': '', + 'state': 'finished', 'builder': mock.ANY, }) @@ -448,13 +448,13 @@ def test_environment_failed_build_without_update_but_with_error(self): 'version': self.version.pk, 'success': False, 'project': self.project.pk, - 'setup_error': u'', + 'setup_error': '', 'exit_code': 1, 'length': 0, 'error': 'Test', - 'setup': u'', - 'output': u'', - 'state': u'finished', + 'setup': '', + 'output': '', + 'state': 'finished', 'builder': mock.ANY, }) @@ -482,7 +482,7 @@ def _inner(): 'version': self.version.pk, 'success': False, 'project': self.project.pk, - 'setup_error': u'', + 'setup_error': '', 'exit_code': 1, 'length': 0, 'error': ( @@ -490,9 +490,9 @@ def _inner(): "documentation. Please report this to us with your build id " "(123)." ), - 'setup': u'', - 'output': u'', - 'state': u'finished', + 'setup': '', + 'output': '', + 'state': 'finished', 'builder': mock.ANY, }) @@ -612,13 +612,13 @@ def test_api_failure_on_error_in_exit(self): 'version': self.version.pk, 'success': False, 'project': self.project.pk, - 'setup_error': u'', + 'setup_error': '', 'exit_code': 1, 'length': 0, 'error': 'Failed', - 'setup': u'', - 'output': u'', - 'state': u'finished', + 'setup': '', + 'output': '', + 'state': 'finished', 'builder': mock.ANY, }) @@ -652,13 +652,13 @@ def test_api_failure_returns_previous_error_on_error_in_exit(self): 'version': self.version.pk, 'success': False, 'project': self.project.pk, - 'setup_error': u'', + 'setup_error': '', 'exit_code': 1, 'length': 0, 'error': 'Inner failed', - 'setup': u'', - 'output': u'', - 'state': u'finished', + 'setup': '', + 'output': '', + 'state': 'finished', 'builder': mock.ANY, }) @@ -706,13 +706,13 @@ def test_command_execution(self): 'version': self.version.pk, 'success': False, 'project': self.project.pk, - 'setup_error': u'', + 'setup_error': '', 'exit_code': 1, 'length': 0, 'error': '', - 'setup': u'', - 'output': u'', - 'state': u'finished', + 'setup': '', + 'output': '', + 'state': 'finished', 'builder': mock.ANY, }) @@ -751,13 +751,13 @@ def test_command_not_recorded(self): 'version': self.version.pk, 'success': False, 'project': self.project.pk, - 'setup_error': u'', + 'setup_error': '', 'exit_code': 1, 'length': 0, 'error': '', - 'setup': u'', - 'output': u'', - 'state': u'finished', + 'setup': '', + 'output': '', + 'state': 'finished', 'builder': mock.ANY, }) @@ -804,13 +804,13 @@ def test_record_command_as_success(self): 'version': self.version.pk, 'success': False, 'project': self.project.pk, - 'setup_error': u'', + 'setup_error': '', 'exit_code': 1, 'length': 0, 'error': '', - 'setup': u'', - 'output': u'', - 'state': u'finished', + 'setup': '', + 'output': '', + 'state': 'finished', 'builder': mock.ANY, }) @@ -905,13 +905,13 @@ def _inner(): 'version': self.version.pk, 'success': False, 'project': self.project.pk, - 'setup_error': u'', + 'setup_error': '', 'exit_code': 1, 'length': 0, 'error': 'A build environment is currently running for this version', - 'setup': u'', - 'output': u'', - 'state': u'finished', + 'setup': '', + 'output': '', + 'state': 'finished', 'builder': mock.ANY, }) From f1f7d7e4e7982f9d658ffcf3cde790a1bc8df6d7 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Fri, 23 Mar 2018 18:07:15 -0500 Subject: [PATCH 13/20] Better comment --- readthedocs/rtd_tests/tests/test_doc_building.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_doc_building.py b/readthedocs/rtd_tests/tests/test_doc_building.py index 52c8312e20d..002c060041f 100644 --- a/readthedocs/rtd_tests/tests/test_doc_building.py +++ b/readthedocs/rtd_tests/tests/test_doc_building.py @@ -389,7 +389,7 @@ def test_environment_successful_build(self): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) - # No command were executed + # No commands were executed self.assertFalse(self.mocks.mocks['api_v2.command'].post.called) self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, @@ -421,7 +421,7 @@ def test_environment_successful_build_without_update(self): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) - # No command were executed + # No commands were executed self.assertFalse(self.mocks.mocks['api_v2.command'].post.called) self.assertFalse(self.mocks.mocks['api_v2.build']().put.called) @@ -475,7 +475,7 @@ def _inner(): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) - # The command was not saved + # No commands were executed self.assertFalse(self.mocks.mocks['api_v2.command'].post.called) self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, @@ -605,7 +605,7 @@ def test_api_failure_on_error_in_exit(self): # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) - # No command were executed + # No commands were executed self.assertFalse(self.mocks.mocks['api_v2.command'].post.called) self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, From 38fede09ddba8f44e1347c1e365b49977f1c0947 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Fri, 23 Mar 2018 18:15:42 -0500 Subject: [PATCH 14/20] Fix test Only recorded commands are added to the list --- readthedocs/rtd_tests/tests/test_doc_building.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/rtd_tests/tests/test_doc_building.py b/readthedocs/rtd_tests/tests/test_doc_building.py index 002c060041f..409c193a4aa 100644 --- a/readthedocs/rtd_tests/tests/test_doc_building.py +++ b/readthedocs/rtd_tests/tests/test_doc_building.py @@ -122,7 +122,7 @@ def test_command_not_recorded(self): self.assertTrue(self.mocks.process.communicate.called) self.assertTrue(build_env.done) self.assertTrue(build_env.successful) - self.assertEqual(len(build_env.commands), 1) + self.assertEqual(len(build_env.commands), 0) self.assertEqual(build_env.commands[0].output, u'This is okay') # api() is not called anymore, we use api_v2 instead From 81ad90da0408398f98718385a59b181051071d0d Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Fri, 23 Mar 2018 18:17:07 -0500 Subject: [PATCH 15/20] Fix test If the command fails and it's recorded as success the build does not fail --- readthedocs/rtd_tests/tests/test_doc_building.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/rtd_tests/tests/test_doc_building.py b/readthedocs/rtd_tests/tests/test_doc_building.py index 409c193a4aa..55c4ede23b7 100644 --- a/readthedocs/rtd_tests/tests/test_doc_building.py +++ b/readthedocs/rtd_tests/tests/test_doc_building.py @@ -160,7 +160,7 @@ def test_record_command_as_success(self): build_env.run('echo', 'test', record_as_success=True) self.assertTrue(self.mocks.process.communicate.called) self.assertTrue(build_env.done) - self.assertFalse(build_env.successful) + self.assertTrue(build_env.successful) self.assertEqual(len(build_env.commands), 1) self.assertEqual(build_env.commands[0].output, u'This is okay') From 4b6a64ee3aa7aecc92c27654c1c3bc906238f4f2 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Fri, 23 Mar 2018 18:18:42 -0500 Subject: [PATCH 16/20] Fix test The command is saved as success, the original exit code is lost --- readthedocs/rtd_tests/tests/test_doc_building.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/rtd_tests/tests/test_doc_building.py b/readthedocs/rtd_tests/tests/test_doc_building.py index 55c4ede23b7..be26d03694a 100644 --- a/readthedocs/rtd_tests/tests/test_doc_building.py +++ b/readthedocs/rtd_tests/tests/test_doc_building.py @@ -781,7 +781,7 @@ def test_record_command_as_success(self): self.mocks.docker_client.exec_create.assert_called_with( container='build-123-project-6-pip', cmd="/bin/sh -c 'cd /tmp && echo\\ test'", stderr=True, stdout=True) - self.assertEqual(build_env.commands[0].exit_code, 1) + self.assertEqual(build_env.commands[0].exit_code, 0) self.assertEqual(build_env.commands[0].output, u'This is the return') self.assertEqual(build_env.commands[0].error, None) self.assertTrue(build_env.failed) From 09a2487f5a12e16b44df8ce8af9e1d03efeb7a5b Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Fri, 23 Mar 2018 18:22:08 -0500 Subject: [PATCH 17/20] Fix test There is no recorded command --- readthedocs/rtd_tests/tests/test_doc_building.py | 1 - 1 file changed, 1 deletion(-) diff --git a/readthedocs/rtd_tests/tests/test_doc_building.py b/readthedocs/rtd_tests/tests/test_doc_building.py index be26d03694a..26467200ada 100644 --- a/readthedocs/rtd_tests/tests/test_doc_building.py +++ b/readthedocs/rtd_tests/tests/test_doc_building.py @@ -123,7 +123,6 @@ def test_command_not_recorded(self): self.assertTrue(build_env.done) self.assertTrue(build_env.successful) self.assertEqual(len(build_env.commands), 0) - self.assertEqual(build_env.commands[0].output, u'This is okay') # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) From 04308a69bbbe4d8bf1da817419fd4a17846ac70d Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Fri, 23 Mar 2018 18:29:53 -0500 Subject: [PATCH 18/20] Fix test The build is show as success if the command fails but it is recorded as success. --- readthedocs/rtd_tests/tests/test_doc_building.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/rtd_tests/tests/test_doc_building.py b/readthedocs/rtd_tests/tests/test_doc_building.py index 26467200ada..92c5088ebfa 100644 --- a/readthedocs/rtd_tests/tests/test_doc_building.py +++ b/readthedocs/rtd_tests/tests/test_doc_building.py @@ -783,7 +783,7 @@ def test_record_command_as_success(self): self.assertEqual(build_env.commands[0].exit_code, 0) self.assertEqual(build_env.commands[0].output, u'This is the return') self.assertEqual(build_env.commands[0].error, None) - self.assertTrue(build_env.failed) + self.assertFalse(build_env.failed) # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) From 52c6875f0b402b16d7e5fd2d4e5c15c5dde9c3e3 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Fri, 23 Mar 2018 18:34:04 -0500 Subject: [PATCH 19/20] Fix test When the command is not recorded we lose that information --- readthedocs/rtd_tests/tests/test_doc_building.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_doc_building.py b/readthedocs/rtd_tests/tests/test_doc_building.py index 92c5088ebfa..650329029ca 100644 --- a/readthedocs/rtd_tests/tests/test_doc_building.py +++ b/readthedocs/rtd_tests/tests/test_doc_building.py @@ -736,10 +736,8 @@ def test_command_not_recorded(self): self.mocks.docker_client.exec_create.assert_called_with( container='build-123-project-6-pip', cmd="/bin/sh -c 'cd /tmp && echo\\ test'", stderr=True, stdout=True) - self.assertEqual(build_env.commands[0].exit_code, 1) - self.assertEqual(build_env.commands[0].output, u'This is the return') - self.assertEqual(build_env.commands[0].error, None) - self.assertTrue(build_env.failed) + self.assertEqual(len(build_env.commands), 0) + self.assertFalse(build_env.failed) # api() is not called anymore, we use api_v2 instead self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called) From ef39fe05003e393e735e73b8a391f971beb74101 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Fri, 23 Mar 2018 18:48:02 -0500 Subject: [PATCH 20/20] Fix tests Now the build is recorded as success --- readthedocs/rtd_tests/tests/test_doc_building.py | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_doc_building.py b/readthedocs/rtd_tests/tests/test_doc_building.py index 650329029ca..4b678ce085f 100644 --- a/readthedocs/rtd_tests/tests/test_doc_building.py +++ b/readthedocs/rtd_tests/tests/test_doc_building.py @@ -140,7 +140,6 @@ def test_command_not_recorded(self): 'output': '', 'state': 'finished', 'builder': mock.ANY, - 'exit_code': 0, }) def test_record_command_as_success(self): @@ -179,7 +178,7 @@ def test_record_command_as_success(self): self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, 'version': self.version.pk, - 'success': False, + 'success': True, 'project': self.project.pk, 'setup_error': u'', 'length': mock.ANY, @@ -188,7 +187,7 @@ def test_record_command_as_success(self): 'output': '', 'state': 'finished', 'builder': mock.ANY, - 'exit_code': 1, + 'exit_code': 0, }) def test_incremental_state_update_with_no_update(self): @@ -746,10 +745,9 @@ def test_command_not_recorded(self): self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, 'version': self.version.pk, - 'success': False, + 'success': True, 'project': self.project.pk, 'setup_error': '', - 'exit_code': 1, 'length': 0, 'error': '', 'setup': '', @@ -799,10 +797,10 @@ def test_record_command_as_success(self): self.mocks.mocks['api_v2.build']().put.assert_called_with({ 'id': DUMMY_BUILD_ID, 'version': self.version.pk, - 'success': False, + 'success': True, 'project': self.project.pk, 'setup_error': '', - 'exit_code': 1, + 'exit_code': 0, 'length': 0, 'error': '', 'setup': '',