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 21611177B9 for ; Thu, 23 Oct 2014 00:21:03 +0000 (UTC) Received: (qmail 34045 invoked by uid 500); 23 Oct 2014 00:21:03 -0000 Delivered-To: apmail-aurora-reviews-archive@aurora.apache.org Received: (qmail 33999 invoked by uid 500); 23 Oct 2014 00:21:03 -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 33988 invoked by uid 99); 23 Oct 2014 00:21:02 -0000 Received: from nike.apache.org (HELO nike.apache.org) (192.87.106.230) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 23 Oct 2014 00:21:02 +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; Thu, 23 Oct 2014 00:20:38 +0000 Received: (qmail 33787 invoked by uid 99); 23 Oct 2014 00:20:35 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 23 Oct 2014 00:20:35 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id 788CC1DF5A5; Thu, 23 Oct 2014 00:20:40 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============6520680861945930860==" MIME-Version: 1.0 Subject: Re: Review Request 27058: Add specs to instances of Mock in Python tests. From: "Maxim Khutornenko" To: "Mark Chu-Carroll" , "Zameer Manji" Cc: "Aurora" , "Kevin Sweeney" , "Maxim Khutornenko" , "David McLaughlin" Date: Thu, 23 Oct 2014 00:20:40 -0000 Message-ID: <20141023002040.1282.16684@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org Auto-Submitted: auto-generated Sender: "Maxim Khutornenko" X-ReviewGroup: Aurora X-ReviewRequest-URL: https://reviews.apache.org/r/27058/ X-Sender: "Maxim Khutornenko" References: <20141022232432.1283.67997@reviews.apache.org> In-Reply-To: <20141022232432.1283.67997@reviews.apache.org> Reply-To: "Maxim Khutornenko" X-ReviewRequest-Repository: aurora X-Virus-Checked: Checked by ClamAV on apache.org --===============6520680861945930860== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit > 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 > > > > > > 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() > 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 > ----- > > 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 > src/test/python/apache/aurora/client/cli/test_diff.py 10817695352687cdb5b0c3ed9720e3091b230e68 > src/test/python/apache/aurora/client/cli/test_sla.py a1a3d8161ba747aa23a5e614e9ae31473d2058c1 > src/test/python/apache/aurora/client/cli/test_status.py 4f62cf0c52e5837309cf7ad702df6d907df8f510 > src/test/python/apache/aurora/client/cli/test_task_run.py 16fde14c03f6fd2c000e76625fad174835763f1b > src/test/python/apache/aurora/client/cli/util.py 3fa609a5f71525393ca0a5dbd81423005fadb583 > 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 c8d01456aa52fd61374b4f0960b5159da2cb235b > 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 abb657ba397c23ddac6c6b188f70d1c4e34597a6 > src/test/python/apache/aurora/client/commands/test_status.py 639763501348a35bff2f127e18780ac74852f51b > src/test/python/apache/aurora/client/commands/test_update.py 07cbe53109e8bcdd09dcac47f6353b10e095717d > > 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 > > --===============6520680861945930860==--