RFR: 8346929: runtime/ClassUnload/DictionaryDependsTest.java fails with "Test failed: should be unloaded" [v2]

[Coleen Phillimore]

On Thu, 9 Jan 2025 03:16:05 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fixed @calvincheung comments.
>
> test/hotspot/jtreg/runtime/ClassUnload/KeepAliveClass.java line 75:
> 
>> 73:     ClassUnloadCommon.triggerUnloading();
>> 74: 
>> 75:     Set<String> aliveClasses = ClassUnloadCommon.triggerUnloading(List.of(className));
> 
> The call to `triggerUnloading` at line 73 is redundant because you don't check the result afterwards and the call here will repeat it.

oops missed one.

> test/hotspot/jtreg/runtime/ClassUnload/KeepAliveSoftReference.java line 40:
> 
>> 38: import jdk.test.lib.classloader.ClassUnloadCommon;
>> 39: import java.util.List;
>> 40: import java.util.Set;
> 
> This is unused in this test, but it may be better to check the returned set anyway rather than calling `wb.isClassAlive` at line 76.

It's trying to match if the weak reference is cleared and comparing to isAlive so I thought that part of the test is still useful.   Only Set is unused so I removed that.

> test/hotspot/jtreg/runtime/ClassUnload/SuperDependsTest.java line 80:
> 
>> 78:         SuperDependsTest d = new SuperDependsTest();
>> 79:         d.test();
>> 80:         System.out.println("Should unload MyTest and p2.c2 just now");
> 
> With the removal of line 78 the println is no longer relevant/correct/useful.

removed.

> test/hotspot/jtreg/runtime/ClassUnload/UnloadInterfaceTest.java line 93:
> 
>> 91: 
>> 92:         // Now unload className. This calls triggerUnloading but we only pass the class we expect to be unloaded
>> 93:         // otherwise the test will take too long.
> 
> Don't understand why this one needs a comment and none of the others do. ?? Also why the "otherwise" part?

I was trying to explain why interfaceName is not in the List.  I could have added it and tested that it's not unloaded but then the test would take too long.  It may be more correct to add it and make triggerUnloading try to unload it and fail to prove it cannot be unloaded.  But like I said in the comment, it would make the test take too long.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/22974#discussion_r1908795497
PR Review Comment: https://git.openjdk.org/jdk/pull/22974#discussion_r1908798705
PR Review Comment: https://git.openjdk.org/jdk/pull/22974#discussion_r1908799946
PR Review Comment: https://git.openjdk.org/jdk/pull/22974#discussion_r1908804028