Hi Martin,
I updated the webrev to use long timeouts for the test in question; they
can't be too long
because the spawned sleep is only alive for 10 seconds.
Take a look please:
http://cr.openjdk.java.net/~rriggs/webrev-basic-debug-8043477/
Thanks, Roger
On 11/12/2014 4:45 PM, Martin Buchholz wrote:
Looking over the test again (I was the original author IIRC),
I think this test still has lots of value with a longer timeout.
Experience seems to show that 200ms is not a long enough timeout for
"system" operations to use in non-flaky tests, including everything in
Process handling.
I say we ncrease the timeout by at least 10x and move on.
On Wed, Nov 12, 2014 at 12:16 PM, roger riggs <roger.ri...@oracle.com> wrote:
Hi Martin, Rob,
I thought the point of the test was to verify the timeout logic in
waitFor(timeout).
Adding the prints without changing the test criteria was intended to gather
data
about the distribution.
A long timeout would still catch a failure in the reaper/waitFor
communication
so maybe that's ok.
As for printing the time, I would switch to the newer
java.time.Instant/Duration
which have a decent toString.
Roger
On 11/12/2014 3:02 PM, Rob McKenna wrote:
Eesh, Sorry Roger, I have something like this on my todo.
Martin, my concern with the long delay approach is that it effectively
nullifies the point of the test. Given that this test has been flakey the
approach has been to simply bump the acceptable delta by another 100ms or so
every time. Since we've had to do this repeatedly however, I'm beginning to
question whether the test is more trouble than its worth.
-Rob
On 12/11/14 19:44, Martin Buchholz wrote:
The print statement below seems redundant with the assertion failure
message.
You could improve the assertion message instead if need be.
Adding thousand separator underscores to 200000000L would help a little.
I like my little helper
static long millisElapsedSince(long startNanoTime) {
return NANOSECONDS.toMillis(System.nanoTime() - startNanoTime);
}
+ System.out.printf(" waitFor process: delta: %d%n",(end -
start) );
+
if ((end - start) > 200000000L * (AIX.is() ? 2 : 1))
fail("Test failed: waitFor took too long (" + (end -
start) + "ns)");
200 ms timeout for subprocesses to finish is just too damn low. In
j.u.c. tests we switched to 10 seconds for most "long" timeouts
(LONG_DELAY_MS) and are happy with the disappearance of rare flaky
results.
On Wed, Nov 12, 2014 at 11:34 AM, roger riggs <roger.ri...@oracle.com>
wrote:
Please review test changes to ProcessBuilder Basic.java to add some
debugging information.
The test has been failing intermittently. The wait times have been
extended
to see
if the systems are just slow. The failure criteria have not changed.
Suggestions welcome.
Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-basic-debug-8043477/
issue:
8043477: java/lang/ProcessBuilder/Basic.java failed with:
java.lang.AssertionError: Some tests failed
Thanks, Roger
[1] https://bugs.openjdk.java.net/browse/JDK-8043477
