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

[roger riggs]

Hi,

Thank you for you patience.

On 11/13/2014 11:47 AM, Martin Buchholz wrote:
> You should delete your import
>
>    36 import java.lang.IllegalStateException;
>
> ---
>
> I would bump up the time you're willing to wait here, by at least 10x.
>
> 2277             if ((end - start) > 200000000L * (AIX.is() ? 2 : 1))
> 2278                 fail("Test failed: waitFor took too long (" +
> (end - start) + "ns)");
ok, then the special case of the AIX bump isn't necessary
>
> ---
>
> When you are surely waiting, it's best to optimize by waiting as
> little as possible (although jtreg concurrency helps)
>
> 2301             p.waitFor(1, TimeUnit.SECONDS);
>
> I would set the timeout to only 10 millis here, and check that the
> operation took at least 10 millis and no more than your long timeout
> (you've settled on 7 seconds?)
> as in jsr166 tck tests
ok, I was thinking that too short and the spawned process would not have 
gotten started;
but that's not significant in the test.
>
> ---
>
> 2315                 exitValue = p.exitValue();
> 2316             } catch (IllegalThreadStateException ise) {
> 2317                 // no exitValue
> 2318             }
>
> If ITSE exception is thrown by exitValue, then the test should always
> fail but that is not currently the case.
> If the test does fail, it currently throws uninformative NPE in
> bjects.requireNonNull(exitValue), and we can do better.
The exitValue was intended to help with further debugging. 
Objects.toString was intended.

But it is as effective to leave the ITSE fall into the unexpected 
exception handler.

Webrev updated.

Thanks, Roger

>
> On Thu, Nov 13, 2014 at 7:37 AM, roger riggs <roger.riggs at oracle.com> wrote:
>> Please re-review,
>>
>> Corrected the webrev, and a few changes to consistently use the same
>> units, thanks for the review:
>>
>> The timeout time is extended to wait up to 7 seconds
>> and the initial waitFor should last at least 1 second.
>>
>> http://cr.openjdk.java.net/~rriggs/webrev-basic-debug-8043477/
>>
>> Roger
>>
>>
>>
>>
>> On 11/12/2014 10:42 PM, Martin Buchholz wrote:
>>> On Wed, Nov 12, 2014 at 6:25 PM, roger riggs <roger.riggs at oracle.com>
>>> wrote:
>>>> Hi Martin,
>>>>
>>>> I think that sleep is the version that spawns a JavaChild process and
>>>> has its own comment interpreter. See line 309: Am I getting the sleeps
>>>> mixed
>>>> up?
>>> Ah, good point - yes, you're right.  Although we could always bump up
>>> the sleep of the JavaChild from 10 if we wanted to - it should be
>>> comfortably bigger than any individual test case's need.
>>