aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Joe Smith" <yasumo...@gmail.com>
Subject Re: Review Request 18334: Move and unit test Maintenance module and commands
Date Tue, 04 Mar 2014 02:26:43 GMT


> On Feb. 24, 2014, 6:36 p.m., Kevin Sweeney wrote:
> > src/main/python/apache/aurora/admin/mesos_maintenance.py, line 73
> > <https://reviews.apache.org/r/18334/diff/3/?file=502368#file502368line73>
> >
> >     Inclined to revert this - better to explicitly call out a dependency on system
time IMO.

done


> On Feb. 24, 2014, 6:36 p.m., Kevin Sweeney wrote:
> > src/main/python/apache/aurora/client/commands/maintenance.py, lines 53-56
> > <https://reviews.apache.org/r/18334/diff/3/?file=502372#file502372line53>
> >
> >     DRY - these options are used multiple times. Consider factoring them as constants.

fixed


> On Feb. 24, 2014, 6:36 p.m., Kevin Sweeney wrote:
> > src/test/python/apache/aurora/client/commands/test_maintenance.py, line 43
> > <https://reviews.apache.org/r/18334/diff/3/?file=502376#file502376line43>
> >
> >     make_mock_options would be more descriptive. also docstring is not informative,
consider dropping it.

fixed


> On Feb. 24, 2014, 6:36 p.m., Kevin Sweeney wrote:
> > src/test/python/apache/aurora/client/commands/test_maintenance.py, line 88
> > <https://reviews.apache.org/r/18334/diff/3/?file=502376#file502376line88>
> >
> >     parens aren't necessary

fixed


> On Feb. 24, 2014, 6:36 p.m., Kevin Sweeney wrote:
> > src/test/python/apache/aurora/client/commands/test_maintenance.py, line 103
> > <https://reviews.apache.org/r/18334/diff/3/?file=502376#file502376line103>
> >
> >     parens not necessary

fixed


> On Feb. 24, 2014, 6:36 p.m., Kevin Sweeney wrote:
> > src/test/python/apache/aurora/client/commands/test_maintenance.py, line 129
> > <https://reviews.apache.org/r/18334/diff/3/?file=502376#file502376line129>
> >
> >     no parens needed.

fixed


> On Feb. 24, 2014, 6:36 p.m., Kevin Sweeney wrote:
> > src/test/python/apache/aurora/client/commands/test_maintenance.py, line 136
> > <https://reviews.apache.org/r/18334/diff/3/?file=502376#file502376line136>
> >
> >     Not sure how I feel about this style of mock - I'd prefer the class under test
to explicitly call out "I'm using system time" to avoid missed mock coverage causing the test
suite to slow down.

hm.. any chance you have suggestions on how to pass this through? I'd prefer not to add it
to the command line itself, I think


- Joe


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18334/#review35362
-----------------------------------------------------------


On Feb. 24, 2014, 10:19 a.m., Joe Smith wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18334/
> -----------------------------------------------------------
> 
> (Updated Feb. 24, 2014, 10:19 a.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney, Mark Chu-Carroll, and Brian Wickman.
> 
> 
> Bugs: AURORA-223
>     https://issues.apache.org/jira/browse/AURORA-223
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> As always, feel free to tear it apart.
> 
> I plan to add tests for the other commands as well, but wanted to get this out sooner
than later to ensure others agreed with my approach.
> 
> 
> Diffs
> -----
> 
>   src/main/python/apache/aurora/admin/BUILD 480dad69d8122168a6b7222ef9df0eb689fae386

>   src/main/python/apache/aurora/admin/mesos_maintenance.py d8ffdec062db07ce82556fb38ff4a5eea7921953

>   src/main/python/apache/aurora/client/bin/BUILD dbabfd0e56288d04c399e20976ea6e0287112d91

>   src/main/python/apache/aurora/client/commands/BUILD 14bbdd4d0cb0e8e5102e75baef4e8b34d98967c0

>   src/main/python/apache/aurora/client/commands/admin.py 989c5b625b48fe67ef1297ceda8d7e35cb8ead7e

>   src/main/python/apache/aurora/client/commands/maintenance.py PRE-CREATION 
>   src/test/python/apache/aurora/admin/BUILD 258b1eb58a4eec1650bfd317823f76a30889f912

>   src/test/python/apache/aurora/admin/test_mesos_maintenance.py 000fbe5a873669bf55a9110f1aa6f1d7c8350ef8

>   src/test/python/apache/aurora/client/commands/BUILD 6d448d7320a78bf140873b743805846c179281a7

>   src/test/python/apache/aurora/client/commands/test_maintenance.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/18334/diff/
> 
> 
> Testing
> -------
> 
> ./pants ./src/test/python/apache/aurora:all
> 
> particularly:
> 
> src.test.python.apache.aurora.client.commands.maintenance                       .....
  SUCCESS
> 
> 
> Thanks,
> 
> Joe Smith
> 
>


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