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

[Stephen Colebourne]

The added text and tests represent a spec clarification. The behaviour
in this area was not precisely defined before. The bug itself was
clear due to the expected round-trip behaviour of toString() and
parse(). While this is a slightly more tricky backport than some, it
is really quite broken in one of the most important areas for a
date-time library, the DST overlap.

IMO, the two extra spec <li> elements probably should not be
backported. However the rest should be. I'm not sure whether the "TCK"
test classes are still used as the TCK, so it is possible that the
backport should have tests located in a different place.

Hope that helps
Stephen



On 11 December 2015 at 20:07, Roger Riggs <Roger.Riggs at oracle.com> wrote:
> 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>
>> 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: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>
>>> 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.
>
>