On Fri, 28 Aug 2026 17:30:49 GMT, Bill Huang <[email protected]> wrote:
>> Changed Tests: >> >> HashMap/KeySetRemove.java: equal ValueClass keys mapped to null can be >> removed from HashMap and TreeMap. >> >> HashMap/NullKeyAtResize.java: null-key resize behavior remains correct when >> surrounding keys are ValueClass instances. >> >> HashMap/PutNullKey.java: colliding comparable tree-bin keys are >> value-capable via @AsValueClass. >> >> HashMap/ReplaceExisting.java: replacing an existing ValueClass key during >> active iteration does not corrupt the iterator. >> >> HashMap/SetValue.java: Map.Entry.setValue() returns the old ValueClass value. >> >> HashMap/ToArray.java: toArray() coverage is added for ValueClass keys, >> values, and set elements across hash and linked variants. >> >> HashMap/TreeBinAssert.java: tree-bin iterator-removal coverage now uses a >> value-capable key. >> >> Hashtable/EqualsCast.java: Provider/Hashtable equality now includes matching >> ValueClass key/value entries. >> >> Hashtable/SimpleSerialization.java: serialization round-trip now also covers >> Hashtable<Integer,Integer>. >> >> LinkedHashMap/ComputeIfAbsentAccessOrder.java: access-order behavior is >> repeated with Integer keys. >> >> LinkedHashMap/EmptyMapIterator.java: fail-fast iterator behavior is repeated >> with Integer key/value entries. >> >> LinkedList/AddAll.java: append-order behavior is repeated with ValueClass >> elements. >> >> LinkedList/Clone.java: clone/equality checks for LinkedList, TreeSet, and >> TreeMap subclasses now include ValueClass contents. >> >> Unchanged Tests: >> >> HashMap/HashMapCloneLeak.java: unchanged because the test depends on >> WeakReference reachability. Value objects are not valid weak-reference >> targets, so adding VClass would not match the regression being tested. >> >> HashMap/OverrideIsEmpty.java: unchanged because the test is about HashMap >> subclass method dispatch. Value classes cannot extend HashMap, and changing >> only the key/value payload would not add meaningful value-class coverage. >> >> HashMap/WhiteBoxResizeTest.java: unchanged because it is a >> white-box/internal capacity and table-sizing test using >> reflection/VarHandles over HashMap, LinkedHashMap, HashSet, and WeakHashMap >> internals. Its purpose is sizing/lazy allocation/resize arithmetic, not >> key/value equality or value-object behavior. >> >> HashMap/ToString.java: unchanged because it specifically verifies that >> HashMap.Entry.toString() does not throw when the map contains null keys or >> values. Adding value-class keys or values would not extend the original >> null-handling regression in a meaningful ... > > Bill Huang has updated the pull request incrementally with one additional > commit since the last revision: > > Implemented review comments I have an overall low-level comment and some overall high-level comments. The low-level comment is just about the code itself. For the moment, let's accept the premise of updating these tests to test collections instances that contain not only Integer but also instances of VClass. To accomplish that, it seems to me the best approach is to generify the test methods and parameterize them with things like factories and such and then call the test methods with Integer and with VClass, along with additional stuff like factories. What you have does that partially but it needs to go further. I've commented on the first four files in the PR, all in test/jdk/java/util/Collections: * AddAll.java * BigBinarySearch.java * BinarySearchNullComparator.java * CheckedListReplaceAll.java This is fairly intensive work. However, if you compare my versions with the pre-JEP401 versions, the diffs are mostly focused on generifying the test logic to enable it to be invoked with Integer and VClass, while preserving the logic and overall structure of the test (for the most part). If we were to proceed down this path, this is what I'd like to see. It might make sense in some cases to start over from the pre-JEP401 version and generify it directly, instead of trying to remove the duplication that was added in the intermediate steps. Now to the high level comments. I'm not sure that we actually want to proceed down this path. Previously, you said that there were some enhancements to the test framework or infrastructure that runs the tests twice, once with VClass as a value class and once as an identity class. Is that the long term direction for the JDK regression suite, or is it just a stopgap while JEP 401 is in Preview? Another way of looking at this is that, assuming we want coverage of both identity and value classes here, is this the responsibility of the test itself, or will we rely on the framework to run the tests in both modes? If individual tests are responsible, then each test needs to invoke its test logic once with a value class and once with an identity class. In this case it's not clear what VClass and its value/identity switch is useful for. The tests could just invoke the test logic with Integer and String. When Preview mode is enabled, Integer becomes a value class, so we have coverage. If, on the other hand, the framework is responsible, then the individual tests don't need to invoke all the logic twice. The tests can be rewritten to use VClass and rely on the framework to invoke in identity mode and again in value mode. In that case we can get rid of the Integer paths, we don't need to generify everything, and we just rewrite all the tests in terms of VClass. Either way, it may behoove us to pause the generification of all the tests until we've figured out what the longer term strategy is (or until somebody tells me what it is, because I don't know). Finally, there is the premise that updating dozens of collections tests to use both value and identity classes will provide useful "coverage". I think this is questionable. Consider the CheckedListReplaceAll test. This is a regression test for a bug, and the bug was that an older version of CheckedList failed to perform a typecheck on the return value of the operator passed to replaceAll(). You can see the change at line 3500 of this commit: https://github.com/openjdk/jdk/commit/6c860a781a7da9ca6702de17d82bb230467bf4eb (There are other a few other changes there but they're not really relevant.) The new code hardly performs any operation on the contents of the collections at all: it compares the object to null, it passes the object to the `isInstance` method of a class object, and it returns the object from the method. None of these operations have anything to do with collections. In fact, the only things collections do with any object is call `Object` methods and also do fundamental stuff like: * methods of `Object` * compare against null * compare against other reference * `instanceof` and `isInstance` * assign to and read from field * pass as argument * return value from method Surely there are tests elsewhere in the test suite that test that these fundamental operations perform as expected with both value classes and identity classes. Can we not rely on them? In that light it doesn't really make sense to me to change dozens of collections tests in order to chase some elusive "coverage" goal of value classes. test/jdk/java/util/Collections/AddAll.java line 103: > 101: } > 102: } > 103: This is a good first step, but I think we can improve things by generifying the range creation and passing in a factory method for creating values, that is, one thing to create Integer values and another thing to create VClass values. (Note that this uses a static factory method VClass::of which does the fairly obvious thing of setting `x` to the int argument and `arr` to an array consisting of the int argument.) The whole thing looks like this: public class AddAll { static final int N = 100; public static void main(String[] args) { test(new ArrayList<>(), Integer.class, Integer::valueOf); test(new LinkedList<>(), Integer.class, Integer::valueOf); test(new HashSet<>(), Integer.class, Integer::valueOf); test(new LinkedHashSet<>(), Integer.class, Integer::valueOf); test(new ArrayList<>(), VClass.class, VClass::of); test(new LinkedList<>(), VClass.class, VClass::of); test(new HashSet<>(), VClass.class, VClass::of); test(new LinkedHashSet<>(), VClass.class, VClass::of); } private static Random rnd = new Random(); static <T> void test(Collection<T> c, Class<T> clazz, IntFunction<T> valueFactory) { int x = 0; for (int i = 0; i < N; i++) { int rangeLen = rnd.nextInt(10); if (Collections.addAll(c, range(x, x + rangeLen, clazz, valueFactory)) != (rangeLen != 0)) throw new RuntimeException("" + rangeLen); x += rangeLen; } if (c instanceof List) { if (!c.equals(Arrays.asList(range(0, x, clazz, valueFactory)))) throw new RuntimeException(x +": "+c); } else { if (!c.equals(new HashSet<>(Arrays.asList(range(0, x, clazz, valueFactory))))) throw new RuntimeException(x +": "+c); } } private static <T> T[] range(int from, int to, Class<T> clazz, IntFunction<T> valueFactory) { T[] result = (T[]) Array.newInstance(clazz, to - from); for (int i = from, j=0; i < to; i++, j++) result[j] = valueFactory.apply(i); return result; } } If you look at a diff between the pre-JEP401 version and this version, it's a pretty straightforward conversion of the test logic that uses use Integer values to logic that uses values of T. Of course each test itself need to be invoked twice, once for Integer and once for VClass, for the different kind of collections. I tried a generalizing this to have a method that covers the four collections of type T and then to invoke this method once with Integer and once with VClass (and corresponding factories). This added another layer but it didn't make anything shorter, so it didn't seem worthwhile. Anyway, this is sort of thing I'd expect to see if we're going to generalize the tests to use both Integer and VClass. test/jdk/java/util/Collections/BigBinarySearch.java line 121: > 119: vl.set(n - 2, new VClass(n - 2, new int[] { n - 2 })); > 120: vl.set(n - 1, new VClass(n - 1, new int[] { n - 1 })); > 121: equal(n - 1, Collections.binarySearch(vl, new VClass(n - 1, new > int[] { n - 1 }))); The generification of SparseList was done well. The main part of the test in the VClass case seems to diverge from the Integer case though. It doesn't look like it's testing the same things, and it doesn't test the reversed and double-reversed comparator cases. I pushed harder on generifying the `realMain` method (changing it to a `test` method, since the new `realMain` needs to invoke it for Integer and VClass), and I came up with this: private static <T extends Comparable<T>> void test(IntFunction<T> valueFactory, UnaryOperator<T> negate) throws Throwable { final int n = (1<<30) + 47; List<T> big = new SparseList<>(valueFactory.apply(0)); big.set( 0, valueFactory.apply(-44)); big.set( 1, valueFactory.apply(-43)); big.set(n-2, valueFactory.apply( 43)); big.set(n-1, valueFactory.apply( 44)); int[] ints = { 0, 1, n-2, n-1 }; Comparator<T> reverse = Collections.reverseOrder(); Comparator<T> natural = Collections.reverseOrder(reverse); for (int i : ints) { checkBinarySearch(big, i); checkBinarySearch(big, i, null); checkBinarySearch(big, i, natural); } for (int i : ints) big.set(i, negate.apply(big.get(i))); for (int i : ints) checkBinarySearch(big, i, reverse); } private static void realMain(String[] args) throws Throwable { System.out.println("binarySearch(List<Integer>, Integer)"); test(Integer::valueOf, i -> -i); System.out.println("binarySearch(SparseList<VClass>, VClass)"); test(VClass::of, v -> VClass.of(-v.x)); } We need to pass in a factory for values, but we also need to pass in a "negate" operator. The original Integer test negates the values, which has the effect of reversing the sort order of the array, so that the reversed comparator can be tested. With the VClass::of static factory method it's pretty easy to write a lambda that negates a VClass value. test/jdk/java/util/Collections/BinarySearchNullComparator.java line 42: > 40: test(Arrays.asList(new VClass(1, new int[] { 1 }), new VClass(2, > new int[] { 2 }), new VClass(3, new int[] { 3 })), > 41: new VClass(3, new int[] { 3 }), 2); > 42: } Generifying the `test` method worked out well. Not much else going on here, but I'll note that it's much easier to see what's going on if the array constructors are replaced with calls to the VClass::of static factory method: test(Arrays.asList(VClass.of(1), VClass.of(2), VClass.of(3)), VClass.of(3), 2); test/jdk/java/util/Collections/CheckedListReplaceAll.java line 72: > 70: thwarted.printStackTrace(System.out); > 71: System.out.println("Curses! Foiled again!"); > 72: } A few things are going on in this test. The first is that the original test actually tested TWO things in a single method: (1) that CheckedList::replaceAll can't insert an object of the wrong type, and (2) that CheckedList::replaceAll throws NPE if given a null argument. (The fix for (1) introduced the problem that (2) tests, so it kind of makes sense that these tests are in the same file.) The "(1) wrong type" test needs to cover both Integer and VClass, whereas the "(2) null argument" test has nothing to do with the type of the contents. So it's probably best to separate these into two test methods and generify only the "wrong type" method. The `testReplaceAllWrongType` method is a decent attempt at extracting commonality but it requires a fair amount of setup. Making it generic (except for the type-unsafe UnaryOperator), and parameterizing the error message, makes it a bit easier to invoke. The code at lines 55-58 seems to introduce a new test case, which looks like a functional test of List::replaceAll or possibly CheckedList::replaceAll. This should be tested elsewhere. This specific test is a regression test for a bug, which is a missing type check on the return value of the call to the operator passed to replaceAll(). If this isn't tested elsewhere, it should be added in the right place, not here. In addition this new case covers only VClass and not Integer, so it introduces an asymmetry. Here's my alternative: public class CheckedListReplaceAll { public static void main(String[] args) { checkBad(Integer.class, new Integer[] { 1, 2, 3 }, e -> (((int) e) % 2 != 0) ? e : "evil"); checkBad(VClass.class, new VClass[] { VClass.of(1), VClass.of(2), VClass.of(3) }, e -> (((VClass) e).x % 2 != 0) ? e : "evil"); checkNull(); } static <T> void checkBad(Class<T> clazz, T[] array, UnaryOperator /*raw*/ evil) { List unwrapped = Arrays.asList(array); List<Object> wrapped = Collections.checkedList(unwrapped, clazz); try { wrapped.replaceAll(evil); System.out.printf("Bwahaha! I have defeated you! %s\n", wrapped); throw new RuntimeException("String added to checked List<" + clazz.getName() + ">"); } catch (ClassCastException thwarted) { thwarted.printStackTrace(System.out); System.out.println("Curses! Foiled again!"); } } static void checkNull() { List<Integer> unwrapped = Arrays.asList(new Integer[]{}); // Empty list List<Integer> wrapped = Collections.checkedList(unwrapped, Integer.class); try { wrapped.replaceAll((UnaryOperator)null); System.out.printf("Bwahaha! I have defeated you! %s\n", wrapped); throw new RuntimeException("NPE not thrown when passed a null operator"); } catch (NullPointerException thwarted) { thwarted.printStackTrace(System.out); System.out.println("Curses! Foiled again!"); } } } ------------- PR Review: https://git.openjdk.org/jdk/pull/32201#pullrequestreview-5373429896 PR Review Comment: https://git.openjdk.org/jdk/pull/32201#discussion_r4150541114 PR Review Comment: https://git.openjdk.org/jdk/pull/32201#discussion_r4150581822 PR Review Comment: https://git.openjdk.org/jdk/pull/32201#discussion_r4150598778 PR Review Comment: https://git.openjdk.org/jdk/pull/32201#discussion_r4150690724
