testlib: Add ConcurrentMapSpliteratorTester for spliterator characteristics #8173
Conversation
…istics Add tests to verify that ConcurrentMap implementations return spliterators with the CONCURRENT characteristic on their entrySet(), keySet(), and values() views. Also verifies that no spliterator reports both CONCURRENT and SIZED characteristics (which are mutually incompatible per the Java specification). Fixes google#7855
| Spliterator<?> spliterator = getMap().values().spliterator(); | ||
| assertTrue( | ||
| "values().spliterator() should have CONCURRENT characteristic", | ||
| spliterator.hasCharacteristics(Spliterator.CONCURRENT)); |
There was a problem hiding this comment.
I think we can move the testEntrySetSpliteratorNotConcurrentAndSized, testKeySetSpliteratorNotConcurrentAndSized, and testValuesSpliteratorNotConcurrentAndSized tests to CollectionSpliteratorTester instead.
Per https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/Spliterator.html#CONCURRENT,
A top-level Spliterator should not report both
CONCURRENTandSIZED
A top-level Spliterator should not report bothCONCURRENTandIMMUTABLE
and that should apply to all top-level Spliterators, not just concurrent ones.
Also, https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/Spliterator.html#SUBSIZED says
A Spliterator that does not report
SIZEDas required bySUBSIZEDis inconsistent
We can check that as well.
Per review feedback — the spec invariants apply to all spliterators, not just ConcurrentMap ones, so they belong in the common tester. Added checks for all four spec-mandated invariants: - CONCURRENT + SIZED must not coexist - CONCURRENT + IMMUTABLE must not coexist - SUBSIZED requires SIZED - SORTED requires ORDERED Stripped ConcurrentMapSpliteratorTester down to just the CONCURRENT-must-exist assertions and dropped the reflection methods to match the other ConcurrentMap*Tester classes.
| * Tests that if the spliterator reports {@code SORTED}, it also reports {@code ORDERED}, as | ||
| * required by the {@link Spliterator} specification. | ||
| */ | ||
| public void testSpliteratorSortedRequiresOrdered() { |
There was a problem hiding this comment.
Good catch! Thanks for adding this as well.
| ConcurrentMapRemoveTester.class, | ||
| ConcurrentMapReplaceTester.class, | ||
| ConcurrentMapReplaceEntryTester.class); | ||
| ConcurrentMapReplaceEntryTester.class, |
There was a problem hiding this comment.
Instead of adding a new tester here, we can maybe override the
createDerivedEntrySetSuitecreateDerivedKeySetSuitecreateDerivedValueCollectionSuite
methods to append a ConcurrentCollectionSpliteratorTester, so it could be reused for arbitrary concurrent collections.
Instead of adding ConcurrentMapSpliteratorTester to the map tester list, use the derived suite architecture as @chaoren suggested: - Add ConcurrentCollectionSpliteratorTester that tests any collection's spliterator for the CONCURRENT characteristic - Add ConcurrentSetTestSuiteBuilder and ConcurrentCollectionTestSuiteBuilder that include this tester - Override createDerivedEntrySetSuite/KeySetSuite/ValueCollectionSuite in ConcurrentMapTestSuiteBuilder to use the concurrent builders - Delete ConcurrentMapSpliteratorTester (no longer needed) This approach is more composable - the tester can be reused for any concurrent collection, not just ConcurrentMap views.
3 participants
Summary
ConcurrentMapSpliteratorTesterto verify spliterator characteristics onConcurrentMapviewsentrySet(),keySet(), andvalues()spliterators have theCONCURRENTcharacteristicCONCURRENTandSIZEDcharacteristics (mutually incompatible per Java spec)Motivation
Per issue #7855 and maintainer guidance from @chaoren:
ConcurrentMapimplementations should return spliterators with theCONCURRENTcharacteristicCONCURRENTandSIZEDCurrently, some Guava
ConcurrentMapimplementations (e.g.,LocalCache,MapMakerInternalMap) return spliterators that:CONCURRENT(should have it)SIZED(should not have it for concurrent collections)This PR adds tests to document the expected behavior. A follow-up PR can fix the implementations.
Testing
ConcurrentMap*Testerpattern in testlibConcurrentHashMapcorrectly returnsCONCURRENT=true, SIZED=falseRelated Issues
Fixes #7855