<i18n dev> Review request for JDK-8066982: ZonedDateTime.parse() returns wrong ZoneOffset around DST fall transition

Mon Dec 14 15:06:01 UTC 2015

Hi Ramanand,

Thanks for the cleanup of the test.

On 12/14/2015 3:14 AM, Ramanand Patil wrote:
> RE: <i18n dev> Review request for JDK-8066982: ZonedDateTime.parse() 
> returns wrong ZoneOffset around DST fall transition
>
> Hi Rogerand all,
>
> Please review the updated 
> Webrev:_http://cr.openjdk.java.net/~ntv/ramanand/8066982/webrev.02/_ 
> <http://cr.openjdk.java.net/%7Entv/ramanand/8066982/webrev.02/>
>
DateTimeFormatter.java has an added import that is unused and should be 
removed.

Looks fine.

Thanks,  Roger

> <http://cr.openjdk.java.net/%7Entv/ramanand/8066982/webrev.02/>
>
> Bug:_https://bugs.openjdk.java.net/browse/JDK-8066982_
>
> Roger, please see my commentsabout new tests:
>
> -All my test methodswere 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:_core-libs-dev at openjdk.java.net_<mailto:core-libs-dev at openjdk.java.net>
>
> Cc:_i18n-dev at openjdk.java.net_<mailto:i18n-dev at 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 
> <_ramanand.patil at oracle.com_<mailto:ramanand.patil at oracle.com>> wrote:
>
> >> Hi all,
>
> >>
>
> >> Please review the updated webrev:
>
> >>_http://cr.openjdk.java.net/~aefimov/8066982/webrev.01/_<http://cr.openjdk.java.net/%7Eaefimov/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:scolebourne at 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 
> <_ramanand.patil at oracle.com_<mailto:ramanand.patil at 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/_<http://cr.openjdk.java.net/%7Eaefimov/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.
>
>