Re: Review Request 27058: Add specs to instances of Mock in Python tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27058/ --- (Updated Oct. 24, 2014, 8:09 p.m.) Review request for Aurora, Mark Chu-Carroll and Zameer Manji. Changes --- rebase. Bugs: AURORA-248 https://issues.apache.org/jira/browse/AURORA-248 Repository: aurora Description --- Use of Mock() without a specification considered harmful. I went through and updated as many mocks as I could. Any remaining can be classified as: 1) Mocks of classes that cannot be spec'd. Almost all instances of SchedulerProxy, which uses __getattr__ to delegate to the read or write client. 2) Primitives like strings and callback functions or data objects like dicts and pystachio structs. 3) Weird mocks that broke code where they really shouldn't have (off the top of my head - in test_diff.py and commands/test_run.py) - both when they were spec'd and when they were replaced with real thrift structs. The remaining offenders: $ grep -r --include=*.py Mock() src/test/python src/test/python/apache/aurora/admin/test_host_maintenance.py:mock_callback = mock.Mock() src/test/python/apache/aurora/admin/test_host_maintenance.py:mock_callback = mock.Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_proxy = Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_get = Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_task_config = Mock() src/test/python/apache/aurora/client/api/test_job_monitor.py: self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_quota_check.py: self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_scheduler_client.py: client._connect_scheduler = mock.MagicMock() src/test/python/apache/aurora/client/api/test_sla.py:self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_task_util.py:scheduler = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: job = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: job.assignedTask.task.executorConfig.data = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as (_, _, subprocess_patch, _): src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/cli/test_inspect.py:raw_config = Mock() src/test/python/apache/aurora/client/cli/test_inspect.py:mock_task = Mock() src/test/python/apache/aurora/client/cli/test_inspect.py:mock_process = Mock() src/test/python/apache/aurora/client/cli/test_kill.py:mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/test_kill.py:mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/test_kill.py:mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/test_kill.py:mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_scheduler = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_scheduler_client = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_api_factory = Mock() src/test/python/apache/aurora/client/commands/test_diff.py: job = Mock() src/test/python/apache/aurora/client/commands/test_diff.py: job.assignedTask.task.executorConfig.data = Mock() src/test/python/apache/aurora/client/commands/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/commands/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/commands/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/commands/test_listjobs.py:mock_options = Mock() src/test/python/apache/aurora/client/commands/test_listjobs.py: job = Mock() src/test/python/apache/aurora/client/commands/test_maintenance.py: mock_callback = Mock() src/test/python/apache/aurora/client/commands/test_maintenance.py: mock_wait = Mock() src/test/python/apache/aurora/client/commands/util.py:mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/commands/util.py:mock_api_factory = Mock() Diffs (updated) - src/test/python/apache/aurora/admin/test_admin_util.py f5c8c69c1109d15ee3886fb863014c3285240db1 src/test/python/apache/aurora/client/cli/test_command_hooks.py 60c75300501c36ac20a97f78ff18b3ca7af30699 src/test/python/apache/aurora/client/cli/test_cron.py c7b71c29d44150162fec8066947623fa91815424 src/test/python/apache/aurora/client/cli/test_diff.py
Re: Review Request 27058: Add specs to instances of Mock in Python tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27058/#review58376 --- Ship it! Ship It! - Zameer Manji On Oct. 24, 2014, 1:09 p.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27058/ --- (Updated Oct. 24, 2014, 1:09 p.m.) Review request for Aurora, Mark Chu-Carroll and Zameer Manji. Bugs: AURORA-248 https://issues.apache.org/jira/browse/AURORA-248 Repository: aurora Description --- Use of Mock() without a specification considered harmful. I went through and updated as many mocks as I could. Any remaining can be classified as: 1) Mocks of classes that cannot be spec'd. Almost all instances of SchedulerProxy, which uses __getattr__ to delegate to the read or write client. 2) Primitives like strings and callback functions or data objects like dicts and pystachio structs. 3) Weird mocks that broke code where they really shouldn't have (off the top of my head - in test_diff.py and commands/test_run.py) - both when they were spec'd and when they were replaced with real thrift structs. The remaining offenders: $ grep -r --include=*.py Mock() src/test/python src/test/python/apache/aurora/admin/test_host_maintenance.py: mock_callback = mock.Mock() src/test/python/apache/aurora/admin/test_host_maintenance.py: mock_callback = mock.Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_proxy = Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_get = Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_task_config = Mock() src/test/python/apache/aurora/client/api/test_job_monitor.py: self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_quota_check.py: self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_scheduler_client.py: client._connect_scheduler = mock.MagicMock() src/test/python/apache/aurora/client/api/test_sla.py:self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_task_util.py:scheduler = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: job = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: job.assignedTask.task.executorConfig.data = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as (_, _, subprocess_patch, _): src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/cli/test_inspect.py:raw_config = Mock() src/test/python/apache/aurora/client/cli/test_inspect.py:mock_task = Mock() src/test/python/apache/aurora/client/cli/test_inspect.py:mock_process = Mock() src/test/python/apache/aurora/client/cli/test_kill.py: mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/test_kill.py: mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/test_kill.py: mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/test_kill.py: mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_scheduler = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_scheduler_client = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_api_factory = Mock() src/test/python/apache/aurora/client/commands/test_diff.py: job = Mock() src/test/python/apache/aurora/client/commands/test_diff.py: job.assignedTask.task.executorConfig.data = Mock() src/test/python/apache/aurora/client/commands/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/commands/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/commands/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/commands/test_listjobs.py: mock_options = Mock() src/test/python/apache/aurora/client/commands/test_listjobs.py: job = Mock() src/test/python/apache/aurora/client/commands/test_maintenance.py: mock_callback = Mock() src/test/python/apache/aurora/client/commands/test_maintenance.py: mock_wait = Mock() src/test/python/apache/aurora/client/commands/util.py: mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/commands/util.py:mock_api_factory = Mock()
Re: Review Request 27058: Add specs to instances of Mock in Python tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27058/#review58403 --- -1: Master (5be667f) is red with this patch. ./build-support/jenkins/build.sh src.test.python.apache.aurora.client.api.updater_util . SUCCESS src.test.python.apache.aurora.client.binding_helper . SUCCESS src.test.python.apache.aurora.client.cli.api . SUCCESS src.test.python.apache.aurora.client.cli.bridge . SUCCESS src.test.python.apache.aurora.client.cli.command_hooks . SUCCESS src.test.python.apache.aurora.client.cli.config . SUCCESS src.test.python.apache.aurora.client.cli.cron . SUCCESS src.test.python.apache.aurora.client.cli.help . SUCCESS src.test.python.apache.aurora.client.cli.inspect . SUCCESS src.test.python.apache.aurora.client.cli.job . SUCCESS src.test.python.apache.aurora.client.cli.logging . SUCCESS src.test.python.apache.aurora.client.cli.plugins . SUCCESS src.test.python.apache.aurora.client.cli.quota . SUCCESS src.test.python.apache.aurora.client.cli.sla . SUCCESS src.test.python.apache.aurora.client.cli.supdate . SUCCESS src.test.python.apache.aurora.client.cli.task . SUCCESS src.test.python.apache.aurora.client.cli.update . SUCCESS src.test.python.apache.aurora.client.commands.admin . SUCCESS src.test.python.apache.aurora.client.commands.core . FAILURE src.test.python.apache.aurora.client.config . SUCCESS - Aurora ReviewBot On Oct. 24, 2014, 8:09 p.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27058/ --- (Updated Oct. 24, 2014, 8:09 p.m.) Review request for Aurora, Mark Chu-Carroll and Zameer Manji. Bugs: AURORA-248 https://issues.apache.org/jira/browse/AURORA-248 Repository: aurora Description --- Use of Mock() without a specification considered harmful. I went through and updated as many mocks as I could. Any remaining can be classified as: 1) Mocks of classes that cannot be spec'd. Almost all instances of SchedulerProxy, which uses __getattr__ to delegate to the read or write client. 2) Primitives like strings and callback functions or data objects like dicts and pystachio structs. 3) Weird mocks that broke code where they really shouldn't have (off the top of my head - in test_diff.py and commands/test_run.py) - both when they were spec'd and when they were replaced with real thrift structs. The remaining offenders: $ grep -r --include=*.py Mock() src/test/python src/test/python/apache/aurora/admin/test_host_maintenance.py: mock_callback = mock.Mock() src/test/python/apache/aurora/admin/test_host_maintenance.py: mock_callback = mock.Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_proxy = Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_get = Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_task_config = Mock() src/test/python/apache/aurora/client/api/test_job_monitor.py: self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_quota_check.py: self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_scheduler_client.py: client._connect_scheduler = mock.MagicMock() src/test/python/apache/aurora/client/api/test_sla.py:self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_task_util.py:scheduler = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: job = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: job.assignedTask.task.executorConfig.data = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as (_, _, subprocess_patch, _): src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as (
Re: Review Request 27058: Add specs to instances of Mock in Python tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27058/ --- (Updated Oct. 24, 2014, 10:48 p.m.) Review request for Aurora, Mark Chu-Carroll and Zameer Manji. Changes --- Thanks ReviewBot! I forgot to run the commands target rather than just the cli one. Bugs: AURORA-248 https://issues.apache.org/jira/browse/AURORA-248 Repository: aurora Description --- Use of Mock() without a specification considered harmful. I went through and updated as many mocks as I could. Any remaining can be classified as: 1) Mocks of classes that cannot be spec'd. Almost all instances of SchedulerProxy, which uses __getattr__ to delegate to the read or write client. 2) Primitives like strings and callback functions or data objects like dicts and pystachio structs. 3) Weird mocks that broke code where they really shouldn't have (off the top of my head - in test_diff.py and commands/test_run.py) - both when they were spec'd and when they were replaced with real thrift structs. The remaining offenders: $ grep -r --include=*.py Mock() src/test/python src/test/python/apache/aurora/admin/test_host_maintenance.py:mock_callback = mock.Mock() src/test/python/apache/aurora/admin/test_host_maintenance.py:mock_callback = mock.Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_proxy = Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_get = Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_task_config = Mock() src/test/python/apache/aurora/client/api/test_job_monitor.py: self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_quota_check.py: self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_scheduler_client.py: client._connect_scheduler = mock.MagicMock() src/test/python/apache/aurora/client/api/test_sla.py:self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_task_util.py:scheduler = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: job = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: job.assignedTask.task.executorConfig.data = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as (_, _, subprocess_patch, _): src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/cli/test_inspect.py:raw_config = Mock() src/test/python/apache/aurora/client/cli/test_inspect.py:mock_task = Mock() src/test/python/apache/aurora/client/cli/test_inspect.py:mock_process = Mock() src/test/python/apache/aurora/client/cli/test_kill.py:mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/test_kill.py:mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/test_kill.py:mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/test_kill.py:mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_scheduler = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_scheduler_client = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_api_factory = Mock() src/test/python/apache/aurora/client/commands/test_diff.py: job = Mock() src/test/python/apache/aurora/client/commands/test_diff.py: job.assignedTask.task.executorConfig.data = Mock() src/test/python/apache/aurora/client/commands/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/commands/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/commands/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/commands/test_listjobs.py:mock_options = Mock() src/test/python/apache/aurora/client/commands/test_listjobs.py: job = Mock() src/test/python/apache/aurora/client/commands/test_maintenance.py: mock_callback = Mock() src/test/python/apache/aurora/client/commands/test_maintenance.py: mock_wait = Mock() src/test/python/apache/aurora/client/commands/util.py:mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/commands/util.py:mock_api_factory = Mock() Diffs (updated) - src/test/python/apache/aurora/admin/test_admin_util.py f5c8c69c1109d15ee3886fb863014c3285240db1 src/test/python/apache/aurora/client/cli/test_command_hooks.py 60c75300501c36ac20a97f78ff18b3ca7af30699 src/test/python/apache/aurora/client/cli/test_cron.py c7b71c29d44150162fec8066947623fa91815424
Re: Review Request 27058: Add specs to instances of Mock in Python tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27058/#review58420 --- Ship it! Long live ReviewBot! - Kevin Sweeney On Oct. 24, 2014, 3:48 p.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27058/ --- (Updated Oct. 24, 2014, 3:48 p.m.) Review request for Aurora, Mark Chu-Carroll and Zameer Manji. Bugs: AURORA-248 https://issues.apache.org/jira/browse/AURORA-248 Repository: aurora Description --- Use of Mock() without a specification considered harmful. I went through and updated as many mocks as I could. Any remaining can be classified as: 1) Mocks of classes that cannot be spec'd. Almost all instances of SchedulerProxy, which uses __getattr__ to delegate to the read or write client. 2) Primitives like strings and callback functions or data objects like dicts and pystachio structs. 3) Weird mocks that broke code where they really shouldn't have (off the top of my head - in test_diff.py and commands/test_run.py) - both when they were spec'd and when they were replaced with real thrift structs. The remaining offenders: $ grep -r --include=*.py Mock() src/test/python src/test/python/apache/aurora/admin/test_host_maintenance.py: mock_callback = mock.Mock() src/test/python/apache/aurora/admin/test_host_maintenance.py: mock_callback = mock.Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_proxy = Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_get = Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_task_config = Mock() src/test/python/apache/aurora/client/api/test_job_monitor.py: self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_quota_check.py: self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_scheduler_client.py: client._connect_scheduler = mock.MagicMock() src/test/python/apache/aurora/client/api/test_sla.py:self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_task_util.py:scheduler = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: job = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: job.assignedTask.task.executorConfig.data = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as (_, _, subprocess_patch, _): src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/cli/test_inspect.py:raw_config = Mock() src/test/python/apache/aurora/client/cli/test_inspect.py:mock_task = Mock() src/test/python/apache/aurora/client/cli/test_inspect.py:mock_process = Mock() src/test/python/apache/aurora/client/cli/test_kill.py: mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/test_kill.py: mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/test_kill.py: mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/test_kill.py: mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_scheduler = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_scheduler_client = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_api_factory = Mock() src/test/python/apache/aurora/client/commands/test_diff.py: job = Mock() src/test/python/apache/aurora/client/commands/test_diff.py: job.assignedTask.task.executorConfig.data = Mock() src/test/python/apache/aurora/client/commands/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/commands/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/commands/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/commands/test_listjobs.py: mock_options = Mock() src/test/python/apache/aurora/client/commands/test_listjobs.py: job = Mock() src/test/python/apache/aurora/client/commands/test_maintenance.py: mock_callback = Mock() src/test/python/apache/aurora/client/commands/test_maintenance.py: mock_wait = Mock() src/test/python/apache/aurora/client/commands/util.py: mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/commands/util.py:mock_api_factory =
Re: Review Request 27058: Add specs to instances of Mock in Python tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27058/#review58444 --- -1: Master (3778330) is red with this patch. ./build-support/jenkins/build.sh Skipping installation of /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter/__init__.py (namespace package) Skipping installation of /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter/common/__init__.py (namespace package) Installing /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter.common.lang-0.3.0-py2.7-nspkg.pth Successfully installed twitter.checkstyle pyflakes pep8 GitPython twitter.common.app gitdb twitter.common.process twitter.common.log twitter.common.util twitter.common.collections async smmap twitter.common.string twitter.common.options twitter.common.dirutil twitter.common.contextutil twitter.common.lang Cleaning up... F401:ERROR src/test/python/apache/aurora/client/cli/test_task_run.py:024-031 'ScheduledTask' imported but unused |from gen.apache.aurora.api.ttypes import ( |JobKey, |ResponseCode, |ScheduledTask, |ScheduleStatus, |ScheduleStatusResult, |TaskQuery |) E501:ERROR src/test/python/apache/aurora/client/commands/test_maintenance.py:045-046 line too long (101 100 characters) |mock_options = Mock(spec_set=['cluster', 'disable_all_hooks', 'duration', 'filename', 'grouping', | 'hosts', 'percentage', 'post_drain_script', 'reason', 'unsafe_hosts_filename', 'verbosity']) - Aurora ReviewBot On Oct. 24, 2014, 10:48 p.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27058/ --- (Updated Oct. 24, 2014, 10:48 p.m.) Review request for Aurora, Mark Chu-Carroll and Zameer Manji. Bugs: AURORA-248 https://issues.apache.org/jira/browse/AURORA-248 Repository: aurora Description --- Use of Mock() without a specification considered harmful. I went through and updated as many mocks as I could. Any remaining can be classified as: 1) Mocks of classes that cannot be spec'd. Almost all instances of SchedulerProxy, which uses __getattr__ to delegate to the read or write client. 2) Primitives like strings and callback functions or data objects like dicts and pystachio structs. 3) Weird mocks that broke code where they really shouldn't have (off the top of my head - in test_diff.py and commands/test_run.py) - both when they were spec'd and when they were replaced with real thrift structs. The remaining offenders: $ grep -r --include=*.py Mock() src/test/python src/test/python/apache/aurora/admin/test_host_maintenance.py: mock_callback = mock.Mock() src/test/python/apache/aurora/admin/test_host_maintenance.py: mock_callback = mock.Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_proxy = Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_get = Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_task_config = Mock() src/test/python/apache/aurora/client/api/test_job_monitor.py: self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_quota_check.py: self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_scheduler_client.py: client._connect_scheduler = mock.MagicMock() src/test/python/apache/aurora/client/api/test_sla.py:self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_task_util.py:scheduler = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: job = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: job.assignedTask.task.executorConfig.data = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as (_, _, subprocess_patch, _): src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/cli/test_inspect.py:raw_config = Mock() src/test/python/apache/aurora/client/cli/test_inspect.py:mock_task = Mock() src/test/python/apache/aurora/client/cli/test_inspect.py:mock_process = Mock() src/test/python/apache/aurora/client/cli/test_kill.py: mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/test_kill.py: mock_scheduler_proxy = Mock()
Re: Review Request 27058: Add specs to instances of Mock in Python tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27058/ --- (Updated Oct. 25, 2014, 12:24 a.m.) Review request for Aurora, Mark Chu-Carroll and Zameer Manji. Changes --- Okay ran _all_ tests, and also ran isort-check and checkstyle-check again. Bugs: AURORA-248 https://issues.apache.org/jira/browse/AURORA-248 Repository: aurora Description --- Use of Mock() without a specification considered harmful. I went through and updated as many mocks as I could. Any remaining can be classified as: 1) Mocks of classes that cannot be spec'd. Almost all instances of SchedulerProxy, which uses __getattr__ to delegate to the read or write client. 2) Primitives like strings and callback functions or data objects like dicts and pystachio structs. 3) Weird mocks that broke code where they really shouldn't have (off the top of my head - in test_diff.py and commands/test_run.py) - both when they were spec'd and when they were replaced with real thrift structs. The remaining offenders: $ grep -r --include=*.py Mock() src/test/python src/test/python/apache/aurora/admin/test_host_maintenance.py:mock_callback = mock.Mock() src/test/python/apache/aurora/admin/test_host_maintenance.py:mock_callback = mock.Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_proxy = Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_get = Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_task_config = Mock() src/test/python/apache/aurora/client/api/test_job_monitor.py: self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_quota_check.py: self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_scheduler_client.py: client._connect_scheduler = mock.MagicMock() src/test/python/apache/aurora/client/api/test_sla.py:self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_task_util.py:scheduler = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: job = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: job.assignedTask.task.executorConfig.data = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as (_, _, subprocess_patch, _): src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/cli/test_inspect.py:raw_config = Mock() src/test/python/apache/aurora/client/cli/test_inspect.py:mock_task = Mock() src/test/python/apache/aurora/client/cli/test_inspect.py:mock_process = Mock() src/test/python/apache/aurora/client/cli/test_kill.py:mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/test_kill.py:mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/test_kill.py:mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/test_kill.py:mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_scheduler = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_scheduler_client = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_api_factory = Mock() src/test/python/apache/aurora/client/commands/test_diff.py: job = Mock() src/test/python/apache/aurora/client/commands/test_diff.py: job.assignedTask.task.executorConfig.data = Mock() src/test/python/apache/aurora/client/commands/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/commands/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/commands/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/commands/test_listjobs.py:mock_options = Mock() src/test/python/apache/aurora/client/commands/test_listjobs.py: job = Mock() src/test/python/apache/aurora/client/commands/test_maintenance.py: mock_callback = Mock() src/test/python/apache/aurora/client/commands/test_maintenance.py: mock_wait = Mock() src/test/python/apache/aurora/client/commands/util.py:mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/commands/util.py:mock_api_factory = Mock() Diffs (updated) - src/test/python/apache/aurora/admin/test_admin_util.py f5c8c69c1109d15ee3886fb863014c3285240db1 src/test/python/apache/aurora/client/cli/test_command_hooks.py 60c75300501c36ac20a97f78ff18b3ca7af30699 src/test/python/apache/aurora/client/cli/test_cron.py c7b71c29d44150162fec8066947623fa91815424
Re: Review Request 27058: Add specs to instances of Mock in Python tests.
On Oct. 22, 2014, 11:24 p.m., Mark Chu-Carroll wrote: src/test/python/apache/aurora/client/cli/test_command_hooks.py, line 252 https://reviews.apache.org/r/27058/diff/1/?file=729290#file729290line252 I don't think this needs to be a mock at all - I'm pretty sure that you can just populate a real Response object directly. It looks like a lot of the others are like this. Kevin Sweeney wrote: +1, and since the thrift structs work off dynamic properties, spec is useless here. Calling the generated kwargs constructor gives you better coverage here, as you can't accidentally create a thrift struct with a field that doesn't exist without a TypeError Maxim Khutornenko wrote: That's not true. Spec works just fine with thrift objects. For example, adding 'spec=TaskConfig' generates an error where the same test would previously pass: AttributeError: Mock object has no attribute 'executorConfig' David McLaughlin wrote: This is the behavior I observed as well. For example, see where I had to update failure_count to failureCount because I added the spec. I'd really prefer a separate ticket for swapping out mocks for real thrift objects. Joe Smith wrote: +1 on specs for thrift structs. @dmcg: I'm good with a separate JIRA if you cut it and add the 'newbie' label. David McLaughlin wrote: https://issues.apache.org/jira/browse/AURORA-890 Kevin Sweeney wrote: @Maxim spec=TaskConfig did nothing there; spec=object does the same thing. David McLaughlin wrote: This whole conversation is redundant. You create a mock to make sure that the code under test can access the properties it needs from other objects you are not testing. If the calling code tries to access a property and there is a) a typo in your mock/spec/real object b) an old/obsolete property name on your mock/spec/real object Then the test would fail with object has no attribute 'blah'. If the calling code doesn't trigger such an error, then those attributes are not relevant to this unit test. Kevin Sweeney wrote: As a code reader using the struct type name gives me a false sense of security - there's nothing to prevent you from doing ```py config = Mock(spec=TaskConfig) config.fake_value = bogus assert config.fake_value == bogus ``` whereas: ```py config = TaskConfig(fake_value=bogus) assert config.fake_value == bogus ``` will raise a `TypeError`. David McLaughlin wrote: This code is an example of you testing the code you've written in a test. I feel this is not really relevant to the spirit of this particular ticket. Does the ticket I've filed satisfy you or do you want to block this review on this issue? Kevin Sweeney wrote: I disagree - that change is exactly what's in scope of this particular ticket. The point of spec is to prevent typos by speccing against the API. The particular mocking technique of using spec=StructName doesn't work here because the generated class doesn't have any attributes. However it is possible, by using the arg names as a `spec_set` argument: ```py TaskConfig_argnames = inspect.getargspec(TaskConfig.__init__)[0][1:] Mock(spec_set=TaskConfig_argnames) ``` Then sets to properties that aren't defined in the Thrift API will fail. If you look at the rb that was filed for this ticket you'll see the 'spirit' I was referring to. I mean we literally have specs that look like this: mock_options = Mock(spec=['bindings', 'cluster', 'env', 'json', 'open_browser', 'rename_from']) These strings are duplicated from the option names defined in argparse/our layers of wrappers. And this is where we've actually seen bugs, not thrift field renames. - David --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27058/#review57926 --- On Oct. 23, 2014, 6:14 p.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27058/ --- (Updated Oct. 23, 2014, 6:14 p.m.) Review request for Aurora, Mark Chu-Carroll and Zameer Manji. Bugs: AURORA-248 https://issues.apache.org/jira/browse/AURORA-248 Repository: aurora Description --- Use of Mock() without a specification considered harmful. I went through and updated as many mocks as I could. Any remaining can be classified as: 1) Mocks of classes that cannot be spec'd. Almost all instances of SchedulerProxy, which uses __getattr__ to delegate to the read or write client. 2) Primitives like strings and
Re: Review Request 27058: Add specs to instances of Mock in Python tests.
On Oct. 22, 2014, 11:24 p.m., Mark Chu-Carroll wrote: src/test/python/apache/aurora/client/cli/test_command_hooks.py, line 252 https://reviews.apache.org/r/27058/diff/1/?file=729290#file729290line252 I don't think this needs to be a mock at all - I'm pretty sure that you can just populate a real Response object directly. It looks like a lot of the others are like this. Kevin Sweeney wrote: +1, and since the thrift structs work off dynamic properties, spec is useless here. Calling the generated kwargs constructor gives you better coverage here, as you can't accidentally create a thrift struct with a field that doesn't exist without a TypeError Maxim Khutornenko wrote: That's not true. Spec works just fine with thrift objects. For example, adding 'spec=TaskConfig' generates an error where the same test would previously pass: AttributeError: Mock object has no attribute 'executorConfig' David McLaughlin wrote: This is the behavior I observed as well. For example, see where I had to update failure_count to failureCount because I added the spec. I'd really prefer a separate ticket for swapping out mocks for real thrift objects. Joe Smith wrote: +1 on specs for thrift structs. @dmcg: I'm good with a separate JIRA if you cut it and add the 'newbie' label. David McLaughlin wrote: https://issues.apache.org/jira/browse/AURORA-890 Kevin Sweeney wrote: @Maxim spec=TaskConfig did nothing there; spec=object does the same thing. David McLaughlin wrote: This whole conversation is redundant. You create a mock to make sure that the code under test can access the properties it needs from other objects you are not testing. If the calling code tries to access a property and there is a) a typo in your mock/spec/real object b) an old/obsolete property name on your mock/spec/real object Then the test would fail with object has no attribute 'blah'. If the calling code doesn't trigger such an error, then those attributes are not relevant to this unit test. Kevin Sweeney wrote: As a code reader using the struct type name gives me a false sense of security - there's nothing to prevent you from doing ```py config = Mock(spec=TaskConfig) config.fake_value = bogus assert config.fake_value == bogus ``` whereas: ```py config = TaskConfig(fake_value=bogus) assert config.fake_value == bogus ``` will raise a `TypeError`. David McLaughlin wrote: This code is an example of you testing the code you've written in a test. I feel this is not really relevant to the spirit of this particular ticket. Does the ticket I've filed satisfy you or do you want to block this review on this issue? Kevin Sweeney wrote: I disagree - that change is exactly what's in scope of this particular ticket. The point of spec is to prevent typos by speccing against the API. The particular mocking technique of using spec=StructName doesn't work here because the generated class doesn't have any attributes. However it is possible, by using the arg names as a `spec_set` argument: ```py TaskConfig_argnames = inspect.getargspec(TaskConfig.__init__)[0][1:] Mock(spec_set=TaskConfig_argnames) ``` Then sets to properties that aren't defined in the Thrift API will fail. David McLaughlin wrote: If you look at the rb that was filed for this ticket you'll see the 'spirit' I was referring to. I mean we literally have specs that look like this: mock_options = Mock(spec=['bindings', 'cluster', 'env', 'json', 'open_browser', 'rename_from']) These strings are duplicated from the option names defined in argparse/our layers of wrappers. And this is where we've actually seen bugs, not thrift field renames. /s/rb/JIRA description/ - David --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27058/#review57926 --- On Oct. 23, 2014, 6:14 p.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27058/ --- (Updated Oct. 23, 2014, 6:14 p.m.) Review request for Aurora, Mark Chu-Carroll and Zameer Manji. Bugs: AURORA-248 https://issues.apache.org/jira/browse/AURORA-248 Repository: aurora Description --- Use of Mock() without a specification considered harmful. I went through and updated as many mocks as I could. Any remaining can be classified as: 1) Mocks of classes that cannot be spec'd. Almost all instances of SchedulerProxy, which uses
Re: Review Request 27058: Add specs to instances of Mock in Python tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27058/ --- (Updated Oct. 23, 2014, 9:47 p.m.) Review request for Aurora, Mark Chu-Carroll and Zameer Manji. Changes --- Use spec_set instead of spec for mock options. Bugs: AURORA-248 https://issues.apache.org/jira/browse/AURORA-248 Repository: aurora Description --- Use of Mock() without a specification considered harmful. I went through and updated as many mocks as I could. Any remaining can be classified as: 1) Mocks of classes that cannot be spec'd. Almost all instances of SchedulerProxy, which uses __getattr__ to delegate to the read or write client. 2) Primitives like strings and callback functions or data objects like dicts and pystachio structs. 3) Weird mocks that broke code where they really shouldn't have (off the top of my head - in test_diff.py and commands/test_run.py) - both when they were spec'd and when they were replaced with real thrift structs. The remaining offenders: $ grep -r --include=*.py Mock() src/test/python src/test/python/apache/aurora/admin/test_host_maintenance.py:mock_callback = mock.Mock() src/test/python/apache/aurora/admin/test_host_maintenance.py:mock_callback = mock.Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_proxy = Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_get = Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_task_config = Mock() src/test/python/apache/aurora/client/api/test_job_monitor.py: self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_quota_check.py: self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_scheduler_client.py: client._connect_scheduler = mock.MagicMock() src/test/python/apache/aurora/client/api/test_sla.py:self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_task_util.py:scheduler = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: job = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: job.assignedTask.task.executorConfig.data = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as (_, _, subprocess_patch, _): src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/cli/test_inspect.py:raw_config = Mock() src/test/python/apache/aurora/client/cli/test_inspect.py:mock_task = Mock() src/test/python/apache/aurora/client/cli/test_inspect.py:mock_process = Mock() src/test/python/apache/aurora/client/cli/test_kill.py:mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/test_kill.py:mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/test_kill.py:mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/test_kill.py:mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_scheduler = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_scheduler_client = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_api_factory = Mock() src/test/python/apache/aurora/client/commands/test_diff.py: job = Mock() src/test/python/apache/aurora/client/commands/test_diff.py: job.assignedTask.task.executorConfig.data = Mock() src/test/python/apache/aurora/client/commands/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/commands/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/commands/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/commands/test_listjobs.py:mock_options = Mock() src/test/python/apache/aurora/client/commands/test_listjobs.py: job = Mock() src/test/python/apache/aurora/client/commands/test_maintenance.py: mock_callback = Mock() src/test/python/apache/aurora/client/commands/test_maintenance.py: mock_wait = Mock() src/test/python/apache/aurora/client/commands/util.py:mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/commands/util.py:mock_api_factory = Mock() Diffs (updated) - src/test/python/apache/aurora/admin/test_admin_util.py f5c8c69c1109d15ee3886fb863014c3285240db1 src/test/python/apache/aurora/client/cli/test_command_hooks.py 9fc6fe2c2063cda494437d83044557b345acacea src/test/python/apache/aurora/client/cli/test_cron.py c7b71c29d44150162fec8066947623fa91815424
Re: Review Request 27058: Add specs to instances of Mock in Python tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27058/#review58120 --- Ship it! Ship It! - Kevin Sweeney On Oct. 23, 2014, 2:47 p.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27058/ --- (Updated Oct. 23, 2014, 2:47 p.m.) Review request for Aurora, Mark Chu-Carroll and Zameer Manji. Bugs: AURORA-248 https://issues.apache.org/jira/browse/AURORA-248 Repository: aurora Description --- Use of Mock() without a specification considered harmful. I went through and updated as many mocks as I could. Any remaining can be classified as: 1) Mocks of classes that cannot be spec'd. Almost all instances of SchedulerProxy, which uses __getattr__ to delegate to the read or write client. 2) Primitives like strings and callback functions or data objects like dicts and pystachio structs. 3) Weird mocks that broke code where they really shouldn't have (off the top of my head - in test_diff.py and commands/test_run.py) - both when they were spec'd and when they were replaced with real thrift structs. The remaining offenders: $ grep -r --include=*.py Mock() src/test/python src/test/python/apache/aurora/admin/test_host_maintenance.py: mock_callback = mock.Mock() src/test/python/apache/aurora/admin/test_host_maintenance.py: mock_callback = mock.Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_proxy = Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_get = Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_task_config = Mock() src/test/python/apache/aurora/client/api/test_job_monitor.py: self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_quota_check.py: self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_scheduler_client.py: client._connect_scheduler = mock.MagicMock() src/test/python/apache/aurora/client/api/test_sla.py:self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_task_util.py:scheduler = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: job = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: job.assignedTask.task.executorConfig.data = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as (_, _, subprocess_patch, _): src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/cli/test_inspect.py:raw_config = Mock() src/test/python/apache/aurora/client/cli/test_inspect.py:mock_task = Mock() src/test/python/apache/aurora/client/cli/test_inspect.py:mock_process = Mock() src/test/python/apache/aurora/client/cli/test_kill.py: mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/test_kill.py: mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/test_kill.py: mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/test_kill.py: mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_scheduler = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_scheduler_client = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_api_factory = Mock() src/test/python/apache/aurora/client/commands/test_diff.py: job = Mock() src/test/python/apache/aurora/client/commands/test_diff.py: job.assignedTask.task.executorConfig.data = Mock() src/test/python/apache/aurora/client/commands/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/commands/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/commands/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/commands/test_listjobs.py: mock_options = Mock() src/test/python/apache/aurora/client/commands/test_listjobs.py: job = Mock() src/test/python/apache/aurora/client/commands/test_maintenance.py: mock_callback = Mock() src/test/python/apache/aurora/client/commands/test_maintenance.py: mock_wait = Mock() src/test/python/apache/aurora/client/commands/util.py: mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/commands/util.py:mock_api_factory = Mock()
Re: Review Request 27058: Add specs to instances of Mock in Python tests.
On Oct. 22, 2014, 11:24 p.m., Mark Chu-Carroll wrote: src/test/python/apache/aurora/client/cli/test_command_hooks.py, line 252 https://reviews.apache.org/r/27058/diff/1/?file=729290#file729290line252 I don't think this needs to be a mock at all - I'm pretty sure that you can just populate a real Response object directly. It looks like a lot of the others are like this. Kevin Sweeney wrote: +1, and since the thrift structs work off dynamic properties, spec is useless here. Calling the generated kwargs constructor gives you better coverage here, as you can't accidentally create a thrift struct with a field that doesn't exist without a TypeError Maxim Khutornenko wrote: That's not true. Spec works just fine with thrift objects. For example, adding 'spec=TaskConfig' generates an error where the same test would previously pass: AttributeError: Mock object has no attribute 'executorConfig' David McLaughlin wrote: This is the behavior I observed as well. For example, see where I had to update failure_count to failureCount because I added the spec. I'd really prefer a separate ticket for swapping out mocks for real thrift objects. Joe Smith wrote: +1 on specs for thrift structs. @dmcg: I'm good with a separate JIRA if you cut it and add the 'newbie' label. David McLaughlin wrote: https://issues.apache.org/jira/browse/AURORA-890 Kevin Sweeney wrote: @Maxim spec=TaskConfig did nothing there; spec=object does the same thing. David McLaughlin wrote: This whole conversation is redundant. You create a mock to make sure that the code under test can access the properties it needs from other objects you are not testing. If the calling code tries to access a property and there is a) a typo in your mock/spec/real object b) an old/obsolete property name on your mock/spec/real object Then the test would fail with object has no attribute 'blah'. If the calling code doesn't trigger such an error, then those attributes are not relevant to this unit test. Kevin Sweeney wrote: As a code reader using the struct type name gives me a false sense of security - there's nothing to prevent you from doing ```py config = Mock(spec=TaskConfig) config.fake_value = bogus assert config.fake_value == bogus ``` whereas: ```py config = TaskConfig(fake_value=bogus) assert config.fake_value == bogus ``` will raise a `TypeError`. David McLaughlin wrote: This code is an example of you testing the code you've written in a test. I feel this is not really relevant to the spirit of this particular ticket. Does the ticket I've filed satisfy you or do you want to block this review on this issue? Kevin Sweeney wrote: I disagree - that change is exactly what's in scope of this particular ticket. The point of spec is to prevent typos by speccing against the API. The particular mocking technique of using spec=StructName doesn't work here because the generated class doesn't have any attributes. However it is possible, by using the arg names as a `spec_set` argument: ```py TaskConfig_argnames = inspect.getargspec(TaskConfig.__init__)[0][1:] Mock(spec_set=TaskConfig_argnames) ``` Then sets to properties that aren't defined in the Thrift API will fail. David McLaughlin wrote: If you look at the rb that was filed for this ticket you'll see the 'spirit' I was referring to. I mean we literally have specs that look like this: mock_options = Mock(spec=['bindings', 'cluster', 'env', 'json', 'open_browser', 'rename_from']) These strings are duplicated from the option names defined in argparse/our layers of wrappers. And this is where we've actually seen bugs, not thrift field renames. David McLaughlin wrote: /s/rb/JIRA description/ I filed another ticket to try and solve the problem above: https://issues.apache.org/jira/browse/AURORA-892 In the meantime I've switched to spec_set as spec alone was a no-op the way we were using it. - David --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27058/#review57926 --- On Oct. 23, 2014, 9:47 p.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27058/ --- (Updated Oct. 23, 2014, 9:47 p.m.) Review request for Aurora, Mark Chu-Carroll and Zameer Manji. Bugs: AURORA-248 https://issues.apache.org/jira/browse/AURORA-248 Repository: aurora Description --- Use of Mock()
Re: Review Request 27058: Add specs to instances of Mock in Python tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27058/#review58205 --- This patch does not apply cleanly on master (53f4e73), do you need to rebase? - Aurora ReviewBot On Oct. 23, 2014, 9:47 p.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27058/ --- (Updated Oct. 23, 2014, 9:47 p.m.) Review request for Aurora, Mark Chu-Carroll and Zameer Manji. Bugs: AURORA-248 https://issues.apache.org/jira/browse/AURORA-248 Repository: aurora Description --- Use of Mock() without a specification considered harmful. I went through and updated as many mocks as I could. Any remaining can be classified as: 1) Mocks of classes that cannot be spec'd. Almost all instances of SchedulerProxy, which uses __getattr__ to delegate to the read or write client. 2) Primitives like strings and callback functions or data objects like dicts and pystachio structs. 3) Weird mocks that broke code where they really shouldn't have (off the top of my head - in test_diff.py and commands/test_run.py) - both when they were spec'd and when they were replaced with real thrift structs. The remaining offenders: $ grep -r --include=*.py Mock() src/test/python src/test/python/apache/aurora/admin/test_host_maintenance.py: mock_callback = mock.Mock() src/test/python/apache/aurora/admin/test_host_maintenance.py: mock_callback = mock.Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_proxy = Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_get = Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_task_config = Mock() src/test/python/apache/aurora/client/api/test_job_monitor.py: self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_quota_check.py: self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_scheduler_client.py: client._connect_scheduler = mock.MagicMock() src/test/python/apache/aurora/client/api/test_sla.py:self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_task_util.py:scheduler = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: job = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: job.assignedTask.task.executorConfig.data = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as (_, _, subprocess_patch, _): src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/cli/test_inspect.py:raw_config = Mock() src/test/python/apache/aurora/client/cli/test_inspect.py:mock_task = Mock() src/test/python/apache/aurora/client/cli/test_inspect.py:mock_process = Mock() src/test/python/apache/aurora/client/cli/test_kill.py: mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/test_kill.py: mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/test_kill.py: mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/test_kill.py: mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_scheduler = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_scheduler_client = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_api_factory = Mock() src/test/python/apache/aurora/client/commands/test_diff.py: job = Mock() src/test/python/apache/aurora/client/commands/test_diff.py: job.assignedTask.task.executorConfig.data = Mock() src/test/python/apache/aurora/client/commands/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/commands/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/commands/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/commands/test_listjobs.py: mock_options = Mock() src/test/python/apache/aurora/client/commands/test_listjobs.py: job = Mock() src/test/python/apache/aurora/client/commands/test_maintenance.py: mock_callback = Mock() src/test/python/apache/aurora/client/commands/test_maintenance.py: mock_wait = Mock() src/test/python/apache/aurora/client/commands/util.py: mock_scheduler_proxy = Mock()
Re: Review Request 27058: Add specs to instances of Mock in Python tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27058/#review57936 --- src/test/python/apache/aurora/client/cli/test_diff.py https://reviews.apache.org/r/27058/#comment98846 Feel free to drop this TODO as it is fixed in https://reviews.apache.org/r/26954. The caveat: setup_populate_job_config(). It uses the result of create_mock_scheduled_tasks() (list of ScheduledTask) to populate a set of TaskConfigs. src/test/python/apache/aurora/client/commands/test_kill.py https://reviews.apache.org/r/27058/#comment98850 4 spaces - Maxim Khutornenko On Oct. 22, 2014, 11:18 p.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27058/ --- (Updated Oct. 22, 2014, 11:18 p.m.) Review request for Aurora, Mark Chu-Carroll and Zameer Manji. Bugs: AURORA-248 https://issues.apache.org/jira/browse/AURORA-248 Repository: aurora Description --- Use of Mock() without a specification considered harmful. I went through and updated as many mocks as I could. Any remaining can be classified as: 1) Mocks of classes that cannot be spec'd. Almost all instances of SchedulerProxy, which uses __getattr__ to delegate to the read or write client. 2) Primitives like strings and callback functions or data objects like dicts and pystachio structs. 3) Weird mocks that broke code where they really shouldn't have (off the top of my head - in test_diff.py and commands/test_run.py) - both when they were spec'd and when they were replaced with real thrift structs. The remaining offenders: $ grep -r --include=*.py Mock() src/test/python src/test/python/apache/aurora/admin/test_host_maintenance.py: mock_callback = mock.Mock() src/test/python/apache/aurora/admin/test_host_maintenance.py: mock_callback = mock.Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_proxy = Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_get = Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_task_config = Mock() src/test/python/apache/aurora/client/api/test_job_monitor.py: self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_quota_check.py: self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_scheduler_client.py: client._connect_scheduler = mock.MagicMock() src/test/python/apache/aurora/client/api/test_sla.py:self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_task_util.py:scheduler = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: job = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: job.assignedTask.task.executorConfig.data = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as (_, _, subprocess_patch, _): src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/cli/test_inspect.py:raw_config = Mock() src/test/python/apache/aurora/client/cli/test_inspect.py:mock_task = Mock() src/test/python/apache/aurora/client/cli/test_inspect.py:mock_process = Mock() src/test/python/apache/aurora/client/cli/test_kill.py: mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/test_kill.py: mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/test_kill.py: mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/test_kill.py: mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_scheduler = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_scheduler_client = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_api_factory = Mock() src/test/python/apache/aurora/client/commands/test_diff.py: job = Mock() src/test/python/apache/aurora/client/commands/test_diff.py: job.assignedTask.task.executorConfig.data = Mock() src/test/python/apache/aurora/client/commands/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/commands/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/commands/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/commands/test_listjobs.py: mock_options =
Re: Review Request 27058: Add specs to instances of Mock in Python tests.
On Oct. 22, 2014, 11:24 p.m., Mark Chu-Carroll wrote: src/test/python/apache/aurora/client/cli/test_command_hooks.py, line 252 https://reviews.apache.org/r/27058/diff/1/?file=729290#file729290line252 I don't think this needs to be a mock at all - I'm pretty sure that you can just populate a real Response object directly. It looks like a lot of the others are like this. Kevin Sweeney wrote: +1, and since the thrift structs work off dynamic properties, spec is useless here. Calling the generated kwargs constructor gives you better coverage here, as you can't accidentally create a thrift struct with a field that doesn't exist without a TypeError That's not true. Spec works just fine with thrift objects. For example, adding 'spec=TaskConfig' generates an error where the same test would previously pass: AttributeError: Mock object has no attribute 'executorConfig' - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27058/#review57926 --- On Oct. 22, 2014, 11:18 p.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27058/ --- (Updated Oct. 22, 2014, 11:18 p.m.) Review request for Aurora, Mark Chu-Carroll and Zameer Manji. Bugs: AURORA-248 https://issues.apache.org/jira/browse/AURORA-248 Repository: aurora Description --- Use of Mock() without a specification considered harmful. I went through and updated as many mocks as I could. Any remaining can be classified as: 1) Mocks of classes that cannot be spec'd. Almost all instances of SchedulerProxy, which uses __getattr__ to delegate to the read or write client. 2) Primitives like strings and callback functions or data objects like dicts and pystachio structs. 3) Weird mocks that broke code where they really shouldn't have (off the top of my head - in test_diff.py and commands/test_run.py) - both when they were spec'd and when they were replaced with real thrift structs. The remaining offenders: $ grep -r --include=*.py Mock() src/test/python src/test/python/apache/aurora/admin/test_host_maintenance.py: mock_callback = mock.Mock() src/test/python/apache/aurora/admin/test_host_maintenance.py: mock_callback = mock.Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_proxy = Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_get = Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_task_config = Mock() src/test/python/apache/aurora/client/api/test_job_monitor.py: self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_quota_check.py: self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_scheduler_client.py: client._connect_scheduler = mock.MagicMock() src/test/python/apache/aurora/client/api/test_sla.py:self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_task_util.py:scheduler = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: job = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: job.assignedTask.task.executorConfig.data = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as (_, _, subprocess_patch, _): src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/cli/test_inspect.py:raw_config = Mock() src/test/python/apache/aurora/client/cli/test_inspect.py:mock_task = Mock() src/test/python/apache/aurora/client/cli/test_inspect.py:mock_process = Mock() src/test/python/apache/aurora/client/cli/test_kill.py: mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/test_kill.py: mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/test_kill.py: mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/test_kill.py: mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_scheduler = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_scheduler_client = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_api_factory = Mock() src/test/python/apache/aurora/client/commands/test_diff.py: job = Mock() src/test/python/apache/aurora/client/commands/test_diff.py: job.assignedTask.task.executorConfig.data = Mock()
Re: Review Request 27058: Add specs to instances of Mock in Python tests.
On Oct. 22, 2014, 4:24 p.m., Mark Chu-Carroll wrote: src/test/python/apache/aurora/client/cli/test_command_hooks.py, line 252 https://reviews.apache.org/r/27058/diff/1/?file=729290#file729290line252 I don't think this needs to be a mock at all - I'm pretty sure that you can just populate a real Response object directly. It looks like a lot of the others are like this. Kevin Sweeney wrote: +1, and since the thrift structs work off dynamic properties, spec is useless here. Calling the generated kwargs constructor gives you better coverage here, as you can't accidentally create a thrift struct with a field that doesn't exist without a TypeError Maxim Khutornenko wrote: That's not true. Spec works just fine with thrift objects. For example, adding 'spec=TaskConfig' generates an error where the same test would previously pass: AttributeError: Mock object has no attribute 'executorConfig' David McLaughlin wrote: This is the behavior I observed as well. For example, see where I had to update failure_count to failureCount because I added the spec. I'd really prefer a separate ticket for swapping out mocks for real thrift objects. +1 on specs for thrift structs. @dmcg: I'm good with a separate JIRA if you cut it and add the 'newbie' label. - Joe --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27058/#review57926 --- On Oct. 22, 2014, 4:18 p.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27058/ --- (Updated Oct. 22, 2014, 4:18 p.m.) Review request for Aurora, Mark Chu-Carroll and Zameer Manji. Bugs: AURORA-248 https://issues.apache.org/jira/browse/AURORA-248 Repository: aurora Description --- Use of Mock() without a specification considered harmful. I went through and updated as many mocks as I could. Any remaining can be classified as: 1) Mocks of classes that cannot be spec'd. Almost all instances of SchedulerProxy, which uses __getattr__ to delegate to the read or write client. 2) Primitives like strings and callback functions or data objects like dicts and pystachio structs. 3) Weird mocks that broke code where they really shouldn't have (off the top of my head - in test_diff.py and commands/test_run.py) - both when they were spec'd and when they were replaced with real thrift structs. The remaining offenders: $ grep -r --include=*.py Mock() src/test/python src/test/python/apache/aurora/admin/test_host_maintenance.py: mock_callback = mock.Mock() src/test/python/apache/aurora/admin/test_host_maintenance.py: mock_callback = mock.Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_proxy = Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_get = Mock() src/test/python/apache/aurora/client/api/test_api.py:mock_task_config = Mock() src/test/python/apache/aurora/client/api/test_job_monitor.py: self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_quota_check.py: self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_scheduler_client.py: client._connect_scheduler = mock.MagicMock() src/test/python/apache/aurora/client/api/test_sla.py:self._scheduler = Mock() src/test/python/apache/aurora/client/api/test_task_util.py:scheduler = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: job = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: job.assignedTask.task.executorConfig.data = Mock() src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as (_, _, subprocess_patch, _): src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/cli/test_diff.py: patch('json.loads', return_value=Mock())) as ( src/test/python/apache/aurora/client/cli/test_inspect.py:raw_config = Mock() src/test/python/apache/aurora/client/cli/test_inspect.py:mock_task = Mock() src/test/python/apache/aurora/client/cli/test_inspect.py:mock_process = Mock() src/test/python/apache/aurora/client/cli/test_kill.py: mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/test_kill.py: mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/test_kill.py: mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/test_kill.py: mock_scheduler_proxy = Mock() src/test/python/apache/aurora/client/cli/util.py:mock_scheduler_proxy = Mock()