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

[Roger Riggs]

Hi,

ok,  (I still think of import is for code, not docs, but it makes the 
{@link}s easier).

Roger


On 12/14/2015 10:53 AM, Ramanand Patil wrote:
> RE: <i18n dev> Review request for JDK-8066982: ZonedDateTime.parse() 
> returns wrong ZoneOffset around DST fall transition
>
> Hi Roger,
>
> The added import in DateTimeFormatter.java  is because of the javadocs 
> entry - {@link ChronoLocalDateTime#atZone(ZoneId)}
> Regards,
> Ramanand.
>
> *From:*Roger Riggs
> *Sent:* Monday, December 14, 2015 8:36 PM
> *To:* Ramanand Patil; core-libs-dev at openjdk.java.net
> *Cc:* 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 Ramanand,
>
> Thanks for the cleanup of the test.
>
> On 12/14/2015 3:14 AM, Ramanand Patil wrote:
>
>     Hi Roger and 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
>
>
>
>
>
> 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: 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.
>