aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "David McLaughlin" <da...@dmclaughlin.com>
Subject Re: Review Request 27058: Add specs to instances of Mock in Python tests.
Date Thu, 23 Oct 2014 20:30:37 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message