Hi Peter,
Love those pathological edge cases...
On 9/10/2013 12:18 PM, Peter Levart wrote:
Hi Roger,
Sorry to be persistent, but if the LocalTime.now() returns local time
for a time zone that is not the default time zone (which is an error
in java.time implementation that I assume the test is trying to catch)
then the diff can be a constant > 15 minutes and the loop will roll
forever.
Forever is a long time but the framework will timeout and fail the test
before then.
I still don't quite get what the test is trying to test, but if it is
testing whether LocalTime.now() is returning the local time for
default time zone and to prove that, it compares the result with
LocalTime.now(Clock.systemDefaultZone()), then the trick to get the
right behaviour is as follows:
When taking the following three consecutive samples (on a preloaded
time zone data):
before = LocalTime.now(Clock.systemDefaultZone());
test = LocalTime.now();
after = LocalTime.now(Clock.systemDefaultZone());
It can happen that a local time jump occurs between samples 'before'
and 'test' or between samples 'test' and 'after' or never, but it can
not occur at both times. So if you take:
If we are really pathological then the time can jump more than once in a
short period of time.
Who knows what NTP will provide; it has been known to be wrong/faulty.
Math.min(
Math.abs(before.toNanoOfDay() - test.toNanoOfDay()),
Math.abs(test.toNanoOfDay() - after.toNanoOfDay())
)
... it should be < 0.1 s always if LocalTime.now() is returning local
time for default time zone
What do you think?
Compact but it discards some perfectly good measurement
because it is inconvenient to compute (the wrap at midnight cases).
And it is inefficient because it tests the function twice even if it
does not need to.
I updated the test to retry once if it failed the success criteria.
This should work in the case of a single clock change within the test
window.
Thanks, Roger
Regards, Peter
On 09/10/2013 04:08 PM, roger riggs wrote:
Hi Peter,
Point taken about the edge cases, I'm not sure it will occur in practice
but I updated the test to retry if the time changes by more than 15
minutes.
There are likely to be other existing tests that do not taken into
account
DST changes but it is not a high priority now to find and fix them.
http://cr.openjdk.java.net/~rriggs/webrev-localtime-now-8023639/
Thanks, Roger
On 9/10/2013 2:43 AM, Peter Levart wrote:
On 09/09/2013 09:42 PM, roger riggs wrote:
Hi Peter,
Right, max doesn't solve the issue but I'm not keen on a test that
retries
until it gets a better answer.
Hi Roger,
If java.time logic is correct, it should only ever retry once when
roll-over or DST jump-back happens, so the test could be made to
fail if it tries to retry the 2nd time, indicating unexpected
behaviour. The "jumps" in LocalTime should be very far-apart so the
test should only encounter one of them, if any.
Adding nanosPerDay if the difference comes out negative would adjust
for the crossing of midnight and not require looping on a more complex
test condition.
That's ok for midnight roll-over, but what about DST jumps? They
only happen two times a year, so you expect the test will never
encounter them?
Regards, Peter
The longish delay in the now() method is due to first-time
initialization
that reads the timezone data file. Introducing the loop it would
change
the test condition so that it is not testing the 'cold' startup.
However, the purpose of the test in not to measure the
initialization overhead
so adding an extra sampling of now(Clock) before the test will
remove the first time
initialization.
Updated webrev at:
http://cr.openjdk.java.net/~rriggs/webrev-localtime-now-8023639/
Thanks, Roger
On 9/9/2013 11:14 AM, Peter Levart wrote:
On 09/09/2013 03:12 PM, roger riggs wrote:
Hi Peter,
The possible wrap-around caused by crossing midnight is handled
by Math.max
so a retry is not needed.
Math.abs(test.toNanoOfDay() - expected.toNanoOfDay())
Hi Roger,
In case there is a wrap-around, the 'diff' is much more than
500,000,000 ns (about 24*60*60*1,000,000,000 ns - delay), which
fails the test.
But what do you think about testing before <= test <= after ? It
should not be timing dependent, like it is now. Does it test the
same thing?
Regards, Peter
Thanks, Roger
On 9/9/2013 2:14 AM, Peter Levart wrote:
On 09/06/2013 07:58 PM, roger riggs wrote:
Please review for two corrections:
- The java/time/tck/java/time/TCKLocalTime test failed on a
slow machine;
the test should be more lenient. The test is not
appropriate for a conformance test
and is moved to java/time/test/java/time/TestLocalTime.
- The javadoc for the JapaneseEra.MEIJI era should indicate the
start date is 1868-01-01
to be consistent with java.util.Calendar. Note that
java.time does not permit dates before Meiji 6
to be created since the calendar is not clearly defined until
then.
Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-localtime-now-8023639/
Thanks, Roger
Hi Roger,
Although very in-probable, the test can fail when 'expected' is
sampled before and 'test' is sampled after midnight. I'm
guessing the test is trying to prove that LocalTime.now() is
equivalent to LocalTime.now(Clock.systemDefaultZone()), right?
In that case, what about the following:
public void now() {
LocalTime before, test, after;
do {
before = LocalTime.now(Clock.systemDefaultZone());
test = LocalTime.now();
after = LocalTime.now(Clock.systemDefaultZone());
// retry in case the samples were obtained around
midnight
} while (before.compareTo(after) > 0);
assertTrue(before.compareTo(test) <= 0 &&
test.compareTo(after) <= 0);
}
Regards, Peter
