Looks good!
On Thu, Nov 13, 2014 at 8:47 AM, Martin Buchholz <marti...@google.com> 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)");
>
> ---
>
> 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
>
> ---
>
> 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.
>
> On Thu, Nov 13, 2014 at 7:37 AM, roger riggs <roger.ri...@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.ri...@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.
>>
>>
