On Thu, 1 Oct 2026 02:33:33 GMT, Stuart Marks <[email protected]> wrote:
>> 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 o... Hi @stuart-marks , here's where we've landed on your high-level questions. Framework vs. test responsibility. It's the framework's job. Tier 6 runs the valhalla_adopted group with the value class plugin, which also turns on --enable-preview. So a class annotated @AsValueClass runs as an identity class in normal tiers and as a value class in tier 6, without the test doing anything twice. That's a short-term setup while JEP 401 is in preview. Once value classes are a permanent feature, we'll update the tests to cover identity and value classes directly. Also, Integer, Boolean and the other box classes are value classes under preview, so existing Integer-based tests already push value objects through the collections whenever they run in preview mode. Generifying the tests. Dropped. Given the above, there's no need to generify the collections tests to run every path with both Integer and a value class. "Elusive coverage." You were right. In most of these tests the collection only stores, compares or iterates elements, which doesn't depend on whether the element is a value object. The underlying operations (==, hashCode, equals, assignment, arrays, synchronized, references, serialization) are already covered at the language/VM level under test/jdk/valhalla/valuetypes, java/lang/Object, test/hotspot/jtreg/runtime/valhalla and java/io/Serializable/valueObjects. So I've reverted all the value class variants except two, where the java.util code itself does something value-specific: **Collections/Ser**: a value class that isn't a record can't be serialized without a writeReplace/readResolve proxy. This test checks that the serialized forms of singleton/nCopies still work with a value-class element, which the java/io serialization tests don't cover. **HashMap/PutNullKey**: reworked so its value-class key isn't Comparable. Every insert into the tree bin then goes through TreeNode.tieBreakOrder, which uses System.identityHashCode. The test also looks up and removes every key around the null-key insertion. As far as I can tell, no existing test reaches that path, even with identity keys. ------------- PR Comment: https://git.openjdk.org/jdk/pull/32201#issuecomment-6084787293
