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