[15] RFR: 8246261: TCKLocalTime.java failed due to "AssertionError: expected [18:14:22] but found [18:14:23]"

[Joe Wang]

Looks good.  Thanks for the analysis!

-Joe

On 6/1/2020 9:13 PM, naoto.sato at oracle.com wrote:
> Updated webrev: http://cr.openjdk.java.net/~naoto/8246261/webrev.01/
>
> Naoto
>
> On 6/1/20 6:30 PM, naoto.sato at oracle.com wrote:
>> Hi Joe,
>>
>> In fact, this bug was possibly revealed by the fix to 8242504, where 
>> the system clock precision is now nanoseconds. Before that, it used 
>> to be millisecond precision, so the first try for the exact match 
>> succeeded for most of the cases. Even with the nano precision fix, 
>> most of the cases the test exits with exact match in the loop. But 
>> you are right, exact match or not does not matter in this test case, 
>> so I think we can just eliminate these exact match try loops. I will 
>> remove them and do some sniff testing on it.
>>
>> Naoto
>>
>> On 6/1/20 5:58 PM, Joe Wang wrote:
>>> Hi Naoto,
>>>
>>> The patch looks good to fix the failure. I'm just curious whether 
>>> the 100-time comparison is necessary because of the existence of 
>>> this assertion outside the loop that allowed the test to pass if the 
>>> different was within a certain period of time. None of the tests had 
>>> commented on the purpose of the test,  it looks like it's testing 
>>> the assertion that (for the now method) "This will query the system 
>>> clock to obtain the current time." The 100-loop therefore was a 
>>> compromise for lack of a better way to prove that. I agree with what 
>>> you said that "inherently those two objects could have different 
>>> times". The outside-loop assertion  therefore makes better sense, 
>>> and the loop was kind of just wasting time to me (I mean you could 
>>> get lucky to have the two returning the same time down to a 
>>> nanosecond, but that didn't make the test better than just the 
>>> out-of-loop assertion.
>>>
>>> my 2 cents
>>>
>>> -Joe
>>>
>>> On 6/1/2020 12:31 PM, naoto.sato at oracle.com wrote:
>>>> Hello,
>>>>
>>>> Please review the fix to the following issue:
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8246261
>>>>
>>>> The proposed changeset is located at:
>>>>
>>>> https://cr.openjdk.java.net/~naoto/8246261/webrev.00/
>>>>
>>>> The test case compares two LocalTime objects, created with 
>>>> LocalTime.now(Clock/ZoneId). So inherently those two objects could 
>>>> have different times. The test tries to compare them 100 times for 
>>>> the exact match, and if not, then falls back to compare those times 
>>>> by truncating nanoseconds. The failure could occur when those two 
>>>> LocalTimes are around the whole second, e.g., expected == 
>>>> 18:14:22.999999 and test == 18:14:23.000001. To fix this, check the 
>>>> difference of those objects and ensure it is less than a second.
>>>>
>>>> Similar test cases exist in TCKLocalDateTime.java and 
>>>> TCKZonedDateTime.java so they should also be fixed. It is ok to 
>>>> leave the similar test case in TCKLocalDate.java, as multiple tries 
>>>> do exact match.
>>>>
>>>> Naoto
>>>