Hi Daniil,
I think you can remove "Ok" from the following message:
237 System.out.println("DEBUG: OK. Spawned java process terminated
with expected exit code of "
238 + STOP_PROCESS_EXIT_VAL);
It's somewhat misleading since the test can still fail. I think the
following is also misleading:
249 if (testFailed) {
250 output.reportDiagnosticSummary();
251 if (output.getStderr().contains("Port already in
use")) {
252 // Need to retry
253 return true;
254 }
255 }
The test can still pass after this happens, right? And I think there are
other "Test FAILURE" error message that can appear just before this.
Perhaps the code above should add a println that indicates that there
will be a retry attempt since the failure was due to the port being in use.
Otherwise looks good.
thanks,
Chris
On 7/17/19 1:30 PM, Alex Menkov wrote:
Hi Daniil,
The fix looks good in general.
Couple cosmetic notes (no new webrev required):
162 needRetry = runTest();
163 }
164 while (needRetry && (attempts++ < MAX_RETRY_ATTEMTS));
Please move "while" to the prev line:
163 } while (needRetry && (attempts++ < MAX_RETRY_ATTEMTS));
242 System.out.println("Test FAILURE on" + name +
" reason: The expected line \"" + READY_MSG
243 + "\" is not present in the process
output");
Please add space: "Test FAILURE on " + name
--alex
On 07/17/2019 12:46, Daniil Titov wrote:
Hi Chris,
It's a little unclear to me why you moved from ProcessThread to
TestProcessThread + Process. An explanation of that would make it
easier
to understand many of the changes.
There are two reasons for that:
1) For every network interface the test starts a separate thread
that runs the test for this interface. We want that if the test fails
due to the bind error the thread not to exit but try to repeat
the test several times. jdk.test.lib.thread.ProcessThread doesn't
allow this.
2) To filter out the cases when the test fails due to the bind error
we need to parse the process output. jdk.test.lib.thread.ProcessThread
registers its own pumps for stdin and sdtout (by calling
ProcessTools.startProcess()) that consume all output of the process
and prevent
the output analyzer from collecting this output.
Thanks!
--Daniil
On 7/17/19, 12:11 PM, "Chris Plummer" wrote:
Hi Daniil,
It's a little unclear to me why you moved from
ProcessThread to
TestProcessThread + Process. An explanation of that would make
it easier
to understand many of the changes.
thanks,
Chris
On 7/11/19 10:16 AM, Daniil Titov wrote:
> Please review the change that fixes an intermittent failure of
sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java
> test due to ports collision. The tests finds all network
interfaces and for every interface starts a separate process that tests
> the connection to JMX agent server for a specific
ports/interface combination.
>
> The test was changed to retry in case of the failure. If the
subprocess fails to bind and the number
> of retry attempts doesn't exceed the limit a new pair of
random ports is selected and the test is run again.
>
> Webrev: http://cr.openjdk.java.net/~dtitov/8221303/webrev.01/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8221303
>
> Thanks!
> --Daniil
>
>
