On 02.06.2015 11:20, Paul Sandoz wrote:
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 }
Alright, I will use expectedExceptions here.
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}
I agree there is not much value in testing the case source == null, so
it can be removed.
However, I would leave other test cases.
Earlier in this thread Rémi spotted a bug, which had been observed in a
call "bar".replace("foo", null), but not in "foo".replace("foo", null).
That was my motivation to extend the number of test cases for null inputs.
Sincerely yours,
Ivan
No need for another review for any of these.
Paul.
