Return-Path: X-Original-To: apmail-aurora-reviews-archive@minotaur.apache.org Delivered-To: apmail-aurora-reviews-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 7FA6F17437 for ; Fri, 24 Oct 2014 20:10:59 +0000 (UTC) Received: (qmail 46334 invoked by uid 500); 24 Oct 2014 20:10:59 -0000 Delivered-To: apmail-aurora-reviews-archive@aurora.apache.org Received: (qmail 46290 invoked by uid 500); 24 Oct 2014 20:10:59 -0000 Mailing-List: contact reviews-help@aurora.incubator.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: reviews@aurora.incubator.apache.org Delivered-To: mailing list reviews@aurora.incubator.apache.org Received: (qmail 46275 invoked by uid 99); 24 Oct 2014 20:10:59 -0000 Received: from nike.apache.org (HELO nike.apache.org) (192.87.106.230) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 24 Oct 2014 20:10:59 +0000 X-ASF-Spam-Status: No, hits=-1999.2 required=5.0 tests=ALL_TRUSTED,HTML_MESSAGE,RP_MATCHES_RCVD X-Spam-Check-By: apache.org Received: from [140.211.11.3] (HELO mail.apache.org) (140.211.11.3) by apache.org (qpsmtpd/0.29) with SMTP; Fri, 24 Oct 2014 20:10:34 +0000 Received: (qmail 42518 invoked by uid 99); 24 Oct 2014 20:09:17 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 24 Oct 2014 20:09:17 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id EF7701DF680; Fri, 24 Oct 2014 20:09:21 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============5220468487764507773==" MIME-Version: 1.0 Subject: Re: Review Request 27058: Add specs to instances of Mock in Python tests. From: "David McLaughlin" To: "Mark Chu-Carroll" , "Zameer Manji" Cc: "Joe Smith" , "Aurora" , "Kevin Sweeney" , "Aurora ReviewBot" , "Maxim Khutornenko" , "David McLaughlin" Date: Fri, 24 Oct 2014 20:09:21 -0000 Message-ID: <20141024200921.20835.3859@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org Auto-Submitted: auto-generated Sender: "David McLaughlin" X-ReviewGroup: Aurora X-ReviewRequest-URL: https://reviews.apache.org/r/27058/ X-Sender: "David McLaughlin" References: <20141023214726.1283.2104@reviews.apache.org> In-Reply-To: <20141023214726.1283.2104@reviews.apache.org> Reply-To: "David McLaughlin" X-ReviewRequest-Repository: aurora X-Virus-Checked: Checked by ClamAV on apache.org --===============5220468487764507773== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit ----------------------------------------------------------- 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 78694d7559f2041f27cd2a7e4cb81ca467f63ac2 src/test/python/apache/aurora/client/cli/test_sla.py a1a3d8161ba747aa23a5e614e9ae31473d2058c1 src/test/python/apache/aurora/client/cli/test_task_run.py 12163df0d2e1e42f2a321603ec10ff9359848216 src/test/python/apache/aurora/client/cli/util.py 796c4f9880a0f834a6950472892981e8a6789a97 src/test/python/apache/aurora/client/commands/test_cancel_update.py 13aa1fef1d94d46f2837f500606028baa694fa6e src/test/python/apache/aurora/client/commands/test_create.py 4a753fb5942555854538047eb947e5465cdff607 src/test/python/apache/aurora/client/commands/test_diff.py 9f1d459e51c663b9ad62bbbbb16a8127568662d1 src/test/python/apache/aurora/client/commands/test_hooks.py d4d8d3cd15704353d958e1ef6b220eaa37696a4d src/test/python/apache/aurora/client/commands/test_kill.py 1e13b926379147295a3a1b3d6ce79a727719dedb src/test/python/apache/aurora/client/commands/test_maintenance.py 13d753f6870c9f552903b077e3c38d306ead5bc4 src/test/python/apache/aurora/client/commands/test_restart.py efa0849c1f11d9304e2da981dfb9c1d0ff59a15d src/test/python/apache/aurora/client/commands/test_run.py 0c395f7a8106acf3d45842a6f536dfb74b71a309 src/test/python/apache/aurora/client/commands/test_ssh.py cf9f425b3dd64afe9d8fcfd70495a3c58108824f src/test/python/apache/aurora/client/commands/test_status.py 9eb8def26692cf5fbd0c20bc96975125e411f0ba src/test/python/apache/aurora/client/commands/test_update.py 555ea0d2727fca61256faf7815945320fcbde55d Diff: https://reviews.apache.org/r/27058/diff/ Testing ------- $ ./pants src/test/python/apache/aurora/:all $ build-support/python/checkstyle-check src/test/ $ build-support/python/isort-check Thanks, David McLaughlin --===============5220468487764507773==--