HI Jim and Brock,
I've updated my patch with your suggestions, however not all of them were met. Please see my comments inline (sorry for the formatting, I'm not sure why Apache instance of JIRA is ignoring formatting tags):
CounterWrapper should use private instance variables
I've changed the type to explicit private.
CounterWrapper can use else statements in the findCounterValue methods, since if one counters variable is null the other must not be
Changed. I do agree that only one counter might be filled. My original intention with two if statements was to have defined default value that can be used for distinguishing case where the counter is not present at all.
TestDriver's with methods dont need to return this, they should return void since the calling methods wont use that result
Not changed. I do agree that return value in TestDriver.withCounter() is not used, however changing it to void will result in compile errors:
[ERROR] /home/jarcec/projects/apache/mrunit/src/main/java/org/apache/hadoop/mrunit/MapReduceDriver.java:[381,49] withCounter(java.lang.String,java.lang.String,long) in org.apache.hadoop.mrunit.MapReduceDriver cannot override withCounter(java.lang.String,java.lang.String,long) in org.apache.hadoop.mrunit.TestDriver; attempting to use incompatible return type
[ERROR] found : org.apache.hadoop.mrunit.MapReduceDriver<K1,V1,K2,V2,K3,V3>
[ERROR] required: void
TestDriver's validate should use fail(msg) not RuntimeException
Changed. Thank you for pointing this out. I've started working on the patch when we were still using RuntimeExceptions.
You should use the ExpectedSuppliedException class in test mrunit to verify the exception message returned
Changed as well.
I think 0 would be a clearer default value
I've removed default value completely.