> On Oct. 22, 2014, 11:51 p.m., Bill Farner wrote: > > src/test/python/apache/aurora/client/cli/test_diff.py, line 60 > > <https://reviews.apache.org/r/26954/diff/3/?file=729386#file729386line60> > > > > While you're here, please change this to not use Mock, all down the > > hierarchy. > > > > Ditto for other cargo cults (thanks for collapsing a few, btw). > > Maxim Khutornenko wrote: > I collapsed all related cli methods into util.py and using specs for all > Mocks now. Did not modify the commands (v1) tests to let them die out > naturally. > > I actually like mocks here as tests fail when accessing a field that is > not populated. > > Bill Farner wrote: > Please chime in on https://reviews.apache.org/r/27058/, where the exact > opposite perspective exists. > > Maxim Khutornenko wrote: > Commented there. Having a Mock with spec here really helps identifying > the usage. > > Kevin Sweeney wrote: > See my comments there - tl;dr `Mock(spec=ThriftStruct)` should be > considered harmful as it's no better than `Mock(spec=object)`. > > Either calling the kwargs constructor or using `spec_set` with the > constructor kwargs retrieved via reflection gets us the correct behavior > here. Since this seems to be a source of confusion for several people we're > probably well-served factoring out a thrift_test_util library or moving to > kwargs everywhere.
Agree, spec is not deterministic enough in this case. Refactored. - Maxim ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26954/#review57933 ----------------------------------------------------------- On Oct. 22, 2014, 11:59 p.m., Maxim Khutornenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26954/ > ----------------------------------------------------------- > > (Updated Oct. 22, 2014, 11:59 p.m.) > > > Review request for Aurora, Kevin Sweeney and Bill Farner. > > > Bugs: AURORA-84 > https://issues.apache.org/jira/browse/AURORA-84 > > > Repository: aurora > > > Description > ------- > > Python side of changes for deprecating identity struct. Accounting for the > upcoming deprecation by backfilling missing fields and switching between > old/new fields (whichever is available). > > Branched off of https://reviews.apache.org/r/26762/ and will have to be > pushed together. > > Summary of changes: > - TaskConfig - backfilling _key_ field in job diff and update commands. > > Also, some unit test refactoring to avoid copy-pasted fragments. > > > Diffs > ----- > > src/main/python/apache/aurora/client/api/instance_watcher.py > b390aa8993205f1f6938f8c295e3c16a0bf4df6d > src/main/python/apache/aurora/client/api/sla.py > b9b64680b15f5395ed6aca681b9b1c30ffe2822c > src/main/python/apache/aurora/client/cli/task.py > c41484bdc27266443bc4e139e1ebb362a59be0f9 > src/main/python/apache/aurora/client/commands/admin.py > deee0250f3ba9837feeb92acc654f5b3b68b4e0f > src/main/python/apache/aurora/client/commands/core.py > 58f419e674f1a9a0ae9da6faa2e39c8167bab597 > src/main/python/apache/aurora/client/commands/ssh.py > d2b8bf675556b924d3d63b545d036dc48a081486 > src/main/python/apache/aurora/config/thrift.py > 9ca806db6d2c59836196ea2fdd3fc015b9f6b71f > src/main/python/apache/aurora/executor/aurora_executor.py > 2c6423d096656f426a4385f4edef6875ebad7049 > src/main/python/apache/aurora/executor/common/announcer.py > 74b2114d1ede7a4a4a68b78144f9b08bc994bd87 > src/main/python/apache/aurora/executor/thermos_task_runner.py > bb99bd12d7387e080d8d4c0557d2afa3bf5e7c60 > src/test/python/apache/aurora/client/api/test_instance_watcher.py > ae1b24bf4e3291cb31b3129cabcacdf32db0c560 > src/test/python/apache/aurora/client/api/test_sla.py > 1117f24d5ad3640632a1dd728913ba73c8bec707 > src/test/python/apache/aurora/client/cli/test_api_from_cli.py > 95aa649cfff9166dd10aa432c4d470739e8f06c5 > src/test/python/apache/aurora/client/cli/test_diff.py > 10817695352687cdb5b0c3ed9720e3091b230e68 > 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/test_update.py > 1ec5483506a22a774340acccd33f09f1742be8b7 > src/test/python/apache/aurora/client/cli/util.py > 3fa609a5f71525393ca0a5dbd81423005fadb583 > src/test/python/apache/aurora/client/commands/test_diff.py > c8d01456aa52fd61374b4f0960b5159da2cb235b > 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 > src/test/python/apache/aurora/config/test_thrift.py > 1dd9e79099793aa7bb377c20c2a6a0c4b750be33 > src/test/python/apache/aurora/executor/common/test_announcer.py > 56943351ca09c29580dd764bb2442f0fcd9fde74 > src/test/python/apache/aurora/executor/test_thermos_executor.py > 65e8cce60a5543081175c574aaaf92f200bc6233 > > Diff: https://reviews.apache.org/r/26954/diff/ > > > Testing > ------- > > ./pants src/test/python:all > test_end_to_end.sh > > > Thanks, > > Maxim Khutornenko > >