RFR: 8344942: Template-Based Testing Framework [v61]

[Emanuel Peter]

On Fri, 30 May 2025 07:16:35 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> Emanuel Peter has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - Merge branch 'JDK-8344942-TemplateFramework-v3' of https://github.com/eme64/jdk into JDK-8344942-TemplateFramework-v3
>>  - move verification
>
> test/hotspot/jtreg/compiler/lib/template_framework/Template.java line 539:
> 
>> 537:      * @param arg0Name The name of the (first) argument for hashtag replacement.
>> 538:      * @return A {@link Template} with one argument.
>> 539:      */
> 
> Just a general thought and not really something we can enforce by the framework, but we might want to mention here as well that the `arg0Name` string should match the lambda parameter for easier application and consistency? Theoretically (and not very clever), you can do that:
> 
>         var testTemplate = Template.make("a", "b", (Integer b, Integer a) -> body(
>             """
>             public class Foo {
>                 public static void main() {
>                     int a1 = #a;
>                     int b1 = #b;
>             """,
>             "int a2 = " + a + ";\n", // != a1, oops
>             "int b2 = " + b + ";\n", // != b1, oops
>             """
>                 }
>             }
>             """
>         ));
> 
> We could make the same remark in the two and three arg `make()` versions as well.

Sure. I added this line:
`Good practice but not enforced: {@code arg1Name}, {@code arg2Name}, and {@code arg3Name} should match the lambda argument names.`

> test/hotspot/jtreg/compiler/lib/template_framework/Template.java line 771:
> 
>> 769:         }
>> 770:         boolean mutable = mutability == DataName.Mutability.MUTABLE;
>> 771:         if (0 >= weight || weight > 1000) {
> 
> Could be more readable but up to you
> Suggestion:
> 
>         if (weight <= 0 || weight > 1000) {

Sure. I prefer going from small to large, so I'll do this: `if (weight <= 0 || 1000 < weight) {`

> test/hotspot/jtreg/compiler/lib/template_framework/Template.java line 814:
> 
>> 812:      */
>> 813:     static Token addStructuralName(String name, StructuralName.Type type, int weight) {
>> 814:         if (0 >= weight || weight > 1000) {
> 
> Could be more readable but up to you
> Suggestion:
> 
>         if (weight <= 0 || weight > 1000) {

Sure. I prefer going from small to large, so I'll do this: `if (weight <= 0 || 1000 < weight) {`

> test/hotspot/jtreg/testlibrary_tests/template_framework/tests/TestTemplate.java line 79:
> 
>> 77:         @Override
>> 78:         public boolean isSubtypeOf(DataName.Type other) {
>> 79:             return other instanceof MyPrimitive(String n) && n == name();
> 
> Is `==` wanted and not `equals()`?

Good catch! I suppose it does not really matter here in this example, but it would be better practice.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2118058874
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2118060523
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2118060526
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2118061219