> 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 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
>
>
