aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Mark Chu-Carroll" <mchucarr...@twopensource.com>
Subject Re: Review Request 26137: Fix help for new update command.
Date Fri, 03 Oct 2014 12:55:21 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message