aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bill Farner" <wfar...@apache.org>
Subject Re: Review Request 17131: Improve test coverage for CronJobManager.
Date Wed, 22 Jan 2014 18:54:04 GMT


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


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