RFR 9 8043477: java/lang/ProcessBuilder/Basic.java failed with: java.lang.AssertionError: Some tests failed

[roger riggs]

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.riggs at 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
>>>
>