Re: Review Request 31710: Fix query and timestamp display when running beta-update status.
On March 3, 2015, 3:21 p.m., Zameer Manji wrote:
src/test/python/apache/aurora/client/cli/test_supdate.py, line 211
https://reviews.apache.org/r/31710/diff/1/?file=883801#file883801line211
For the calls like this you can do
`mock_api.pause_job_update.assert_called_once_with(self.TEST_JOBKEY)`
instead of comparing against an array of call objects.
Bill Farner wrote:
In case the diff didn't make it clear, i think we should settle on
`mock_calls` for uniformity. Additionally, i think the error messages from
asserting on `mock_calls` is better:
```
_mock_self = MagicMock name='ctime' id='140532173693328', args = (1,)
kwargs = {}, self = MagicMock name='ctime' id='140532173693328'
msg = 'Expected to be called once. Called 9 times.'
???
E AssertionError: Expected to be called once. Called 9 times.
```
vs
```
assert self.mock_ctime.mock_calls == [call(1)]
E AssertionError: assert [call(1),\n ca...(8),\n call(9)] ==
[call(1)]
E Left contains more items, first extra item: call(2)
E Full diff:
E - [call(1),
E ? ^
E + [call(1)]
E ? ^
E - call(2),
E - call(3),
E - call(4),
E - call(5),
E - call(6),
E - call(7),
E - call(8),
E - call(9)]
```
Moreover, `assert x.mock_calls == [...]` is not vulnerable to typos. In
this test, the first typoed line passes, but the second fails.
```
self.mock_ctime.assret_called_once_with(1)
assert self.mock_ctime.mock_clls == [call(n) for n in range(1, 10)]
```
Your reasoning is sound. I would like to point out the nice error messages come
from using `assert` statements under a py.test runner.
- Zameer
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31710/#review75083
---
On March 3, 2015, 3:16 p.m., Bill Farner wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31710/
---
(Updated March 3, 2015, 3:16 p.m.)
Review request for Aurora and Maxim Khutornenko.
Bugs: AURORA-1165 and AURORA-1166
https://issues.apache.org/jira/browse/AURORA-1165
https://issues.apache.org/jira/browse/AURORA-1166
Repository: aurora
Description
---
Discovered these while working on AURORA-1157
Diffs
-
src/main/python/apache/aurora/client/cli/update.py
ee8146be0cb6e3f3139a5491c883864950f245bd
src/test/python/apache/aurora/client/cli/test_supdate.py
1173650998d397a68a7fabf38201fb9feebf2977
Diff: https://reviews.apache.org/r/31710/diff/
Testing
---
Thanks,
Bill Farner
Re: Review Request 31710: Fix query and timestamp display when running beta-update status.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31710/#review75100 --- Ship it! Master (c0b3701) is green with this patch. ./build-support/jenkins/build.sh I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On March 3, 2015, 11:44 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31710/ --- (Updated March 3, 2015, 11:44 p.m.) Review request for Aurora and Maxim Khutornenko. Bugs: AURORA-1165 and AURORA-1166 https://issues.apache.org/jira/browse/AURORA-1165 https://issues.apache.org/jira/browse/AURORA-1166 Repository: aurora Description --- Discovered these while working on AURORA-1157 Diffs - src/main/python/apache/aurora/client/cli/update.py ee8146be0cb6e3f3139a5491c883864950f245bd src/test/python/apache/aurora/client/cli/test_supdate.py 1173650998d397a68a7fabf38201fb9feebf2977 Diff: https://reviews.apache.org/r/31710/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 31710: Fix query and timestamp display when running beta-update status.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31710/#review75087 --- Master (c0b3701) is red with this patch. ./build-support/jenkins/build.sh SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/client/cli/test_status.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/client/cli/test_plugins.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/client/cli/test_update.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/client/cli/test_task.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/client/cli/util.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/client/cli/test_create.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/client/api/test_task_util.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/client/api/test_job_monitor.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/client/api/test_restarter.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/client/api/test_health_check.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/client/api/test_instance_watcher.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/client/api/test_scheduler_mux.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/client/api/test_sla.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/client/api/test_api.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/client/api/test_updater_util.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/client/api/__init__.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/client/api/test_updater.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/client/api/test_quota_check.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/client/api/test_scheduler_client.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/client/hooks/test_hooked_api.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/client/hooks/test_non_hooked_api.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/thermos/core/test_staged_kill.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/thermos/core/test_helper.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/thermos/core/test_angry.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/thermos/core/test_process.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/thermos/core/test_runner_integration.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/thermos/core/test_finalization.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/thermos/core/test_failure_limit.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/thermos/core/test_ephemerals.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/thermos/core/test_failing_runner.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/thermos/common/test_pathspec.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/thermos/common/test_planner.py Everything Looks Good! SUCCESS: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/thermos/common/test_task_planner.py Everything Looks Good! SUCCESS:
Re: Review Request 31710: Fix query and timestamp display when running beta-update status.
On March 3, 2015, 11:48 p.m., Maxim Khutornenko wrote: src/main/python/apache/aurora/client/cli/update.py, line 302 https://reviews.apache.org/r/31710/diff/1/?file=883800#file883800line302 You may want to extract an internal method to do this, e.g.: ``` def timestamp(time_msec): return time.ctime(time_msec / 1000) ``` Done. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31710/#review75089 --- On March 3, 2015, 11:44 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31710/ --- (Updated March 3, 2015, 11:44 p.m.) Review request for Aurora and Maxim Khutornenko. Bugs: AURORA-1165 and AURORA-1166 https://issues.apache.org/jira/browse/AURORA-1165 https://issues.apache.org/jira/browse/AURORA-1166 Repository: aurora Description --- Discovered these while working on AURORA-1157 Diffs - src/main/python/apache/aurora/client/cli/update.py ee8146be0cb6e3f3139a5491c883864950f245bd src/test/python/apache/aurora/client/cli/test_supdate.py 1173650998d397a68a7fabf38201fb9feebf2977 Diff: https://reviews.apache.org/r/31710/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 31710: Fix query and timestamp display when running beta-update status.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31710/ --- (Updated March 4, 2015, 12:19 a.m.) Review request for Aurora and Maxim Khutornenko. Bugs: AURORA-1165 and AURORA-1166 https://issues.apache.org/jira/browse/AURORA-1165 https://issues.apache.org/jira/browse/AURORA-1166 Repository: aurora Description --- Discovered these while working on AURORA-1157 Diffs (updated) - src/main/python/apache/aurora/client/cli/update.py ee8146be0cb6e3f3139a5491c883864950f245bd src/test/python/apache/aurora/client/cli/test_supdate.py 1173650998d397a68a7fabf38201fb9feebf2977 Diff: https://reviews.apache.org/r/31710/diff/ Testing --- Thanks, Bill Farner
Review Request 31710: Fix query and timestamp display when running beta-update status.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31710/ --- Review request for Aurora and Maxim Khutornenko. Bugs: AURORA-1165 and AURORA-1166 https://issues.apache.org/jira/browse/AURORA-1165 https://issues.apache.org/jira/browse/AURORA-1166 Repository: aurora Description --- Discovered these while working on AURORA-1157 Diffs - src/main/python/apache/aurora/client/cli/update.py ee8146be0cb6e3f3139a5491c883864950f245bd src/test/python/apache/aurora/client/cli/test_supdate.py 1173650998d397a68a7fabf38201fb9feebf2977 Diff: https://reviews.apache.org/r/31710/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 31710: Fix query and timestamp display when running beta-update status.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31710/#review75083 --- src/test/python/apache/aurora/client/cli/test_supdate.py https://reviews.apache.org/r/31710/#comment122016 For the calls like this you can do `mock_api.pause_job_update.assert_called_once_with(self.TEST_JOBKEY)` instead of comparing against an array of call objects. - Zameer Manji On March 3, 2015, 3:16 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31710/ --- (Updated March 3, 2015, 3:16 p.m.) Review request for Aurora and Maxim Khutornenko. Bugs: AURORA-1165 and AURORA-1166 https://issues.apache.org/jira/browse/AURORA-1165 https://issues.apache.org/jira/browse/AURORA-1166 Repository: aurora Description --- Discovered these while working on AURORA-1157 Diffs - src/main/python/apache/aurora/client/cli/update.py ee8146be0cb6e3f3139a5491c883864950f245bd src/test/python/apache/aurora/client/cli/test_supdate.py 1173650998d397a68a7fabf38201fb9feebf2977 Diff: https://reviews.apache.org/r/31710/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 31710: Fix query and timestamp display when running beta-update status.
On March 3, 2015, 11:21 p.m., Zameer Manji wrote:
src/test/python/apache/aurora/client/cli/test_supdate.py, line 211
https://reviews.apache.org/r/31710/diff/1/?file=883801#file883801line211
For the calls like this you can do
`mock_api.pause_job_update.assert_called_once_with(self.TEST_JOBKEY)`
instead of comparing against an array of call objects.
In case the diff didn't make it clear, i think we should settle on `mock_calls`
for uniformity. Additionally, i think the error messages from asserting on
`mock_calls` is better:
```
_mock_self = MagicMock name='ctime' id='140532173693328', args = (1,)
kwargs = {}, self = MagicMock name='ctime' id='140532173693328'
msg = 'Expected to be called once. Called 9 times.'
???
E AssertionError: Expected to be called once. Called 9 times.
```
vs
```
assert self.mock_ctime.mock_calls == [call(1)]
E AssertionError: assert [call(1),\n ca...(8),\n call(9)] == [call(1)]
E Left contains more items, first extra item: call(2)
E Full diff:
E - [call(1),
E ? ^
E + [call(1)]
E ? ^
E - call(2),
E - call(3),
E - call(4),
E - call(5),
E - call(6),
E - call(7),
E - call(8),
E - call(9)]
```
Moreover, `assert x.mock_calls == [...]` is not vulnerable to typos. In this
test, the first typoed line passes, but the second fails.
```
self.mock_ctime.assret_called_once_with(1)
assert self.mock_ctime.mock_clls == [call(n) for n in range(1, 10)]
```
- Bill
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31710/#review75083
---
On March 3, 2015, 11:16 p.m., Bill Farner wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31710/
---
(Updated March 3, 2015, 11:16 p.m.)
Review request for Aurora and Maxim Khutornenko.
Bugs: AURORA-1165 and AURORA-1166
https://issues.apache.org/jira/browse/AURORA-1165
https://issues.apache.org/jira/browse/AURORA-1166
Repository: aurora
Description
---
Discovered these while working on AURORA-1157
Diffs
-
src/main/python/apache/aurora/client/cli/update.py
ee8146be0cb6e3f3139a5491c883864950f245bd
src/test/python/apache/aurora/client/cli/test_supdate.py
1173650998d397a68a7fabf38201fb9feebf2977
Diff: https://reviews.apache.org/r/31710/diff/
Testing
---
Thanks,
Bill Farner
Re: Review Request 31710: Fix query and timestamp display when running beta-update status.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31710/#review75089 --- Ship it! src/main/python/apache/aurora/client/cli/update.py https://reviews.apache.org/r/31710/#comment122025 You may want to extract an internal method to do this, e.g.: ``` def timestamp(time_msec): return time.ctime(time_msec / 1000) ``` - Maxim Khutornenko On March 3, 2015, 11:44 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31710/ --- (Updated March 3, 2015, 11:44 p.m.) Review request for Aurora and Maxim Khutornenko. Bugs: AURORA-1165 and AURORA-1166 https://issues.apache.org/jira/browse/AURORA-1165 https://issues.apache.org/jira/browse/AURORA-1166 Repository: aurora Description --- Discovered these while working on AURORA-1157 Diffs - src/main/python/apache/aurora/client/cli/update.py ee8146be0cb6e3f3139a5491c883864950f245bd src/test/python/apache/aurora/client/cli/test_supdate.py 1173650998d397a68a7fabf38201fb9feebf2977 Diff: https://reviews.apache.org/r/31710/diff/ Testing --- Thanks, Bill Farner
