Looks good. Thanks for the analysis!
-Joe
On 6/1/2020 9:13 PM, 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
