Hi Roger, Thank you. Ship it.
Best Lance On Feb 3, 2015, at 11:59 AM, Roger Riggs <roger.ri...@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.ri...@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.ander...@oracle.com >> >> >> > Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
