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
>>>>
>>
>
