> On Nov. 4, 2014, 11:28 p.m., Bill Farner wrote:
> > I'm not a fan of the refactor.  For one, static analysis tools will no 
> > longer catch missing case coverage, and we have to expose the internal 
> > detail of this map.  Instead, the unit test should self-check to make sure 
> > that it is exercising all transaction operation types.
> 
> Maxim Khutornenko wrote:
>     "For one, static analysis tools will no longer catch missing case 
> coverage" - it's actually the opposite. There was no previous enforcement but 
> after this refactoring there must be a test case or the build fails with this:
>     ```
>       Test coverage missing for 
> org/apache/aurora/scheduler/storage/log/LogStorage$21
>       Test coverage missing for 
> org/apache/aurora/scheduler/storage/log/LogStorage$11
>       Test coverage missing for 
> org/apache/aurora/scheduler/storage/log/LogStorage$10
>       Test coverage missing for 
> org/apache/aurora/scheduler/storage/log/LogStorage$13
>       Test coverage missing for 
> org/apache/aurora/scheduler/storage/log/LogStorage$12
>       Test coverage missing for 
> org/apache/aurora/scheduler/storage/log/LogStorage$15
>       Test coverage missing for 
> org/apache/aurora/scheduler/storage/log/LogStorage$17
>       Test coverage missing for 
> org/apache/aurora/scheduler/storage/log/LogStorage$16
>       Test coverage missing for 
> org/apache/aurora/scheduler/storage/log/LogStorage$18
>       Test coverage missing for 
> org/apache/aurora/scheduler/storage/log/LogStorage$20
>       Test coverage missing for 
> org/apache/aurora/scheduler/storage/log/LogStorage$2
>       Test coverage missing for 
> org/apache/aurora/scheduler/storage/log/LogStorage$3
>       Test coverage missing for 
> org/apache/aurora/scheduler/storage/log/LogStorage$4
>       Test coverage missing for 
> org/apache/aurora/scheduler/storage/log/LogStorage$5
>       Test coverage missing for 
> org/apache/aurora/scheduler/storage/log/LogStorage$9
>     ```
> 
> Bill Farner wrote:
>     Touche!  That's even better.  However, i still would rather avoid leaking 
> the internal Map if we can help it.
> 
> Maxim Khutornenko wrote:
>     I am not sure how else we can fully validate the expected coverage 
> without exposing the internal defs. Any suggestions?
> 
> Bill Farner wrote:
>     Move the map to the test, make sure you have test data for every enum 
> value for `LogEntry._Fields` and `Op._Fields`.

Would not that be less safe though? Test code could inadvertently fake coverage 
without verifying the production side. Given that map-exposing methods are 
package internal I don't see how that could mess anything up.


- Maxim


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


On Nov. 4, 2014, 11:10 p.m., Maxim Khutornenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27598/
> -----------------------------------------------------------
> 
> (Updated Nov. 4, 2014, 11:10 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-912
>     https://issues.apache.org/jira/browse/AURORA-912
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Fixing the log replay for the job update history pruner.
> 
> Also, refactored LogStorage replay routine to fully test all 
> LogEntry/LogEntry.TRANSACTION handlers.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
> cbab75964052a950e1b868b3a53eb15fadb31cb7 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> 8eb5c3f7c542206066b39a09911c7df01a43bee7 
> 
> Diff: https://reviews.apache.org/r/27598/diff/
> 
> 
> Testing
> -------
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Reply via email to