On Jun 1, 2015, at 8:53 PM, Ivan Gerasimov <ivan.gerasi...@oracle.com> wrote:
> > > On 01.06.2015 11:33, Paul Sandoz wrote: >> On May 31, 2015, at 6:03 PM, Ivan Gerasimov <ivan.gerasi...@oracle.com> >> wrote: >>> Which is right here: >>> http://cr.openjdk.java.net/~igerasim/8058779/05/webrev/ >>> >> Much better. >> >> For the test can you use RandomFactory recently added to the test library? > > Sure. > Here the updated webrev with this change and a few other minor changes. > http://cr.openjdk.java.net/~igerasim/8058779/06/webrev/ > > The changes are: > - move declaration of i below, > - indent .append(), > - use RandomFactory in the test, > - extend number of test cases with null input. > > Do you think it's ready to go? > Almost :-) Like Sherman i think you should get rid of JavaLangAccess.newStringUnsafe. 78 @Test(dataProvider="sourceTargetReplacementWithNull") You can use the "expectedExceptions" attribute instead of an explicit try/fail/catch. 79 public void testNPE(String source, String target, String replacement) { 80 try { 81 source.replace(target, replacement); 82 fail("Expected to throw NPE: " + source + ".replace(" + target + 83 "," + replacement + ")"); 84 } catch (NullPointerException npe) { 85 } 86 } You could simplify the data provider sourceTargetReplacementWithNull, there is no point testing with a null source. String.replace accepts two arguments, each can be either null or non-null. Thats 2 bits of state, so one can simply be explicit and presumably it should not matter what the non-null argument value (there are enough non-null values supplied by the other data providers): {null, null} {null, "foo"} {"foo", null} No need for another review for any of these. Paul.
