[lworld] RFR: 8364321: Add JDI tests for value objects

[Chris Plummer]

On Tue, 29 Jul 2025 22:21:22 GMT, Alex Menkov <amenkov at openjdk.org> wrote:

> The fix adds sanity tests for JDI classes with value objects

test/jdk/com/sun/jdi/valhalla/FieldWatchpointsTest.java line 62:

> 60:     }
> 61: 
> 62:     static Value staticField = new Value(1);

It would be useful to also have this test take care of testing JDI with a flattened field. You can add a Value instance field to FieldWatchpointsTarg and see if you get events when Value.v is accessed.

test/jdk/com/sun/jdi/valhalla/ValueArrayReferenceTest.java line 110:

> 108:         List<Value> values = array.getValues();
> 109:         // Mirror.toString reports object type and reference id,
> 110:         // it should be enough to see if objects are different.

This is clever. When looking at `arraysEquals()` I was going to suggest that you iterate over the two arrays and compare each array element using `==` or `ObjectReference.equals()`, but I think your comment is right and what you are doing should work just as well.

test/jdk/com/sun/jdi/valhalla/ValueArrayReferenceTest.java line 147:

> 145:         for (ArrayReference arr1: arrays) {
> 146:             for (ArrayReference arr2: arrays) {
> 147:                 if (!arraysEquals(arr1, arr2)) {

This has some duplicate checks. For example, first you compare the 1st and 2nd arrays and later you compare the 2nd and 1st arrays. An alternate approach is a single loop that compares 1st and 2nd, then 2nd and 3rd, and then 3rd and 4th.  No need to compare any of the other combinations.

test/jdk/com/sun/jdi/valhalla/ValueClassTypeTest.java line 45:

> 43: class ValueClassTypeTarg {
> 44:     static value class Value {
> 45:         static Value staticField = new Value(1);

I'm not sure how adding Value.staticField provides any useful additional testing coverage that you don't already get from testing ValueClassTypeTarg.staticField.

test/jdk/com/sun/jdi/valhalla/ValueClassTypeTest.java line 59:

> 57:     }
> 58: 
> 59:     static Value staticField = new Value(2);

Using the same field name here makes the test code below somewhat confusing. I'd suggest using different names.

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

PR Review Comment: https://git.openjdk.org/valhalla/pull/1519#discussion_r2244024679
PR Review Comment: https://git.openjdk.org/valhalla/pull/1519#discussion_r2244048455
PR Review Comment: https://git.openjdk.org/valhalla/pull/1519#discussion_r2244033108
PR Review Comment: https://git.openjdk.org/valhalla/pull/1519#discussion_r2243593617
PR Review Comment: https://git.openjdk.org/valhalla/pull/1519#discussion_r2243591651