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
