Re: Review Request 17131: Improve test coverage for CronJobManager.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17131/#review32516 --- Ship it! Ship It! - Suman Karumuri On Jan. 20, 2014, 9:01 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17131/ --- (Updated Jan. 20, 2014, 9:01 p.m.) Review request for Aurora, Suman Karumuri and Maxim Khutornenko. Bugs: AURORA-62 https://issues.apache.org/jira/browse/AURORA-62 Repository: aurora Description --- This raises instruction test coverage from 76% to 95%, and branch coverage from 75% to 96%. There are only two things not currently covered: - Handling when catching InterruptedException (this only logs) - Handling unknown CollisionPolicy (only logs) Diffs - src/main/java/org/apache/aurora/scheduler/state/CronJobManager.java 5a56a701a6a355f9de3f05fbb95013b96b06b171 src/test/java/org/apache/aurora/scheduler/state/CronJobManagerTest.java e9886cdb279cc42a961d6c964e2cfae3c4c13f61 Diff: https://reviews.apache.org/r/17131/diff/ Testing --- ./gradlew build Thanks, Bill Farner
Re: Review Request 17131: Improve test coverage for CronJobManager.
On Jan. 21, 2014, 3:29 p.m., Suman Karumuri wrote: src/main/java/org/apache/aurora/scheduler/state/CronJobManager.java, line 456 https://reviews.apache.org/r/17131/diff/1/?file=432900#file432900line456 The name CronJob sounds too generic. Consider changing it to SanitizedCronJob or a similar name that captures intent. Done. However, i generally favor brevity/readability when scope is as limited as it is here. The real intention was to put a type in the way to ensure that sanitization is performed. On Jan. 21, 2014, 3:29 p.m., Suman Karumuri wrote: src/main/java/org/apache/aurora/scheduler/state/CronJobManager.java, line 461 https://reviews.apache.org/r/17131/diff/1/?file=432900#file432900line461 inline this variable? Thanks, done. On Jan. 21, 2014, 3:29 p.m., Suman Karumuri wrote: src/test/java/org/apache/aurora/scheduler/state/CronJobManagerTest.java, line 192 https://reviews.apache.org/r/17131/diff/1/?file=432901#file432901line192 Would it be better if these comments are changed to JavaDoc comments? I'm not a fan of that idea since javadoc is really intended to be run through the javadoc tool for formal API documentation, which this is not. It would also force me to document the exception thrown, which wouldn't offer any value. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17131/#review32365 --- On Jan. 20, 2014, 9:01 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17131/ --- (Updated Jan. 20, 2014, 9:01 p.m.) Review request for Aurora, Suman Karumuri and Maxim Khutornenko. Bugs: AURORA-62 https://issues.apache.org/jira/browse/AURORA-62 Repository: aurora Description --- This raises instruction test coverage from 76% to 95%, and branch coverage from 75% to 96%. There are only two things not currently covered: - Handling when catching InterruptedException (this only logs) - Handling unknown CollisionPolicy (only logs) Diffs - src/main/java/org/apache/aurora/scheduler/state/CronJobManager.java 5a56a701a6a355f9de3f05fbb95013b96b06b171 src/test/java/org/apache/aurora/scheduler/state/CronJobManagerTest.java e9886cdb279cc42a961d6c964e2cfae3c4c13f61 Diff: https://reviews.apache.org/r/17131/diff/ Testing --- ./gradlew build Thanks, Bill Farner
Re: Review Request 17131: Improve test coverage for CronJobManager.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17131/ --- (Updated Jan. 22, 2014, 7:02 p.m.) Review request for Aurora, Suman Karumuri and Maxim Khutornenko. Changes --- CronJob - SanitizedCronJob. Bugs: AURORA-62 https://issues.apache.org/jira/browse/AURORA-62 Repository: aurora Description --- This raises instruction test coverage from 76% to 95%, and branch coverage from 75% to 96%. There are only two things not currently covered: - Handling when catching InterruptedException (this only logs) - Handling unknown CollisionPolicy (only logs) Diffs (updated) - src/main/java/org/apache/aurora/scheduler/state/CronJobManager.java 5a56a701a6a355f9de3f05fbb95013b96b06b171 src/test/java/org/apache/aurora/scheduler/state/CronJobManagerTest.java e9886cdb279cc42a961d6c964e2cfae3c4c13f61 Diff: https://reviews.apache.org/r/17131/diff/ Testing --- ./gradlew build Thanks, Bill Farner
Re: Review Request 17131: Improve test coverage for CronJobManager.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17131/#review32376 --- Ship it! src/main/java/org/apache/aurora/scheduler/state/CronJobManager.java https://reviews.apache.org/r/17131/#comment61173 +1 for inlining it. - Maxim Khutornenko On Jan. 20, 2014, 9:01 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17131/ --- (Updated Jan. 20, 2014, 9:01 p.m.) Review request for Aurora, Suman Karumuri and Maxim Khutornenko. Bugs: AURORA-62 https://issues.apache.org/jira/browse/AURORA-62 Repository: aurora Description --- This raises instruction test coverage from 76% to 95%, and branch coverage from 75% to 96%. There are only two things not currently covered: - Handling when catching InterruptedException (this only logs) - Handling unknown CollisionPolicy (only logs) Diffs - src/main/java/org/apache/aurora/scheduler/state/CronJobManager.java 5a56a701a6a355f9de3f05fbb95013b96b06b171 src/test/java/org/apache/aurora/scheduler/state/CronJobManagerTest.java e9886cdb279cc42a961d6c964e2cfae3c4c13f61 Diff: https://reviews.apache.org/r/17131/diff/ Testing --- ./gradlew build Thanks, Bill Farner
