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; [email protected]
Cc: [email protected]
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/
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: HYPERLINK
"mailto:[email protected]"[email protected]
Cc: HYPERLINK "mailto:[email protected]"[email protected]
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:[email protected]"[email protected]> wrote:
>> Hi all,
>>
>> Please review the updated webrev:
>> HYPERLINK
>> "http://cr.openjdk.java.net/%7Eaefimov/8066982/webrev.01/"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:[email protected]]
>> 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:[email protected]"[email protected]> 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.
