The change looks OK also with the loop removed. > On Jun 2, 2020, at 12:13 AM, naoto.s...@oracle.com wrote: > > Updated webrev: http://cr.openjdk.java.net/~naoto/8246261/webrev.01/ > > Naoto > > On 6/1/20 6:30 PM, naoto.s...@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.s...@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 >>>
<http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>
