----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34690/#review85399 -----------------------------------------------------------
Do we really need to repeat *everything* in TestDrillbitResilience? I'm sure some tests (like the leak one) benefit from repetition, but were all of them showing problems when repeated? common/src/main/java/org/apache/drill/common/util/RepeatTestRule.java <https://reviews.apache.org/r/34690/#comment136962> I think count() would be a better name for this. common/src/main/java/org/apache/drill/common/util/RepeatTestRule.java <https://reviews.apache.org/r/34690/#comment136949> Should this be wrapped in a try-catch so that an exception doesn't stop the repetition? common/src/main/java/org/apache/drill/common/util/TestTools.java <https://reviews.apache.org/r/34690/#comment136950> Extra "is?" "... the repeat rule *is* applies only ..." exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java <https://reviews.apache.org/r/34690/#comment136966> Can we rename this to assertStateCompleted()? exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java <https://reviews.apache.org/r/34690/#comment136971> "and times out" exec/java-exec/src/test/java/org/apache/drill/exec/testing/ControlsInjectionBuilder.java <https://reviews.apache.org/r/34690/#comment136999> I really like this class. A lot. It makes the tests look really nice. But you've got to add javadoc to (at least) the public methods. exec/java-exec/src/test/java/org/apache/drill/exec/testing/ControlsInjectionUtil.java <https://reviews.apache.org/r/34690/#comment137001> I seem to recall Hakim just added something like this to TestQueryBase or something like that. Let's merge those together and make sure they're in a place they can be used from multiple tests. exec/java-exec/src/test/java/org/apache/drill/exec/testing/ControlsInjectionUtil.java <https://reviews.apache.org/r/34690/#comment137002> Right, so do we still need these at all? - Chris Westin On May 26, 2015, 5:52 p.m., Sudheesh Katkam wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34690/ > ----------------------------------------------------------- > > (Updated May 26, 2015, 5:52 p.m.) > > > Review request for drill, abdelhakim deneche, Chris Westin, and Venki > Korukanti. > > > Bugs: DRILL-2903 > https://issues.apache.org/jira/browse/DRILL-2903 > > > Repository: drill-git > > > Description > ------- > > DRILL-2903: General improvements in TestDrillbitResilience > + Added RepeatTestRule to repeat tests > + Added ControlsInjectionBuilder to build controls in tests > + Added @Ignore and filed JIRAs for failing tests > > > Diffs > ----- > > common/src/main/java/org/apache/drill/common/util/RepeatTestRule.java > PRE-CREATION > common/src/main/java/org/apache/drill/common/util/TestTools.java 5be8d40 > common/src/test/java/org/apache/drill/test/DrillTest.java bbe014f > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScanBatch.java > 6176f77 > > exec/java-exec/src/main/java/org/apache/drill/exec/rpc/ProtobufLengthDecoder.java > 4f075d3 > > exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlTunnel.java > 16b9b63 > > exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjectionImpl.java > 561d816 > > exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/ControlMessageHandler.java > 8ee7d38 > > exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java > 5d07b49 > > exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java > 71b77c6 > > exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java > e5e0700 > > exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java > 696aed8 > > exec/java-exec/src/test/java/org/apache/drill/exec/testing/ControlsInjectionBuilder.java > PRE-CREATION > > exec/java-exec/src/test/java/org/apache/drill/exec/testing/ControlsInjectionUtil.java > 346c6dd > > exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestCountDownLatchInjection.java > c98f54c > > exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestExceptionInjection.java > e3558a1 > > exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestPauseInjection.java > ba29c58 > > Diff: https://reviews.apache.org/r/34690/diff/ > > > Testing > ------- > > Currently running unit tests. Need to run regression tests. > > > Thanks, > > Sudheesh Katkam > >
