I remain happy with the webrev Stephen
On 14 December 2015 at 08:14, Ramanand Patil <ramanand.pa...@oracle.com> wrote: > Hi Roger and all, > > Please review the updated Webrev: > http://cr.openjdk.java.net/~ntv/ramanand/8066982/webrev.02/ > > Bug: https://bugs.openjdk.java.net/browse/JDK-8066982 > > > Roger, please see my comments about new tests: > > - All my test methods were earlier generic with main method and hence had > private static qualifier. I have changed them now to match and to be > consistent with the existing tests. Thank you for pointing this. > > - I agree with you on this. Particularly when we have tests around DST we > can’t avoid timezone data. > > - I have defined dataProvider for every method and reduced the test data for > cases where zone is not present(testWithoutZoneWithoutOffset() and > testWithOffsetWithoutZone()). > But for the other 2 cases where zone is present(testWithZoneWithOffset() and > testWithZoneWithoutOffset()), I feel most of the data cases are necessary and > some are required to be on safer side if in future the timezone rule changes. > Also, this bug fix mainly affects these cases. > I have created the dataProvider kepping in mind the below cases for 2 DST > zones. > 1. Time before overlap > 2. Time during Overlap > 3. Time after overlap > 4. Valid Offsets for each of these times > 5. Zero Offset for each time > 6. Few Positive and negative invalid offsets for each time > > > Regards, > Ramanand. > > > -----Original Message----- > From: Roger Riggs > Sent: Saturday, December 12, 2015 1:37 AM > To: HYPERLINK "mailto:core-libs-dev@openjdk.java.net"; > core-libs-dev@openjdk.java.net > Cc: HYPERLINK "mailto:i18n-...@openjdk.java.net"; i18n-...@openjdk.java.net > Subject: Re: <i18n dev> Review request for JDK-8066982: ZonedDateTime.parse() > returns wrong ZoneOffset around DST fall transition > > Hi, > > Stephen, can you confirm that the added text and test in DateTimeFormatter > is not a specification change? > Our processes have a bit more to do if it is a spec change and it would limit > the backport to JDK 8. > > This bug fix will cause an existing TCK/JCK test to fail but that is > considered also a bug and is fixed. > test/java/time/tck/java/time/TCKZonedDateTime.java > > Ramanand, some comments on the new test: > - The 'private' qualifier on the tests and data providers is not used in > the current tests and > is not consistently present in the new one. > Since it has little/no function, the tests would be a bit cleaner > without it. > > - Typically, test that have data dependencies (such as the timezone > data) cannot be used for > compatibility to the specification, but the data is stable and it seems > unavoidable in this case. > > - Are all of the data cases necessary to validate the specification? > Redundant cases extend the testing time without adding more confidence to > the quality. > > Thanks, Roger > > > On 12/10/2015 11:00 AM, Stephen Colebourne wrote: >> I believe this is suitable for committing, thanks, other reviews welcome! >> Stephen >> >> >> >> On 10 December 2015 at 15:36, Ramanand Patil < HYPERLINK >> "mailto:ramanand.pa...@oracle.com"; ramanand.pa...@oracle.com> wrote: >>> Hi all, >>> >>> Please review the updated webrev: >>> http://cr.openjdk.java.net/~aefimov/8066982/webrev.01/ >>> >>> I have modified the fix and test cases as per inputs given by Stephen. >>> Also, I have added the javadocs changes in this patch which were proposed >>> in the bug. >>> >>> Bug link is: https://bugs.openjdk.java.net/browse/JDK-8066982 >>> >>> >>> Regards, >>> Ramanand. >>> >>> -----Original Message----- >>> From: Stephen Colebourne [mailto:scolebou...@joda.org] >>> Sent: Wednesday, December 09, 2015 4:46 PM >>> To: core-libs-dev >>> Cc: i18n-dev >>> Subject: Re: <i18n dev> Review request for JDK-8066982: >>> ZonedDateTime.parse() returns wrong ZoneOffset around DST fall >>> transition >>> >>> The logic looks fine. >>> >>> In the main code, this part >>> .getLong(INSTANT_SECONDS); >>> can be replaced with >>> .toEpochSecond(); >>> which will be slightly faster. >>> >>> In the test case, this part >>> .plus(15, ChronoUnit.MINUTES); >>> can be replaced with >>> .plusMinutes(15) >>> >>> And >>> .with(ChronoField.OFFSET_SECONDS, >>> ZoneOffset.of(offsetSamples[j]).getTotalSeconds()) >>> can be replaced with >>> .with(ZoneOffset.of(offsetSamples[j])) >>> >>> In addition to the looping tests, I'd like to see the examples from the bug >>> report as test cases. Those tests would be simple to follow and explain, >>> whereas the looping tests are a little hard to follow. >>> >>> thanks for fixing this >>> Stephen >>> >>> >>> >>> On 9 December 2015 at 07:44, Ramanand Patil < HYPERLINK >>> "mailto:ramanand.pa...@oracle.com"; ramanand.pa...@oracle.com> wrote: >>>> HI all, >>>> >>>> >>>> >>>> Please review a fix for Bug - HYPERLINK >>>> "https://bugs.openjdk.java.net/browse/JDK-8066982"JDK-8066982 >>>> >>>> >>>> >>>> Bug - Parsing a string with ZonedDateTime.parse() that contains zone >>>> offset and zone ID "Europe/Berlin" returns a wrong ZonedDateAndTime >>>> (different offset). This error starts exactly at the transition time >>>> (included) and ends one hour later (excluded). >>>> >>>> >>>> >>>> Webrev - http://cr.openjdk.java.net/~aefimov/8066982/webrev.00/ >>>> >>>> >>>> >>>> One existing test case in TCKZonedDateTime.java is also modified, because >>>> - when offset is invalid the local time is changed to make the result >>>> valid. >>>> >>>> >>>> >>>> >>>> >>>> Regards, >>>> >>>> Ramanand. >
