RFR: 8343876: Enhancements to jpackage test lib

[Alexander Matveev]

On Sat, 9 Nov 2024 00:51:03 GMT, Alexey Semenyuk <asemenyuk at openjdk.org> wrote:

> Make jpackage test lib more practical. List of changes:
> 
> Support multiple args and var args in `@Parameter` annotation:
> 
> @Test
> @Parameter({"12", "foo"})
> @Parameter({"-89", "bar", "more"})
> @Parameter({"-89", "bar", "more", "moore"})
> public void testVarArg(int a, String b, String ... other) {}
> 
> 
> Full support for var args in test constructors.<br/>Better results when looking up the suitable ctor for the ctor args with `null`-s.<br/>Support multiple functions with `@Parameteres` annotation, all will be executed instead of the first one earlier:
> 
> class FooTest {
>     public FooTest(String... args) {}
>     public FooTest(int o) {}
>     public FooTest(int a, Boolean[] b, String c, String ... other) {}
> 
>     @Parameters
>     public static Collection<Object[]> input() {
>         return List.of(new Object[][] {
>             {},
>             {"str"},
>             {55, new Boolean[]{false, true, false}, "foo", "bar"},
>         });
>     }
> 
>     @Parameters
>     public static Collection<Object[]> input2() {
>         return List.of(new Object[][] {
>             {78},
>             {34, null, null},
>         });
>     }
> }
> 
> 
> Static test method will be executed only once and not as many times as the number of the test class instances.
> 
> Introduced `@ParameterSupplier` annotation as a flexible alternative to `@Parameter`:
> 
> 
> class FooTest {
>     @Test
>     @ParameterSupplier("dateSupplier")
>     @ParameterSupplier("AnotherClass.dateSupplier")
>     public void testDates(LocalDate v) {}
> 
>     public static Collection<Object[]> dateSupplier() {
>         return List.of(new Object[][] {
>             { LocalDate.parse("2018-05-05") },
>             { LocalDate.parse("2018-07-11") },
>         });
>     }
> }
> 
> class AnotherClass {
>     public static Collection<Object[]> dateSupplier() {
>         return List.of(new Object[][] {
>             { LocalDate.parse("2028-07-11") },
>         });
>     }
> }
> 
> 
> All annotations support `ifOS` and `ifNotOS` properties of type `jdk.internal.util.OperatingSystem`:
> 
> 
> @Test(ifOS = OperatingSystem.LINUX)
> public void testRunIfLinux() {}
> 
> @Test(ifNotOS = OperatingSystem.LINUX)
> public void testRunIfNotLinux() {}
> 
> @Test(ifNotOS = {OperatingSystem.LINUX,OperatingSystem.MACOS})
> public void testRunIfNotLinuxOrMacOS() {}
> 
> @Test
> @Parameter(value = "foo", ifOS = OperatingSystem.LINUX)
> @Parameter(value = {"foo", "bar"}, ifOS = { OperatingSystem.LINUX, OperatingSystem.MACOS })
> @Parameter(value = {}, ifNotOS = { OperatingSystem.WINDOWS })
> @ParameterSupplier(value = "getWindowsStrings", ifOS = OperatingSyst...

Looks good overall with some comments. I will do second review pass tomorrow.

test/jdk/tools/jpackage/helpers/jdk/jpackage/test/Comm.java line 2:

> 1: /*
> 2:  * Copyright (c) 2019, 2024, Oracle and/or its affiliates. All rights reserved.

If it is new code, then remove 2019.

test/jdk/tools/jpackage/helpers/jdk/jpackage/test/LinuxHelper.java line 441:

> 439:         }
> 440: 
> 441:         // Verify the value of `Exec` key in is escaped if required

This comment is confusing. Did you mean "Verify that the value of `Exec` key is escaped"?

test/jdk/tools/jpackage/helpers/jdk/jpackage/test/MethodCall.java line 157:

> 155:                 final var argValue = args[idx];
> 156:                 newArgs[idx] = Optional.ofNullable(argValue).map(Object::getClass).map(argType -> {
> 157:                     if(argType.isArray() && !paramType.isAssignableFrom(argType) ) {

`if(argType` -> `if (argType` and `(argType) )` -> `(argType))`.

test/jdk/tools/jpackage/helpers/jdk/jpackage/test/TestMethodSupplier.java line 142:

> 140:                 throw ex;
> 141:             }
> 142:         }

Maybe move this code into `switch` statement under `default`? Also, should we use `TypeStatus.UNKNOWN` instead of `null`?

test/jdk/tools/jpackage/helpers/jdk/jpackage/test/TestMethodSupplier.java line 433:

> 431:                     withTestAnnotations = true;
> 432:                 }
> 433:                 verifyAnnotationsCorrect(method);

This code blocks looks confusing. Why we need `method.setAccessible(true)` for all methods? Also, `if` statement looks confusing with empty `if (withTestAnnotations).`

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

PR Review: https://git.openjdk.org/jdk/pull/21996#pullrequestreview-2437415465
PR Review Comment: https://git.openjdk.org/jdk/pull/21996#discussion_r1843023671
PR Review Comment: https://git.openjdk.org/jdk/pull/21996#discussion_r1843028551
PR Review Comment: https://git.openjdk.org/jdk/pull/21996#discussion_r1843041707
PR Review Comment: https://git.openjdk.org/jdk/pull/21996#discussion_r1843068391
PR Review Comment: https://git.openjdk.org/jdk/pull/21996#discussion_r1843073100