Copilot commented on code in PR #681:
URL:
https://github.com/apache/commons-collections/pull/681#discussion_r3409933696
##########
src/test/java/org/apache/commons/collections4/map/ConcurrentHashMapSanityTest.java:
##########
@@ -42,6 +42,11 @@ public boolean isAllowNullValuePut() {
return false;
}
+ @Override
+ public boolean isEntrySetAddSupported() {
+ return true;
+ }
Review Comment:
`ConcurrentHashMap.entrySet()` in the JDK does not support `add(...)` (it
throws `UnsupportedOperationException`). Returning `true` here will cause the
inherited entrySet view tests to exercise `add` and fail against the JDK
contract.
##########
src/test/java/org/apache/commons/collections4/list/AbstractListTest.java:
##########
@@ -131,11 +131,11 @@ public void verify() {
}
}
- public class ListIteratorTest extends AbstractListIteratorTest<E> {
+ public abstract class ListIteratorTest extends AbstractListIteratorTest<E>
{
Review Comment:
`ListIteratorTest` is now `abstract` and not annotated with `@Nested`, so
the list-iterator contract tests inherited from `AbstractListIteratorTest`
won't execute under JUnit 5. If these iterator-view tests are meant to be
restored, this needs to be a concrete nested test class.
##########
src/test/java/org/apache/commons/collections4/map/AbstractMapTest.java:
##########
@@ -400,7 +401,7 @@ public void verify() {
// to the confirmed, that the already-constructed collection views
// are still equal to the confirmed's collection views.
- public class MapValuesTest extends AbstractCollectionTest<V> {
+ public abstract class MapValuesTest extends AbstractCollectionTest<V> {
Review Comment:
`MapValuesTest` is now `abstract` and not annotated with `@Nested`, so its
inherited `@Test` methods from `AbstractCollectionTest` will never run under
JUnit 5. To actually restore values-view coverage, this needs to be a concrete
`@Nested` class (or a concrete `@Nested` wrapper subclass).
##########
src/test/java/org/apache/commons/collections4/iterators/AbstractMapIteratorTest.java:
##########
@@ -137,7 +137,12 @@ void testEmptyMapIterator() {
}
} else {
// setValue() should throw an IllegalStateException
- assertThrows(IllegalStateException.class, () ->
it.setValue(addSetValues()[0]));
+ try {
+ it.setValue(addSetValues()[0]);
+ fail();
+ } catch (final UnsupportedOperationException |
IllegalStateException ex) {
+ // ignore
+ }
}
Review Comment:
In `testEmptyMapIterator()`, the `supportsSetValue()` == true branch
currently accepts `UnsupportedOperationException`, which would let an iterator
that claims to support `setValue()` silently pass even if it doesn't. For
supported iterators, calling `setValue()` before `next()` should fail with
`IllegalStateException` (consistent with the rest of this test suite and the
Iterator contract).
##########
src/test/java/org/apache/commons/collections4/map/AbstractMapTest.java:
##########
@@ -138,7 +139,7 @@
*/
public abstract class AbstractMapTest<M extends Map<K, V>, K, V> extends
AbstractObjectTest {
- public class MapEntrySetTest extends AbstractSetTest<Map.Entry<K, V>> {
+ public abstract class MapEntrySetTest extends AbstractSetTest<Map.Entry<K,
V>> {
Review Comment:
`MapEntrySetTest` contains `@Test` methods, but it is now `abstract` and not
annotated with `@Nested`, so JUnit 5 will not execute these view tests. If the
goal is to restore the dropped nested tests, make this a concrete `@Nested`
class (or add a concrete `@Nested` subclass wrapper).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
