Hi Mandy, Thanks for the review. I wrote the test (and it passed) but since the JCK folks are providing the tests it seemed undesirable to have duplicate tests. Roger On 1/30/2015 5:27 PM, Mandy Chung wrote:

On 1/30/15 2:25 PM, Roger Riggs wrote:

...

Please review this correction of a JapaneseEra range check in java.time. The error was discovered during development of additional conformance tests (to be delivered separately).

Webrev: http://cr.openjdk.java.net/~rriggs//webrev-era-8068278

Looks fine to me. Is it easy to write a regression test to go along with this fix?

Mandy

...

Issue: <https://bugs.openjdk.java.net/browse/JDK-8068278> 8068278 ArrayIndexOutOfBoundsException instead of DateTimeException in j.t.chrono.JapaneseChronology.eraOf()

Thanks, Roger

Hi Mandy, I added a test for the invalid Eras. I do expect the additional conformance tests from the JCK team but this will synchronize with the fix. Webrev updated in place: http://cr.openjdk.java.net/~rriggs/webrev-era-8068278/ Thanks, Roger On 1/30/2015 6:48 PM, Mandy Chung wrote:

On 1/30/15 2:33 PM, Roger Riggs wrote:

...

Hi Mandy,

Thanks for the review.

I wrote the test (and it passed) but since the JCK folks are providing the tests it seemed undesirable to have duplicate tests.

JDK developers don't run JCK tests and I think it'd be nice to have a regression test to go with a fix unless the bug is uncovered by an existing test.

Mandy

...

Roger

On 1/30/2015 5:27 PM, Mandy Chung wrote:

...

On 1/30/15 2:25 PM, Roger Riggs wrote:

...

Please review this correction of a JapaneseEra range check in java.time. The error was discovered during development of additional conformance tests (to be delivered separately).

Webrev: http://cr.openjdk.java.net/~rriggs//webrev-era-8068278

Looks fine to me. Is it easy to write a regression test to go along with this fix?

Mandy

...

Issue: <https://bugs.openjdk.java.net/browse/JDK-8068278> 8068278 ArrayIndexOutOfBoundsException instead of DateTimeException in j.t.chrono.JapaneseChronology.eraOf()

Thanks, Roger

Hi Lance, Ok, 2nd try. http://cr.openjdk.java.net/~rriggs/webrev-era-8068278/ On 2/3/2015 11:21 AM, Lance Andersen wrote:

Hi Roger,

Did you mean to put the @Test annotation on the class itself? It is redundant with the @Test on individual methods but harmless and was there previously.

Also for the test_valueOf test, I would also add the same comment style to it as it is the only one missing a comment Not part of this issue, but fixed.

I think you meant to have test_outofrange uses the values from invalidEras array (though I would use a DataProvider myself) for the values to eraOf as it is currently Wow, my bad. Replaced with a data provider. The overhead of DataProviders is a bit higher; but maybe not enough to notice.

Thanks for reviewing. Roger

JapaneseChronology.INSTANCE.eraOf(Integer.MAX_VALUE);

Best, Lance On Feb 3, 2015, at 11:10 AM, Roger Riggs <Roger.Riggs@oracle.com <mailto:Roger.Riggs@oracle.com>> wrote:

...

Hi Mandy,

I added a test for the invalid Eras. I do expect the additional conformance tests from the JCK team but this will synchronize with the fix.

Webrev updated in place: http://cr.openjdk.java.net/~rriggs/webrev-era-8068278/ <http://cr.openjdk.java.net/%7Erriggs/webrev-era-8068278/>

Thanks, Roger

On 1/30/2015 6:48 PM, Mandy Chung wrote:

...

On 1/30/15 2:33 PM, Roger Riggs wrote:

...

Hi Mandy,

Thanks for the review.

I wrote the test (and it passed) but since the JCK folks are providing the tests it seemed undesirable to have duplicate tests.

JDK developers don't run JCK tests and I think it'd be nice to have a regression test to go with a fix unless the bug is uncovered by an existing test.

Mandy

...

Roger

On 1/30/2015 5:27 PM, Mandy Chung wrote:

...

On 1/30/15 2:25 PM, Roger Riggs wrote:

...

Please review this correction of a JapaneseEra range check in java.time. The error was discovered during development of additional conformance tests (to be delivered separately).

Webrev: http://cr.openjdk.java.net/~rriggs//webrev-era-8068278 <http://cr.openjdk.java.net/%7Erriggs//webrev-era-8068278>

Looks fine to me. Is it easy to write a regression test to go along with this fix?

Mandy

...

Issue: <https://bugs.openjdk.java.net/browse/JDK-8068278> 8068278 ArrayIndexOutOfBoundsException instead of DateTimeException in j.t.chrono.JapaneseChronology.eraOf()

Thanks, Roger

<http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 Lance.Andersen@oracle.com <mailto:Lance.Andersen@oracle.com>

Hi Roger, Thank you. Ship it. Best Lance On Feb 3, 2015, at 11:59 AM, Roger Riggs <Roger.Riggs@Oracle.com> wrote:

Hi Lance,

Ok, 2nd try.

http://cr.openjdk.java.net/~rriggs/webrev-era-8068278/

On 2/3/2015 11:21 AM, Lance Andersen wrote:

...

Hi Roger,

Did you mean to put the @Test annotation on the class itself? It is redundant with the @Test on individual methods but harmless and was there previously.

Also for the test_valueOf test, I would also add the same comment style to it as it is the only one missing a comment Not part of this issue, but fixed.

I think you meant to have test_outofrange uses the values from invalidEras array (though I would use a DataProvider myself) for the values to eraOf as it is currently Wow, my bad. Replaced with a data provider. The overhead of DataProviders is a bit higher; but maybe not enough to notice.

Thanks for reviewing.

Roger

...

JapaneseChronology.INSTANCE.eraOf(Integer.MAX_VALUE);

Best, Lance On Feb 3, 2015, at 11:10 AM, Roger Riggs <Roger.Riggs@oracle.com> wrote:

...

Hi Mandy,

I added a test for the invalid Eras. I do expect the additional conformance tests from the JCK team but this will synchronize with the fix.

Webrev updated in place: http://cr.openjdk.java.net/~rriggs/webrev-era-8068278/

Thanks, Roger

On 1/30/2015 6:48 PM, Mandy Chung wrote:

...

On 1/30/15 2:33 PM, Roger Riggs wrote:

...

Hi Mandy,

Thanks for the review.

I wrote the test (and it passed) but since the JCK folks are providing the tests it seemed undesirable to have duplicate tests.

JDK developers don't run JCK tests and I think it'd be nice to have a regression test to go with a fix unless the bug is uncovered by an existing test.

Mandy

...

Roger

On 1/30/2015 5:27 PM, Mandy Chung wrote:

...

On 1/30/15 2:25 PM, Roger Riggs wrote: > Please review this correction of a JapaneseEra range check in java.time. > The error was discovered during development of additional conformance tests (to be delivered separately). > > Webrev: > http://cr.openjdk.java.net/~rriggs//webrev-era-8068278 >

Looks fine to me. Is it easy to write a regression test to go along with this fix?

Mandy

> Issue: > <https://bugs.openjdk.java.net/browse/JDK-8068278> 8068278 ArrayIndexOutOfBoundsException instead of DateTimeException in j.t.chrono.JapaneseChronology.eraOf() > > Thanks, Roger >

<Mail Attachment.gif>

Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 Lance.Andersen@oracle.com

Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 Lance.Andersen@oracle.com