On Wed, 22 Jan 2025 08:49:52 GMT, Matthias Baesken <mbaes...@openjdk.org> wrote:
>> test/jdk/sun/tools/jhsdb/JShellHeapDumpTest.java line 104:
>>
>>> 102: if (!res) { // try once more
>>> 103: launch(expectedMessage, Arrays.asList(toolArgs), false);
>>> 104: }
>>
>> `launch()` should take an allowRetry argument and only retry if true. This
>> test is operated in two different modes. One is with an active target
>> process and one is with a target process that should be stabilized and
>> inactive. The latter is not allowed to retry since it should never fail.
>> Also, it would help to add a comment as to why the retry is being done. See
>> the comment in `printStackTrace()`.
>>
>> I think a better approach would be for launch() to just return if a retry
>> should be done. testHeapDump() should check the result, and just immediately
>> return true if launch() returns true.
>
>> launch() should take an allowRetry argument and only retry if true.
>
> Are you talking about `public static void launch(String expectedMessage,
> String... toolArgs)` ?
Yes, but my suggestion on how to fix this might not be the best one. It's a bit
hard to get the naming and usage of flags and logic right here. allowRetry, vs
retry, vs doSleep. They all relate. Bottom line is that we should only ever
attempt a retry if doSleep is false. Right now that flag is checked in
printStackTraces(), and determines if true should be returned to allow a retry,
but only if we aren't already doing a retry. Something similar needs to be done
with launch() when it is deciding to return true or false after a failure.
Right now it is only checking the allowRetry flag, but it should also check the
doSleep flag.
I think in testHeapDump(), the calling of `launch(String expectedMessage,
String... toolArgs)` should be done in a way similar to calling
`printStackTraces()`. Pass in allowRetry and assign the result to the retry
boolean. If retry is true, then return it right away (back to main() so it can
retry). In `launch(String expectedMessage, String... toolArgs)`, only call
`launch(String expectedMessage, List<String> toolArgs, boolean allowRetry)`
once, passing in allowRetry flag. In `launch(String expectedMessage,
List<String> toolArgs, boolean allowRetry)`, only return true to allow a retry
if allowRetry is true and doSleep is false.
Anyway, it's kind of messy and there may be another way. The key issue with
your current version is that it always allows for a retry, but it should only
allow a retry if doSleep is false.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23213#discussion_r1925979789
