Re: Review Request 18334: Move and unit test Maintenance module and commands
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18334/ --- (Updated March 7, 2014, 5:20 p.m.) Review request for Aurora, Kevin Sweeney, Mark Chu-Carroll, Maxim Khutornenko, and Brian Wickman. Changes --- Maxim- if you can give one last review then ship this, that'd be rad. 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 (updated) - src/main/python/apache/aurora/admin/BUILD 480dad69d8122168a6b7222ef9df0eb689fae386 src/main/python/apache/aurora/admin/mesos_maintenance.py d8ffdec062db07ce82556fb38ff4a5eea7921953 src/main/python/apache/aurora/client/base.py c5c969f9641c8e0e1604648b1f9ed542ab59b7d0 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 d6f571bec5325f7ab538a7dd9389d9f98d01134a 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
Re: Review Request 18334: Move and unit test Maintenance module and commands
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. Joe Smith wrote: 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 Kevin Sweeney wrote: The clock=time pattern is more explicit to me since it's called out in production code. In the test you can pass clock=mock_time. ah, perhaps this is an interesting code smell then, as this is actually testing the command-line itself, which calls the library method (that has the clock=time parameter). I'm not sure how to go from this test (which is passing in app options) to the mock clock (maybe adding in a mock_clock=True option..?) - Joe --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18334/#review35362 --- On March 3, 2014, 6:50 p.m., Joe Smith wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18334/ --- (Updated March 3, 2014, 6:50 p.m.) Review request for Aurora, Kevin Sweeney, Mark Chu-Carroll, Maxim Khutornenko, 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/base.py c5c969f9641c8e0e1604648b1f9ed542ab59b7d0 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 40588e23f5082604af9f474e9cf6ab84663b8d67 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
Re: Review Request 18334: Move and unit test Maintenance module and commands
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
Re: Review Request 18334: Move and unit test Maintenance module and commands
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18334/ --- (Updated March 3, 2014, 6:50 p.m.) Review request for Aurora, Kevin Sweeney, Mark Chu-Carroll, Maxim Khutornenko, and Brian Wickman. Changes --- Updated, and also adding maxim as we've been dabbling in aurora_admin together (and I'm moving parse_hosts to base) 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 (updated) - src/main/python/apache/aurora/admin/BUILD 480dad69d8122168a6b7222ef9df0eb689fae386 src/main/python/apache/aurora/admin/mesos_maintenance.py d8ffdec062db07ce82556fb38ff4a5eea7921953 src/main/python/apache/aurora/client/base.py c5c969f9641c8e0e1604648b1f9ed542ab59b7d0 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 40588e23f5082604af9f474e9cf6ab84663b8d67 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
Re: Review Request 18334: Move and unit test Maintenance module and commands
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18334/#review35152 --- Ship it! Ship It! - Mark Chu-Carroll On Feb. 20, 2014, 7:05 p.m., Joe Smith wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18334/ --- (Updated Feb. 20, 2014, 7:05 p.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/base.py c5c969f9641c8e0e1604648b1f9ed542ab59b7d0 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 45686aec8d69f0dfa1d649a92a19ca87bd315823 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 02c27aa513d60ef901f446c56cbfc0d891445e30 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
Review Request 18334: Move and unit test Maintenance module and commands
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18334/ --- 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/base.py c5c969f9641c8e0e1604648b1f9ed542ab59b7d0 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 45686aec8d69f0dfa1d649a92a19ca87bd315823 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 02c27aa513d60ef901f446c56cbfc0d891445e30 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
