> On Nov. 4, 2014, 3: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`.
>
> Maxim Khutornenko wrote:
> 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.
After going back and forth I really like this factoring of the production code
- the ability to write
```java
assertEquals("A handler exists for each log entry type.",
EnumSet.allOf(LogEntry._Fields.class), dispatchers.keySet());
```
in a test is powerful. It also lets us factor out unweildly handlers into their
own separately-tested classes with their own dependencies as the need arises.
- Kevin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27598/#review59868
-----------------------------------------------------------
On Nov. 4, 2014, 3: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, 3: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
>
>
