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

[naoto.sato at oracle.com]

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
>>