> On May 27, 2015, 9:13 p.m., Chris Westin wrote: > > 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? > > abdelhakim deneche wrote: > I couldn't figure out the reason, but using a repeat rule didn't seem to > expose memory leaks. I had to use a "simple" for loop instead
+ Not all tests require repetition. I will add the annotation to a few tests that need it. + I did not aim for the test to expose memory leaks. I added repitition because some tests hang sporadically. > On May 27, 2015, 9:13 p.m., Chris Westin wrote: > > common/src/main/java/org/apache/drill/common/util/RepeatTestRule.java, line > > 62 > > <https://reviews.apache.org/r/34690/diff/1/?file=972381#file972381line62> > > > > Should this be wrapped in a try-catch so that an exception doesn't stop > > the repetition? No. The test should fail on the first failure during repitition. > On May 27, 2015, 9:13 p.m., Chris Westin wrote: > > exec/java-exec/src/test/java/org/apache/drill/exec/testing/ControlsInjectionBuilder.java, > > line 32 > > <https://reviews.apache.org/r/34690/diff/1/?file=972393#file972393line32> > > > > 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. Will add docs. > On May 27, 2015, 9:13 p.m., Chris Westin wrote: > > exec/java-exec/src/test/java/org/apache/drill/exec/testing/ControlsInjectionUtil.java, > > line 98 > > <https://reviews.apache.org/r/34690/diff/1/?file=972394#file972394line98> > > > > Right, so do we still need these at all? Yes, the builder itself uses these. In future, other tests could use these methods too. > On May 27, 2015, 9:13 p.m., Chris Westin wrote: > > exec/java-exec/src/test/java/org/apache/drill/exec/testing/ControlsInjectionUtil.java, > > line 54 > > <https://reviews.apache.org/r/34690/diff/1/?file=972394#file972394line54> > > > > 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. Let's not merge these. Hakim's change implicitly uses a client-side silent listener, and resiliency tests don't need that. - Sudheesh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34690/#review85399 ----------------------------------------------------------- On May 27, 2015, 12:52 a.m., Sudheesh Katkam wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34690/ > ----------------------------------------------------------- > > (Updated May 27, 2015, 12:52 a.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 > >
