Please review two corrections for java.time

[Peter Levart]

Hi Roger,

Sorry to be persistent, but if the LocalTime.now() returns local time 
for a time zone that is not the default time zone (which is an error in 
java.time implementation that I assume the test is trying to catch) then 
the diff can be a constant > 15 minutes and the loop will roll forever.

I still don't quite get what the test is trying to test, but if it is 
testing whether LocalTime.now() is returning the local time for default 
time zone and to prove that, it compares the result with 
LocalTime.now(Clock.systemDefaultZone()), then the trick to get the 
right behaviour is as follows:

When taking the following three consecutive samples (on a preloaded time 
zone data):

before = LocalTime.now(Clock.systemDefaultZone());
test = LocalTime.now();
after = LocalTime.now(Clock.systemDefaultZone());

It can happen that a local time jump occurs between samples 'before' and 
'test' or between samples 'test' and 'after' or never, but it can not 
occur at both times. So if you take:

Math.min(
     Math.abs(before.toNanoOfDay() - test.toNanoOfDay()),
     Math.abs(test.toNanoOfDay() - after.toNanoOfDay())
)

... it should be < 0.1 s always if LocalTime.now() is returning local 
time for default time zone

What do you think?

Regards, Peter

On 09/10/2013 04:08 PM, roger riggs wrote:
> Hi Peter,
>
> Point taken about the edge cases, I'm not sure it will occur in practice
> but I updated the test to retry if the time changes by more than 15 
> minutes.
> There are likely to be other existing tests that do not taken into account
> DST changes but it is not a high priority now to find and fix them.
>
> http://cr.openjdk.java.net/~rriggs/webrev-localtime-now-8023639/
>
> Thanks, Roger
>
> On 9/10/2013 2:43 AM, Peter Levart wrote:
>> On 09/09/2013 09:42 PM, roger riggs wrote:
>>> Hi Peter,
>>>
>>> Right, max doesn't solve the issue but I'm not keen on a test that 
>>> retries
>>> until it gets a better answer.
>>
>> Hi Roger,
>>
>> If java.time logic is correct, it should only ever retry once when 
>> roll-over or DST jump-back happens, so the test could be made to fail 
>> if it tries to retry the 2nd time, indicating unexpected behaviour. 
>> The "jumps" in LocalTime should be very far-apart so the test should 
>> only encounter one of them, if any.
>>
>>>
>>> Adding nanosPerDay if the difference comes out negative would adjust
>>> for the crossing of midnight and not require looping on a more complex
>>> test condition.
>>
>> That's ok for midnight roll-over, but what about DST jumps? They only 
>> happen two times a year, so you expect the test will never encounter 
>> them?
>>
>> Regards, Peter
>>
>>>
>>> The longish delay in the now() method is due to first-time 
>>> initialization
>>> that reads the timezone data file.  Introducing the loop it would change
>>> the test condition so that it is not testing the 'cold' startup.
>>> However, the purpose of the test in not to measure the 
>>> initialization overhead
>>> so  adding an extra sampling of now(Clock) before the test will 
>>> remove the first time
>>> initialization.
>>>
>>> Updated webrev at:
>>> http://cr.openjdk.java.net/~rriggs/webrev-localtime-now-8023639/
>>>
>>> Thanks, Roger
>>>
>>>
>>> On 9/9/2013 11:14 AM, Peter Levart wrote:
>>>>
>>>> On 09/09/2013 03:12 PM, roger riggs wrote:
>>>>> Hi Peter,
>>>>>
>>>>> The possible wrap-around caused by crossing midnight is handled by 
>>>>> Math.max
>>>>> so a retry is not needed.
>>>>>
>>>>> Math.abs(test.toNanoOfDay() - expected.toNanoOfDay())
>>>>
>>>> Hi Roger,
>>>>
>>>> In case there is a wrap-around, the 'diff' is much more than 
>>>> 500,000,000 ns (about 24*60*60*1,000,000,000 ns - delay), which 
>>>> fails the test.
>>>>
>>>> But what do you think about testing before <= test <= after ? It 
>>>> should not be timing dependent, like it is now. Does it test the 
>>>> same thing?
>>>>
>>>> Regards, Peter
>>>>
>>>>>
>>>>> Thanks, Roger
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> On 9/9/2013 2:14 AM, Peter Levart wrote:
>>>>>> On 09/06/2013 07:58 PM, roger riggs wrote:
>>>>>>> Please review for two corrections:
>>>>>>>
>>>>>>> -  The java/time/tck/java/time/TCKLocalTime test failed on a 
>>>>>>> slow machine;
>>>>>>>     the test should be more lenient.   The test is not 
>>>>>>> appropriate for a conformance test
>>>>>>>     and is moved to java/time/test/java/time/TestLocalTime.
>>>>>>>
>>>>>>> - The javadoc for the JapaneseEra.MEIJI era should indicate the 
>>>>>>> start date is 1868-01-01
>>>>>>>   to be consistent with java.util.Calendar.  Note that java.time 
>>>>>>> does not permit dates before Meiji 6
>>>>>>>   to be created since the calendar is not clearly defined until 
>>>>>>> then.
>>>>>>>
>>>>>>> Webrev: 
>>>>>>> http://cr.openjdk.java.net/~rriggs/webrev-localtime-now-8023639/
>>>>>>>
>>>>>>> Thanks, Roger
>>>>>>>
>>>>>>>
>>>>>> Hi Roger,
>>>>>>
>>>>>> Although very in-probable, the test can fail when 'expected' is 
>>>>>> sampled before and 'test' is sampled after midnight. I'm guessing 
>>>>>> the test is trying to prove that LocalTime.now() is equivalent to 
>>>>>> LocalTime.now(Clock.systemDefaultZone()), right?
>>>>>>
>>>>>> In that case, what about the following:
>>>>>>
>>>>>>     public void now() {
>>>>>>         LocalTime before, test, after;
>>>>>>         do {
>>>>>>             before = LocalTime.now(Clock.systemDefaultZone());
>>>>>>             test = LocalTime.now();
>>>>>>             after = LocalTime.now(Clock.systemDefaultZone());
>>>>>>           // retry in case the samples were obtained around midnight
>>>>>>         } while (before.compareTo(after) > 0);
>>>>>>
>>>>>>         assertTrue(before.compareTo(test) <= 0 && 
>>>>>> test.compareTo(after) <= 0);
>>>>>>     }
>>>>>>
>>>>>>
>>>>>> Regards, Peter
>>>>>>
>>>>>
>>>>
>>>
>>
>