RFR: 8356811: Some nsk/jdi tests can fetch ThreadReference from static field in the debuggee: part 4

[Chris Plummer]

On Tue, 13 May 2025 21:55:00 GMT, Alex Menkov <amenkov at 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