You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Bill Farner <wf...@apache.org> on 2014/01/20 22:02:01 UTC
Review Request 17131: Improve test coverage for CronJobManager.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17131/
-----------------------------------------------------------
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.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 17131: Improve test coverage for CronJobManager.
Posted by Bill Farner <wf...@apache.org>.
> 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.
Posted by Suman Karumuri <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17131/#review32365
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/state/CronJobManager.java
<https://reviews.apache.org/r/17131/#comment61164>
The name CronJob sounds too generic. Consider changing it to SanitizedCronJob or a similar name that captures intent.
src/main/java/org/apache/aurora/scheduler/state/CronJobManager.java
<https://reviews.apache.org/r/17131/#comment61165>
inline this variable?
src/test/java/org/apache/aurora/scheduler/state/CronJobManagerTest.java
<https://reviews.apache.org/r/17131/#comment61166>
Would it be better if these comments are changed to JavaDoc comments?
- 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.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
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.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17131/
-----------------------------------------------------------
(Updated Jan. 22, 2014, 6:56 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 (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.
Posted by Suman Karumuri <ma...@apache.org>.
-----------------------------------------------------------
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
>
>