You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Mark Chu-Carroll <mc...@twopensource.com> on 2014/09/29 17:05:39 UTC
Review Request 26137: Fix help for new update command.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26137/
-----------------------------------------------------------
Review request for Aurora, David McLaughlin and Zameer Manji.
Bugs: aurora-748
https://issues.apache.org/jira/browse/aurora-748
Repository: aurora
Description
-------
Fix help for new update command.
Diffs
-----
src/main/python/apache/aurora/client/cli/update.py b4dd792dc12f19424c620f4d91748113e272f0c9
Diff: https://reviews.apache.org/r/26137/diff/
Testing
-------
manual testing.
Thanks,
Mark Chu-Carroll
Re: Review Request 26137: Fix help for new update command.
Posted by Zameer Manji <zm...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26137/#review54894
-----------------------------------------------------------
Ship it!
Ship It!
- Zameer Manji
On Sept. 29, 2014, 8:05 a.m., Mark Chu-Carroll wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26137/
> -----------------------------------------------------------
>
> (Updated Sept. 29, 2014, 8:05 a.m.)
>
>
> Review request for Aurora, David McLaughlin and Zameer Manji.
>
>
> Bugs: aurora-748
> https://issues.apache.org/jira/browse/aurora-748
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Fix help for new update command.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/cli/update.py b4dd792dc12f19424c620f4d91748113e272f0c9
>
> Diff: https://reviews.apache.org/r/26137/diff/
>
>
> Testing
> -------
>
> manual testing.
>
>
> Thanks,
>
> Mark Chu-Carroll
>
>
Re: Review Request 26137: Fix help for new update command.
Posted by David McLaughlin <da...@dmclaughlin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26137/#review55125
-----------------------------------------------------------
Ship it!
Ship It!
- David McLaughlin
On Sept. 29, 2014, 3:05 p.m., Mark Chu-Carroll wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26137/
> -----------------------------------------------------------
>
> (Updated Sept. 29, 2014, 3:05 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Zameer Manji.
>
>
> Bugs: aurora-748
> https://issues.apache.org/jira/browse/aurora-748
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Fix help for new update command.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/cli/update.py b4dd792dc12f19424c620f4d91748113e272f0c9
>
> Diff: https://reviews.apache.org/r/26137/diff/
>
>
> Testing
> -------
>
> manual testing.
>
>
> Thanks,
>
> Mark Chu-Carroll
>
>
Re: Review Request 26137: Fix help for new update command.
Posted by Joe Smith <ya...@gmail.com>.
> On Sept. 29, 2014, 11:32 p.m., Joe Smith wrote:
> > src/main/python/apache/aurora/client/cli/update.py, line 45
> > <https://reviews.apache.org/r/26137/diff/1/?file=708198#file708198line45>
> >
> > Could you update a test case to catch accessing these as properties to catch accidental regressions?
>
> David McLaughlin wrote:
> Piggy backing this issue to add that my ship it is pending a test for this command at least?
>
> Mark Chu-Carroll wrote:
> I don't know of any way to write a single test that would always catch this.
Rebased off your diff:
[tw-172-25-132-201 aurora (yasumoto/test_update_help)]$ git diff
diff --git a/src/main/python/apache/aurora/client/cli/update.py b/src/main/python/apache/aurora/client/cli/update.py
index 41475a7..ef07a11 100644
--- a/src/main/python/apache/aurora/client/cli/update.py
+++ b/src/main/python/apache/aurora/client/cli/update.py
@@ -42,7 +42,6 @@ class StartUpdate(Verb):
INSTANCES_SPEC_ARGUMENT, CONFIG_ARGUMENT
]
- @property
def help(self):
return textwrap.dedent("""\
Start a scheduler-driven rolling upgrade on a running job, using the update
diff --git a/src/test/python/apache/aurora/client/cli/test_update.py b/src/test/python/apache/aurora/client/cli/test_update.py
index eeed774..1a38ffe 100644
--- a/src/test/python/apache/aurora/client/cli/test_update.py
+++ b/src/test/python/apache/aurora/client/cli/test_update.py
@@ -23,6 +23,7 @@ from apache.aurora.client.api.quota_check import QuotaCheck
from apache.aurora.client.api.scheduler_mux import SchedulerMux
from apache.aurora.client.cli import EXIT_INVALID_CONFIGURATION, EXIT_OK
from apache.aurora.client.cli.client import AuroraCommandLine
+from apache.aurora.client.cli.update import StartUpdate
from apache.aurora.client.cli.util import AuroraClientCommandTest, FakeAuroraCommandContext, IOMock
from apache.aurora.config import AuroraConfig
@@ -301,6 +302,10 @@ class TestUpdateCommand(AuroraClientCommandTest):
'Update completed successfully']
assert mock_err.get() == []
+ def test_update_start_help(self):
+ start = StartUpdate()
+ assert 'Start a scheduler-driven rolling' in start.help
+
@classmethod
def assert_correct_addinstance_calls(cls, api):
assert api.addInstances.call_count == 20
[tw-172-25-132-201 aurora (yasumoto/test_update_help)]$ ./pants ./src/test/python/apache/aurora/client/cli:job
Build operating on top level addresses: set([BuildFileAddress(/Users/jsmith/workspace/aurora/src/test/python/apache/aurora/client/cli/BUILD, job)])
==================================================================================================================================================== test session starts =====================================================================================================================================================
platform darwin -- Python 2.6.8 -- py-1.4.25 -- pytest-2.6.3
plugins: cov, timeout
collected 61 items
src/test/python/apache/aurora/client/cli/test_cancel_update.py ..
src/test/python/apache/aurora/client/cli/test_create.py ..........
src/test/python/apache/aurora/client/cli/test_diff.py ...
src/test/python/apache/aurora/client/cli/test_kill.py ................
src/test/python/apache/aurora/client/cli/test_open.py .....
src/test/python/apache/aurora/client/cli/test_restart.py ........
src/test/python/apache/aurora/client/cli/test_status.py ...........
src/test/python/apache/aurora/client/cli/test_update.py ..F...
========================================================================================================================================================== FAILURES ==========================================================================================================================================================
__________________________________________________________________________________________________________________________________________ TestUpdateCommand.test_update_start_help __________________________________________________________________________________________________________________________________________
self = <test_update.TestUpdateCommand testMethod=test_update_start_help>
def test_update_start_help(self):
start = StartUpdate()
> assert 'Start a scheduler-driven rolling' in start.help
E TypeError: argument of type 'instancemethod' is not iterable
src/test/python/apache/aurora/client/cli/test_update.py:307: TypeError
============================================================================================================================================ 1 failed, 60 passed in 6.07 seconds =============================================================================================================================================
src.test.python.apache.aurora.client.cli.job ..... FAILURE
[tw-172-25-132-201 aurora (yasumoto/test_update_help)]$ git diff
diff --git a/src/test/python/apache/aurora/client/cli/test_update.py b/src/test/python/apache/aurora/client/cli/test_update.py
index eeed774..1a38ffe 100644
--- a/src/test/python/apache/aurora/client/cli/test_update.py
+++ b/src/test/python/apache/aurora/client/cli/test_update.py
@@ -23,6 +23,7 @@ from apache.aurora.client.api.quota_check import QuotaCheck
from apache.aurora.client.api.scheduler_mux import SchedulerMux
from apache.aurora.client.cli import EXIT_INVALID_CONFIGURATION, EXIT_OK
from apache.aurora.client.cli.client import AuroraCommandLine
+from apache.aurora.client.cli.update import StartUpdate
from apache.aurora.client.cli.util import AuroraClientCommandTest, FakeAuroraCommandContext, IOMock
from apache.aurora.config import AuroraConfig
@@ -301,6 +302,10 @@ class TestUpdateCommand(AuroraClientCommandTest):
'Update completed successfully']
assert mock_err.get() == []
+ def test_update_start_help(self):
+ start = StartUpdate()
+ assert 'Start a scheduler-driven rolling' in start.help
+
@classmethod
def assert_correct_addinstance_calls(cls, api):
assert api.addInstances.call_count == 20
[tw-172-25-132-201 aurora (yasumoto/test_update_help)]$ ./pants ./src/test/python/apache/aurora/client/cli:job
Build operating on top level addresses: set([BuildFileAddress(/Users/jsmith/workspace/aurora/src/test/python/apache/aurora/client/cli/BUILD, job)])
==================================================================================================================================================== test session starts =====================================================================================================================================================
platform darwin -- Python 2.6.8 -- py-1.4.25 -- pytest-2.6.3
plugins: cov, timeout
collected 61 items
src/test/python/apache/aurora/client/cli/test_cancel_update.py ..
src/test/python/apache/aurora/client/cli/test_create.py ..........
src/test/python/apache/aurora/client/cli/test_diff.py ...
src/test/python/apache/aurora/client/cli/test_kill.py ................
src/test/python/apache/aurora/client/cli/test_open.py .....
src/test/python/apache/aurora/client/cli/test_restart.py ........
src/test/python/apache/aurora/client/cli/test_status.py ...........
src/test/python/apache/aurora/client/cli/test_update.py ......
================================================================================================================================================= 61 passed in 5.74 seconds ==================================================================================================================================================
src.test.python.apache.aurora.client.cli.job ..... SUCCESS
- Joe
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26137/#review54953
-----------------------------------------------------------
On Sept. 29, 2014, 8:05 a.m., Mark Chu-Carroll wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26137/
> -----------------------------------------------------------
>
> (Updated Sept. 29, 2014, 8:05 a.m.)
>
>
> Review request for Aurora, David McLaughlin and Zameer Manji.
>
>
> Bugs: aurora-748
> https://issues.apache.org/jira/browse/aurora-748
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Fix help for new update command.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/cli/update.py b4dd792dc12f19424c620f4d91748113e272f0c9
>
> Diff: https://reviews.apache.org/r/26137/diff/
>
>
> Testing
> -------
>
> manual testing.
>
>
> Thanks,
>
> Mark Chu-Carroll
>
>
Re: Review Request 26137: Fix help for new update command.
Posted by Mark Chu-Carroll <mc...@twopensource.com>.
> On Sept. 30, 2014, 2:32 a.m., Joe Smith wrote:
> > src/main/python/apache/aurora/client/cli/update.py, line 45
> > <https://reviews.apache.org/r/26137/diff/1/?file=708198#file708198line45>
> >
> > Could you update a test case to catch accessing these as properties to catch accidental regressions?
>
> David McLaughlin wrote:
> Piggy backing this issue to add that my ship it is pending a test for this command at least?
I don't know of any way to write a single test that would always catch this.
- Mark
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26137/#review54953
-----------------------------------------------------------
On Sept. 29, 2014, 11:05 a.m., Mark Chu-Carroll wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26137/
> -----------------------------------------------------------
>
> (Updated Sept. 29, 2014, 11:05 a.m.)
>
>
> Review request for Aurora, David McLaughlin and Zameer Manji.
>
>
> Bugs: aurora-748
> https://issues.apache.org/jira/browse/aurora-748
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Fix help for new update command.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/cli/update.py b4dd792dc12f19424c620f4d91748113e272f0c9
>
> Diff: https://reviews.apache.org/r/26137/diff/
>
>
> Testing
> -------
>
> manual testing.
>
>
> Thanks,
>
> Mark Chu-Carroll
>
>
Re: Review Request 26137: Fix help for new update command.
Posted by Joe Smith <ya...@gmail.com>.
> On Sept. 29, 2014, 11:32 p.m., Joe Smith wrote:
> > src/main/python/apache/aurora/client/cli/update.py, line 45
> > <https://reviews.apache.org/r/26137/diff/1/?file=708198#file708198line45>
> >
> > Could you update a test case to catch accessing these as properties to catch accidental regressions?
>
> David McLaughlin wrote:
> Piggy backing this issue to add that my ship it is pending a test for this command at least?
>
> Mark Chu-Carroll wrote:
> I don't know of any way to write a single test that would always catch this.
>
> Joe Smith wrote:
> Rebased off your diff:
>
> [tw-172-25-132-201 aurora (yasumoto/test_update_help)]$ git diff
> diff --git a/src/main/python/apache/aurora/client/cli/update.py b/src/main/python/apache/aurora/client/cli/update.py
> index 41475a7..ef07a11 100644
> --- a/src/main/python/apache/aurora/client/cli/update.py
> +++ b/src/main/python/apache/aurora/client/cli/update.py
> @@ -42,7 +42,6 @@ class StartUpdate(Verb):
> INSTANCES_SPEC_ARGUMENT, CONFIG_ARGUMENT
> ]
>
> - @property
> def help(self):
> return textwrap.dedent("""\
> Start a scheduler-driven rolling upgrade on a running job, using the update
> diff --git a/src/test/python/apache/aurora/client/cli/test_update.py b/src/test/python/apache/aurora/client/cli/test_update.py
> index eeed774..1a38ffe 100644
> --- a/src/test/python/apache/aurora/client/cli/test_update.py
> +++ b/src/test/python/apache/aurora/client/cli/test_update.py
> @@ -23,6 +23,7 @@ from apache.aurora.client.api.quota_check import QuotaCheck
> from apache.aurora.client.api.scheduler_mux import SchedulerMux
> from apache.aurora.client.cli import EXIT_INVALID_CONFIGURATION, EXIT_OK
> from apache.aurora.client.cli.client import AuroraCommandLine
> +from apache.aurora.client.cli.update import StartUpdate
> from apache.aurora.client.cli.util import AuroraClientCommandTest, FakeAuroraCommandContext, IOMock
> from apache.aurora.config import AuroraConfig
>
> @@ -301,6 +302,10 @@ class TestUpdateCommand(AuroraClientCommandTest):
> 'Update completed successfully']
> assert mock_err.get() == []
>
> + def test_update_start_help(self):
> + start = StartUpdate()
> + assert 'Start a scheduler-driven rolling' in start.help
> +
> @classmethod
> def assert_correct_addinstance_calls(cls, api):
> assert api.addInstances.call_count == 20
> [tw-172-25-132-201 aurora (yasumoto/test_update_help)]$ ./pants ./src/test/python/apache/aurora/client/cli:job
> Build operating on top level addresses: set([BuildFileAddress(/Users/jsmith/workspace/aurora/src/test/python/apache/aurora/client/cli/BUILD, job)])
> ==================================================================================================================================================== test session starts =====================================================================================================================================================
> platform darwin -- Python 2.6.8 -- py-1.4.25 -- pytest-2.6.3
> plugins: cov, timeout
> collected 61 items
>
> src/test/python/apache/aurora/client/cli/test_cancel_update.py ..
> src/test/python/apache/aurora/client/cli/test_create.py ..........
> src/test/python/apache/aurora/client/cli/test_diff.py ...
> src/test/python/apache/aurora/client/cli/test_kill.py ................
> src/test/python/apache/aurora/client/cli/test_open.py .....
> src/test/python/apache/aurora/client/cli/test_restart.py ........
> src/test/python/apache/aurora/client/cli/test_status.py ...........
> src/test/python/apache/aurora/client/cli/test_update.py ..F...
>
> ========================================================================================================================================================== FAILURES ==========================================================================================================================================================
> __________________________________________________________________________________________________________________________________________ TestUpdateCommand.test_update_start_help __________________________________________________________________________________________________________________________________________
>
> self = <test_update.TestUpdateCommand testMethod=test_update_start_help>
>
> def test_update_start_help(self):
> start = StartUpdate()
> > assert 'Start a scheduler-driven rolling' in start.help
> E TypeError: argument of type 'instancemethod' is not iterable
>
> src/test/python/apache/aurora/client/cli/test_update.py:307: TypeError
> ============================================================================================================================================ 1 failed, 60 passed in 6.07 seconds =============================================================================================================================================
> src.test.python.apache.aurora.client.cli.job ..... FAILURE
>
>
> [tw-172-25-132-201 aurora (yasumoto/test_update_help)]$ git diff
> diff --git a/src/test/python/apache/aurora/client/cli/test_update.py b/src/test/python/apache/aurora/client/cli/test_update.py
> index eeed774..1a38ffe 100644
> --- a/src/test/python/apache/aurora/client/cli/test_update.py
> +++ b/src/test/python/apache/aurora/client/cli/test_update.py
> @@ -23,6 +23,7 @@ from apache.aurora.client.api.quota_check import QuotaCheck
> from apache.aurora.client.api.scheduler_mux import SchedulerMux
> from apache.aurora.client.cli import EXIT_INVALID_CONFIGURATION, EXIT_OK
> from apache.aurora.client.cli.client import AuroraCommandLine
> +from apache.aurora.client.cli.update import StartUpdate
> from apache.aurora.client.cli.util import AuroraClientCommandTest, FakeAuroraCommandContext, IOMock
> from apache.aurora.config import AuroraConfig
>
> @@ -301,6 +302,10 @@ class TestUpdateCommand(AuroraClientCommandTest):
> 'Update completed successfully']
> assert mock_err.get() == []
>
> + def test_update_start_help(self):
> + start = StartUpdate()
> + assert 'Start a scheduler-driven rolling' in start.help
> +
> @classmethod
> def assert_correct_addinstance_calls(cls, api):
> assert api.addInstances.call_count == 20
> [tw-172-25-132-201 aurora (yasumoto/test_update_help)]$ ./pants ./src/test/python/apache/aurora/client/cli:job
> Build operating on top level addresses: set([BuildFileAddress(/Users/jsmith/workspace/aurora/src/test/python/apache/aurora/client/cli/BUILD, job)])
> ==================================================================================================================================================== test session starts =====================================================================================================================================================
> platform darwin -- Python 2.6.8 -- py-1.4.25 -- pytest-2.6.3
> plugins: cov, timeout
> collected 61 items
>
> src/test/python/apache/aurora/client/cli/test_cancel_update.py ..
> src/test/python/apache/aurora/client/cli/test_create.py ..........
> src/test/python/apache/aurora/client/cli/test_diff.py ...
> src/test/python/apache/aurora/client/cli/test_kill.py ................
> src/test/python/apache/aurora/client/cli/test_open.py .....
> src/test/python/apache/aurora/client/cli/test_restart.py ........
> src/test/python/apache/aurora/client/cli/test_status.py ...........
> src/test/python/apache/aurora/client/cli/test_update.py ......
>
> ================================================================================================================================================= 61 passed in 5.74 seconds ==================================================================================================================================================
> src.test.python.apache.aurora.client.cli.job ..... SUCCESS
>
> Mark Chu-Carroll wrote:
> Yes, obviously you can check *this specific* help string, to make sure no one deletes the "@property" annotation. But the likelihood of that happening is close to nil; I have a very hard time seeing any value in adding a test case to prevent that. It's the kind of test that's more harmful than good: it provides a false sense of security, while not actually preventing problems.
>
> The problem that we could encounter in the future is exactly the case that happened here: adding a new noun or verb, and omitting the "@property" from its help or name fields. *That* is the case where I think a test could be useful, but I can't see how we could write one.
https://reviews.apache.org/r/26372/
- Joe
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26137/#review54953
-----------------------------------------------------------
On Sept. 29, 2014, 8:05 a.m., Mark Chu-Carroll wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26137/
> -----------------------------------------------------------
>
> (Updated Sept. 29, 2014, 8:05 a.m.)
>
>
> Review request for Aurora, David McLaughlin and Zameer Manji.
>
>
> Bugs: aurora-748
> https://issues.apache.org/jira/browse/aurora-748
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Fix help for new update command.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/cli/update.py b4dd792dc12f19424c620f4d91748113e272f0c9
>
> Diff: https://reviews.apache.org/r/26137/diff/
>
>
> Testing
> -------
>
> manual testing.
>
>
> Thanks,
>
> Mark Chu-Carroll
>
>
Re: Review Request 26137: Fix help for new update command.
Posted by David McLaughlin <da...@dmclaughlin.com>.
> On Sept. 30, 2014, 6:32 a.m., Joe Smith wrote:
> > src/main/python/apache/aurora/client/cli/update.py, line 45
> > <https://reviews.apache.org/r/26137/diff/1/?file=708198#file708198line45>
> >
> > Could you update a test case to catch accessing these as properties to catch accidental regressions?
Piggy backing this issue to add that my ship it is pending a test for this command at least?
- David
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26137/#review54953
-----------------------------------------------------------
On Sept. 29, 2014, 3:05 p.m., Mark Chu-Carroll wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26137/
> -----------------------------------------------------------
>
> (Updated Sept. 29, 2014, 3:05 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Zameer Manji.
>
>
> Bugs: aurora-748
> https://issues.apache.org/jira/browse/aurora-748
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Fix help for new update command.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/cli/update.py b4dd792dc12f19424c620f4d91748113e272f0c9
>
> Diff: https://reviews.apache.org/r/26137/diff/
>
>
> Testing
> -------
>
> manual testing.
>
>
> Thanks,
>
> Mark Chu-Carroll
>
>
Re: Review Request 26137: Fix help for new update command.
Posted by Mark Chu-Carroll <mc...@twopensource.com>.
> On Sept. 30, 2014, 2:32 a.m., Joe Smith wrote:
> > src/main/python/apache/aurora/client/cli/update.py, line 45
> > <https://reviews.apache.org/r/26137/diff/1/?file=708198#file708198line45>
> >
> > Could you update a test case to catch accessing these as properties to catch accidental regressions?
>
> David McLaughlin wrote:
> Piggy backing this issue to add that my ship it is pending a test for this command at least?
>
> Mark Chu-Carroll wrote:
> I don't know of any way to write a single test that would always catch this.
>
> Joe Smith wrote:
> Rebased off your diff:
>
> [tw-172-25-132-201 aurora (yasumoto/test_update_help)]$ git diff
> diff --git a/src/main/python/apache/aurora/client/cli/update.py b/src/main/python/apache/aurora/client/cli/update.py
> index 41475a7..ef07a11 100644
> --- a/src/main/python/apache/aurora/client/cli/update.py
> +++ b/src/main/python/apache/aurora/client/cli/update.py
> @@ -42,7 +42,6 @@ class StartUpdate(Verb):
> INSTANCES_SPEC_ARGUMENT, CONFIG_ARGUMENT
> ]
>
> - @property
> def help(self):
> return textwrap.dedent("""\
> Start a scheduler-driven rolling upgrade on a running job, using the update
> diff --git a/src/test/python/apache/aurora/client/cli/test_update.py b/src/test/python/apache/aurora/client/cli/test_update.py
> index eeed774..1a38ffe 100644
> --- a/src/test/python/apache/aurora/client/cli/test_update.py
> +++ b/src/test/python/apache/aurora/client/cli/test_update.py
> @@ -23,6 +23,7 @@ from apache.aurora.client.api.quota_check import QuotaCheck
> from apache.aurora.client.api.scheduler_mux import SchedulerMux
> from apache.aurora.client.cli import EXIT_INVALID_CONFIGURATION, EXIT_OK
> from apache.aurora.client.cli.client import AuroraCommandLine
> +from apache.aurora.client.cli.update import StartUpdate
> from apache.aurora.client.cli.util import AuroraClientCommandTest, FakeAuroraCommandContext, IOMock
> from apache.aurora.config import AuroraConfig
>
> @@ -301,6 +302,10 @@ class TestUpdateCommand(AuroraClientCommandTest):
> 'Update completed successfully']
> assert mock_err.get() == []
>
> + def test_update_start_help(self):
> + start = StartUpdate()
> + assert 'Start a scheduler-driven rolling' in start.help
> +
> @classmethod
> def assert_correct_addinstance_calls(cls, api):
> assert api.addInstances.call_count == 20
> [tw-172-25-132-201 aurora (yasumoto/test_update_help)]$ ./pants ./src/test/python/apache/aurora/client/cli:job
> Build operating on top level addresses: set([BuildFileAddress(/Users/jsmith/workspace/aurora/src/test/python/apache/aurora/client/cli/BUILD, job)])
> ==================================================================================================================================================== test session starts =====================================================================================================================================================
> platform darwin -- Python 2.6.8 -- py-1.4.25 -- pytest-2.6.3
> plugins: cov, timeout
> collected 61 items
>
> src/test/python/apache/aurora/client/cli/test_cancel_update.py ..
> src/test/python/apache/aurora/client/cli/test_create.py ..........
> src/test/python/apache/aurora/client/cli/test_diff.py ...
> src/test/python/apache/aurora/client/cli/test_kill.py ................
> src/test/python/apache/aurora/client/cli/test_open.py .....
> src/test/python/apache/aurora/client/cli/test_restart.py ........
> src/test/python/apache/aurora/client/cli/test_status.py ...........
> src/test/python/apache/aurora/client/cli/test_update.py ..F...
>
> ========================================================================================================================================================== FAILURES ==========================================================================================================================================================
> __________________________________________________________________________________________________________________________________________ TestUpdateCommand.test_update_start_help __________________________________________________________________________________________________________________________________________
>
> self = <test_update.TestUpdateCommand testMethod=test_update_start_help>
>
> def test_update_start_help(self):
> start = StartUpdate()
> > assert 'Start a scheduler-driven rolling' in start.help
> E TypeError: argument of type 'instancemethod' is not iterable
>
> src/test/python/apache/aurora/client/cli/test_update.py:307: TypeError
> ============================================================================================================================================ 1 failed, 60 passed in 6.07 seconds =============================================================================================================================================
> src.test.python.apache.aurora.client.cli.job ..... FAILURE
>
>
> [tw-172-25-132-201 aurora (yasumoto/test_update_help)]$ git diff
> diff --git a/src/test/python/apache/aurora/client/cli/test_update.py b/src/test/python/apache/aurora/client/cli/test_update.py
> index eeed774..1a38ffe 100644
> --- a/src/test/python/apache/aurora/client/cli/test_update.py
> +++ b/src/test/python/apache/aurora/client/cli/test_update.py
> @@ -23,6 +23,7 @@ from apache.aurora.client.api.quota_check import QuotaCheck
> from apache.aurora.client.api.scheduler_mux import SchedulerMux
> from apache.aurora.client.cli import EXIT_INVALID_CONFIGURATION, EXIT_OK
> from apache.aurora.client.cli.client import AuroraCommandLine
> +from apache.aurora.client.cli.update import StartUpdate
> from apache.aurora.client.cli.util import AuroraClientCommandTest, FakeAuroraCommandContext, IOMock
> from apache.aurora.config import AuroraConfig
>
> @@ -301,6 +302,10 @@ class TestUpdateCommand(AuroraClientCommandTest):
> 'Update completed successfully']
> assert mock_err.get() == []
>
> + def test_update_start_help(self):
> + start = StartUpdate()
> + assert 'Start a scheduler-driven rolling' in start.help
> +
> @classmethod
> def assert_correct_addinstance_calls(cls, api):
> assert api.addInstances.call_count == 20
> [tw-172-25-132-201 aurora (yasumoto/test_update_help)]$ ./pants ./src/test/python/apache/aurora/client/cli:job
> Build operating on top level addresses: set([BuildFileAddress(/Users/jsmith/workspace/aurora/src/test/python/apache/aurora/client/cli/BUILD, job)])
> ==================================================================================================================================================== test session starts =====================================================================================================================================================
> platform darwin -- Python 2.6.8 -- py-1.4.25 -- pytest-2.6.3
> plugins: cov, timeout
> collected 61 items
>
> src/test/python/apache/aurora/client/cli/test_cancel_update.py ..
> src/test/python/apache/aurora/client/cli/test_create.py ..........
> src/test/python/apache/aurora/client/cli/test_diff.py ...
> src/test/python/apache/aurora/client/cli/test_kill.py ................
> src/test/python/apache/aurora/client/cli/test_open.py .....
> src/test/python/apache/aurora/client/cli/test_restart.py ........
> src/test/python/apache/aurora/client/cli/test_status.py ...........
> src/test/python/apache/aurora/client/cli/test_update.py ......
>
> ================================================================================================================================================= 61 passed in 5.74 seconds ==================================================================================================================================================
> src.test.python.apache.aurora.client.cli.job ..... SUCCESS
Yes, obviously you can check *this specific* help string, to make sure no one deletes the "@property" annotation. But the likelihood of that happening is close to nil; I have a very hard time seeing any value in adding a test case to prevent that. It's the kind of test that's more harmful than good: it provides a false sense of security, while not actually preventing problems.
The problem that we could encounter in the future is exactly the case that happened here: adding a new noun or verb, and omitting the "@property" from its help or name fields. *That* is the case where I think a test could be useful, but I can't see how we could write one.
- Mark
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26137/#review54953
-----------------------------------------------------------
On Sept. 29, 2014, 11:05 a.m., Mark Chu-Carroll wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26137/
> -----------------------------------------------------------
>
> (Updated Sept. 29, 2014, 11:05 a.m.)
>
>
> Review request for Aurora, David McLaughlin and Zameer Manji.
>
>
> Bugs: aurora-748
> https://issues.apache.org/jira/browse/aurora-748
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Fix help for new update command.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/cli/update.py b4dd792dc12f19424c620f4d91748113e272f0c9
>
> Diff: https://reviews.apache.org/r/26137/diff/
>
>
> Testing
> -------
>
> manual testing.
>
>
> Thanks,
>
> Mark Chu-Carroll
>
>
Re: Review Request 26137: Fix help for new update command.
Posted by Joe Smith <ya...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26137/#review54953
-----------------------------------------------------------
src/main/python/apache/aurora/client/cli/update.py
<https://reviews.apache.org/r/26137/#comment95276>
Could you update a test case to catch accessing these as properties to catch accidental regressions?
- Joe Smith
On Sept. 29, 2014, 8:05 a.m., Mark Chu-Carroll wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26137/
> -----------------------------------------------------------
>
> (Updated Sept. 29, 2014, 8:05 a.m.)
>
>
> Review request for Aurora, David McLaughlin and Zameer Manji.
>
>
> Bugs: aurora-748
> https://issues.apache.org/jira/browse/aurora-748
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Fix help for new update command.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/cli/update.py b4dd792dc12f19424c620f4d91748113e272f0c9
>
> Diff: https://reviews.apache.org/r/26137/diff/
>
>
> Testing
> -------
>
> manual testing.
>
>
> Thanks,
>
> Mark Chu-Carroll
>
>