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