RFR: 8324641: [IR Framework] Add Setup method to provide custom arguments and set fields [v6]

[Christian Hagedorn]

On Mon, 5 Feb 2024 14:45:22 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> **Bigger goal**
>> 
>> I am tired of always writing IR tests where I have to write elaborate code in the `@Run` method to create inputs, and then run the test method to create `gold` output values in (hopefully) interpreter mode, and then later verify the results of the compiled method.
>> 
>> Hence, I now introduce the "Setup" method, which can create custom argument values.
>> In a later RFE, I will implement automatic result verification, which implicitly intercepts the inputs and outputs of the test method, and compares the behaviour of the interpreter and the compiled code. That way, the pattern will be:
>> 
>> 
>> @Setup
>> ... specify your arguments and fields ...
>> 
>> -> intercept arguments, cache them
>> 
>> @Test
>> ... write a test with arbitrary inputs and outputs...
>> 
>> -> interpreter mode: intercept outputs and cache them (i.e. gold values)
>> -> compiled mode: intercept outputs and compare them to the gold values.
>> 
>> (optional)
>> @Check
>> .. do custom verification...
>> 
>> 
>> **In this RFE: the Setup Method**
>> 
>> The first step is the setup method.
>> 
>> Example:
>> https://github.com/openjdk/jdk/blob/08670a5d45b1c45e954e2dc85eb8c92e11e48fb2/test/hotspot/jtreg/testlibrary_tests/ir_framework/examples/SetupExample.java#L49-L87
>
> Emanuel Peter has updated the pull request incrementally with one additional commit since the last revision:
> 
>   fix more comments

Thanks for the updates. I will have a closer look again at the README tomorrow but the code changes look good.

What's missing are some good and bad tests that use the new features. `SetupExample.java` already provides some good working examples and maybe that's sufficient. Maybe you can double check if it's worth to add some more good (i.e. non-failing) tests.

The bad tests can be added to `TestBadFormat` which should include things like a `@Setup` test without a corresponding `@Test` method, mixing `setup` and `value` etc. Can you add some of these bad tests?

test/hotspot/jtreg/compiler/lib/ir_framework/README.md line 37:

> 35: 
> 36: ### 2.1 Different Tests
> 37: There are two ways a test can be formulated, depending on how much control is needed over the test invocation.

Suggestion:

There are two ways a test can be written, depending on how much control is needed over the test invocation.

test/hotspot/jtreg/compiler/lib/ir_framework/Setup.java line 32:

> 30:  * This annotation is used to identify Setup methods. These can be used to compute arbitrary arguments for a test
> 31:  * method (see {@link Test}), as well as to set field values. A test method can use a setup method, by specifying it in a
> 32:  * {@link Arguments} annotation. A setup method can optionally take a {@link SetupInfo} as an argument. The

Wrapping was off after the suggestions:
Suggestion:

 * method (see {@link Test}), as well as to set field values. A test method can use a setup method, by specifying
 * it in a {@link Arguments} annotation. A setup method can optionally take a {@link SetupInfo} as an argument. The

test/hotspot/jtreg/compiler/lib/ir_framework/Setup.java line 35:

> 33:  * arguments for the test methods are returned as a new object array.
> 34:  *
> 35:  * Examples on how to use test methods can be found in {@link ir_framework.examples.SetupExample} and also as part of the internal testing in the package {@link ir_framework.tests}.

I suggest to wrap this long line.
Suggestion:

 * Examples on how to use test methods can be found in {@link ir_framework.examples.SetupExample} and also as part of the 
 * internal testing in the package {@link ir_framework.tests}.

test/hotspot/jtreg/compiler/lib/ir_framework/test/ArgumentsProvider.java line 61:

> 59: class ArgumentsProviderBuilder {
> 60:    public static ArgumentsProvider build(Method method,
> 61:                                           HashMap<String, Method> setupMethodMap) {

Suggestion:

                                         HashMap<String, Method> setupMethodMap) {

test/hotspot/jtreg/compiler/lib/ir_framework/test/ArgumentsProvider.java line 70:

> 68:         String setupMethodName = argumentsAnnotation.setup();
> 69: 
> 70:         if (setupMethodName.length() > 0) {

I suggest to use `isEmpty()` instead:
Suggestion:

        if (!setupMethodName.isEmpty()) {

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

PR Review: https://git.openjdk.org/jdk/pull/17557#pullrequestreview-1863097021
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1478439370
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1478434370
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1478432962
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1478435501
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1478437934