On Wed, 17 Dec 2025 21:41:50 GMT, Chen Liang <[email protected]> wrote:
>> Refactor java/lang/invoke tests to use JUnit instead of TestNG.
>> This is done by:
>> 1. First a round of automatic conversion
>> 2. Simplify exception handling tests
>> 3. Replacing `assert` keyword and switching to better assertion APIs for
>> equality etc.
>> 4. Some other random cleanups, such as module status
>>
>> Testing: java/lang/invoke on Linux-x64. I un-problemlisted the updated
>> `java/lang/invoke/lambda/LambdaFileEncodingSerialization.java` too.
>
> Chen Liang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Some omissions
I think it would be good to change all the calls to `Assertions.*` to use
static imports instead. This PR changes quite a few calls to use the longer
form, but doesn't do so uniformly
test/jdk/java/lang/invoke/8147078/Test8147078.java line 37:
> 35: import org.junit.jupiter.api.Test;
> 36:
> 37: import static org.junit.jupiter.api.Assertions.assertThrows;
Copyright year on this file should be updated.
test/jdk/java/lang/invoke/AccessControlTest.java line 217:
> 215: boolean sameClass = (c1 == c2);
> 216: Assertions.assertTrue(samePackage || !sameTopLevel);
> 217: Assertions.assertTrue(sameTopLevel || !sameClass);
Suggestion:
assertTrue(samePackage || !sameTopLevel);
assertTrue(sameTopLevel || !sameClass);
test/jdk/java/lang/invoke/CompileThresholdBootstrapTest.java line 46:
> 44: public static void main(String ... args) throws Throwable {
> 45: CompileThresholdBootstrapTest test = new
> CompileThresholdBootstrapTest();
> 46: test.testBootstrap();
Does this main method even do anything? Looks like it's not being run.
test/jdk/java/lang/invoke/DefineClassTest.java line 211:
> 209:
> 210: Class<?> clazz =
> lookup.defineClass(generateClass("java.lang.Foo"));
> 211: assertEquals("java.lang.Foo", clazz.getName());
Looks like the parameter order was already reversed here.
test/jdk/java/lang/invoke/LoopCombinatorTest.java line 236:
> 234: assertEqualsFIXME(messageOrNull, iae.getMessage());
> 235: return;
> 236: }
Could you split this into separate positive and negative tests, with separate
sources?
test/jdk/java/lang/invoke/LoopCombinatorTest.java line 339:
> 337: assertEqualsFIXME(messageOrNull, iae.getMessage());
> 338: return;
> 339: }
Same here.
test/jdk/java/lang/invoke/MethodHandleProxies/Unnamed.java line 46:
> 44: // inaccessible interface
> 45: Method m = intf.getMethod("test");
> 46: assertFalse(m.canAccess(t));
This looks like a semantic change? Fixing an existing bug in the test?
test/jdk/java/lang/invoke/MethodHandles/TestDropReturn.java line 40:
> 38: import org.junit.jupiter.params.provider.MethodSource;
> 39:
> 40: @TestInstance(TestInstance.Lifecycle.PER_CLASS)
Why is this annotation needed here? There's only one test method.
test/jdk/java/lang/invoke/condy/CondyBSMInvocation.java line 241:
> 239: var e = Assertions.assertThrows(BootstrapMethodError.class,
> mh::invoke);
> 240: Throwable t = e.getCause();
> 241:
> Assertions.assertTrue(WrongMethodTypeException.class.isAssignableFrom(t.getClass()));
Suggestion:
assertInstanceOf(WrongMethodTypeException.class, e.getCause());
test/jdk/java/lang/invoke/condy/CondyWrongType.java line 117:
> 115: caught = caught.getCause();
> 116: assertNotNull(caught);
> 117:
> assertTrue(ClassCastException.class.isAssignableFrom(caught.getClass()));
Suggestion:
assertInstanceOf(ClassCastException.class, caught.getCause());
-------------
PR Review: https://git.openjdk.org/jdk/pull/28879#pullrequestreview-3590083827
PR Review Comment: https://git.openjdk.org/jdk/pull/28879#discussion_r2628995639
PR Review Comment: https://git.openjdk.org/jdk/pull/28879#discussion_r2628998893
PR Review Comment: https://git.openjdk.org/jdk/pull/28879#discussion_r2629006433
PR Review Comment: https://git.openjdk.org/jdk/pull/28879#discussion_r2629008843
PR Review Comment: https://git.openjdk.org/jdk/pull/28879#discussion_r2661620158
PR Review Comment: https://git.openjdk.org/jdk/pull/28879#discussion_r2661621257
PR Review Comment: https://git.openjdk.org/jdk/pull/28879#discussion_r2661675942
PR Review Comment: https://git.openjdk.org/jdk/pull/28879#discussion_r2661650953
PR Review Comment: https://git.openjdk.org/jdk/pull/28879#discussion_r2661933198
PR Review Comment: https://git.openjdk.org/jdk/pull/28879#discussion_r2661948489
