Hi David,
On 10/27/2016 9:00 PM, David Holmes wrote:
But this is the second waitFor call after the process is destroyed.
Sorry I don't really see the point.
The tests were added to determine if waitFor(timeout) was handling the
timeout parameter correctly.
The 2nd test here was to check the datapath when the process already
been destroyed.
The 'extra' waitFor() ensures the process is gone.
But you are correct, this exercises a path through the waitFor code that
does not even look
at the timeout value so I'll remove the entire 2nd case.
Webrev updated in place.
http://cr.openjdk.java.net/~rriggs/webrev-basic-destroy-8168517/
Okay, but now you don't need the p.waitFor() after the destroy() at all.
Hmmm ... I guess either we want the original code with a longer
timeout to accommodate slow/loaded platforms (to test that waitFor(n)
after a destroy() is timely); or everything after destroy() can be
deleted.
Here's another approach to test whether waitFor(timeout) is behaving as
expected.
The test should be checking that waitFor does not wait longer than
requested.
So shorten the timeout to a very short time (10 milliseconds) and check that
waitFor returns in no more than that time(plus epsilon). It is similar
to the first test
in that section but the process is exiting.
http://cr.openjdk.java.net/~rriggs/webrev-basic-destroy-8168517/
Thanks, Roger
Cheers,
David
Thanks, Roger
David
Roger
David
I contemplated increasing the timeout but given the issue is system
loading
I didn't have a good idea what to raise it to. 30sec, 1 min, 2 min.
and the harness timeout is 2min.
Roger
David
Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-basic-destroy-8168517/
Thanks, Roger
