date:20151214

Re: Review request for JDK-8066982: ZonedDateTime.parse() returns wrong ZoneOffset around DST fall transition

2015-12-14 Thread Ramanand Patil

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-...@openjdk.java.net"; 
core-libs-...@openjdk.java.net
Cc:  HYPERLINK "mailto:i18n-dev@openjdk.java.net"; i18n-dev@openjdk.java.net
Subject: Re:  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:  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/~ae

Re: Review request for JDK-8066982: ZonedDateTime.parse() returns wrong ZoneOffset around DST fall transition

2015-12-14 Thread 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  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  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 
>> 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:  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 
>>> 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.
>
>

Re: Review request for JDK-8066982: ZonedDateTime.parse() returns wrong ZoneOffset around DST fall transition

2015-12-14 Thread Stephen Colebourne

I remain happy with the webrev
Stephen

On 14 December 2015 at 08:14, Ramanand Patil  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-...@openjdk.java.net"; 
> core-libs-...@openjdk.java.net
> Cc:  HYPERLINK "mailto:i18n-dev@openjdk.java.net"; i18n-dev@openjdk.java.net
> Subject: Re:  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:  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

Review Request: JDK-8139572 SimpleDateFormat parse month stand-alone format bug

2015-12-14 Thread Naveen Kumar


Hi All,
   Please review the following fix for jdk9.

Bug: https://bugs.openjdk.java.net/browse/JDK-8139572
webrev: http://cr.openjdk.java.net/~okutsu/naveen/8139572/webrev.00/

issue description:
SimpleDateFormat with "stand-alone" month format " " and locate 
ru_RU throw java.text.ParseExceptio when parse the valid date string 
"Сентябрь 2015".


Fix:
Implemented parsing for "stand-alone" month format pattern in 
jdk/src/java.base/share/classes/java/text/SimpleDateFormat.java



regards
Naveen Kumar

Re: Review Request: JDK-8139572 SimpleDateFormat parse month stand-alone format bug

2015-12-14 Thread Masayoshi Okutsu


Looks good to me.

Masayoshi

On 12/14/2015 6:29 PM, Naveen Kumar wrote:

Hi All,
   Please review the following fix for jdk9.

Bug: https://bugs.openjdk.java.net/browse/JDK-8139572
webrev: http://cr.openjdk.java.net/~okutsu/naveen/8139572/webrev.00/

issue description:
SimpleDateFormat with "stand-alone" month format " " and 
locate ru_RU throw java.text.ParseExceptio when parse the valid date 
string "Сентябрь 2015".


Fix:
Implemented parsing for "stand-alone" month format pattern in 
jdk/src/java.base/share/classes/java/text/SimpleDateFormat.java



regards
Naveen Kumar

Re: Review request for JDK-8066982: ZonedDateTime.parse() returns wrong ZoneOffset around DST fall transition

2015-12-14 Thread Roger Riggs

Hi Ramanand,

Thanks for the cleanup of the test.

On 12/14/2015 3:14 AM, Ramanand Patil wrote:
RE:  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/_ 

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 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-...@openjdk.java.net_

Cc:_i18n-...@openjdk.java.net_

Subject: Re:  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@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@joda.org_]

>> Sent: Wednesday, December 09, 2015 4:46 PM

>> To: core-libs-dev

>> Cc: i18n-dev

>> Subject: Re:  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

>

Re: Review request for JDK-8066982: ZonedDateTime.parse() returns wrong ZoneOffset around DST fall transition

2015-12-14 Thread Ramanand Patil

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-...@openjdk.java.net
Cc: i18n-dev@openjdk.java.net
Subject: Re:  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:core-libs-...@openjdk.java.net"core-libs-...@openjdk.java.net

Cc: HYPERLINK "mailto:i18n-dev@openjdk.java.net"i18n-dev@openjdk.java.net

Subject: Re:  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  "mailto:ramanand.pa...@oracle.com"ramanand.pa...@oracle.com> 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:scolebou...@joda.org]

>> Sent: Wednesday, December 09, 2015 4:46 PM

>> To: core-libs-dev

>> Cc: i18n-dev

>> Subject: Re:  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 te

Re: Review request for JDK-8066982: ZonedDateTime.parse() returns wrong ZoneOffset around DST fall transition

2015-12-14 Thread 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:  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-...@openjdk.java.net
*Cc:* i18n-dev@openjdk.java.net
*Subject:* Re:  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: core-libs-...@openjdk.java.net 

Cc: i18n-dev@openjdk.java.net 

Subject: Re:  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 > 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@joda.org_]

>> Sent: Wednesday, December 09, 2015 4:46 PM

>> To: core-libs-dev

>> Cc: i18n-dev

>> Subject: Re:  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)

>

Re: Review request for JDK-8066982: ZonedDateTime.parse() returns wrong ZoneOffset around DST fall transition

Re: Review request for JDK-8066982: ZonedDateTime.parse() returns wrong ZoneOffset around DST fall transition

Re: Review request for JDK-8066982: ZonedDateTime.parse() returns wrong ZoneOffset around DST fall transition

Review Request: JDK-8139572 SimpleDateFormat parse month stand-alone format bug

Re: Review Request: JDK-8139572 SimpleDateFormat parse month stand-alone format bug

Re: Review request for JDK-8066982: ZonedDateTime.parse() returns wrong ZoneOffset around DST fall transition

Re: Review request for JDK-8066982: ZonedDateTime.parse() returns wrong ZoneOffset around DST fall transition

Re: Review request for JDK-8066982: ZonedDateTime.parse() returns wrong ZoneOffset around DST fall transition

8 matches

Site Navigation

Mail list logo

Footer information