On Tue, 13 May 2025 21:55:00 GMT, Alex Menkov <amen...@openjdk.org> wrote:
>> This batch of changes mostly concerns the remaining uses of threadByName()
>> and converting them to use threadByFieldNameOrThrow() or the new
>> threadByFieldName(). The latter is used if the caller has code to handle a
>> null result. The former is when an exception is needed to get the test to
>> terminate properly. I did fix a few long standing cases where
>> threadyByName() was being called and not checking the result. These call
>> sites now use threadByFieldNameOrThrow() instead of threadByFieldName().
>>
>> Note there is a minor semantic change in doing this. threadByName() has some
>> extra code to check that the named thread is only found once, and will throw
>> an exception if it is. I think this was just some extra checking that was
>> being done during test development, and is not needed for proper test
>> execution. I've run all the tests without this check and they still pass. I
>> plan on removing this check at some point.
>>
>> Tested by running all tier5 svc tests, which includes the nsk/jdi tests.
>> Also ran tier1 and ran locally.
>
> test/hotspot/jtreg/vmTestbase/nsk/jdi/StackFrame/setValue/setvalue005/setvalue005.java
> line 165:
>
>> 163: debuggee.threadByFieldName(rType, "mainThread",
>> DEBUGGEE_THRDNAME);
>> 164: if (thrRef == null) {
>> 165: log.complain("TEST FAILURE: method Debugee.threadByName()
>> returned null for debuggee thread "
>
> Need to update error message
> BTW `debuggee.classByName` also can throw exceptions, need to catch and call
> `quitDebuggee()` (this applicable to other changes in the fix when
> `debuggee.classByName` is called outside of try block)
This is a common pattern in my changes. I'l need to add something like the
following catch:
} catch (Exception e) {
e.printStackTrace();
log.complain("TEST FAILURE: caught unexpected exception: " + e);
tot_res = Consts.TEST_FAILED;
}
It would have been a lot cleaner if these tests were all written to just let
the exception happen and not worry about printing messages about what it was
doing when the exception was thrown. The exception stack trace provides all the
info you need.
The error handling in these tests is frustrating to work with. It's
inconsistent and incomplete. Many catch exceptions, print a failure message
(and no exception backtrace), and them resume at the next step, only to hang as
a result.
I was tempted to get rid of a lot of the failure handling when thrRef == null
and just use threadByFieldNameOrThrow() instead of threadByFieldName(), but
also wanted to limit the scope of the changes. I did observe that in general
this works well. While making the changes for this PR I dealt with a lot of
failures due to calling threadByFieldNameOrThrow() with bad args, and the test
always failed quickly and gracefully. But in the end I changed to
threadByFieldName() to keep things a bit more consistent.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25190#discussion_r2087727193
