Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] GuavaTestLib RemoveIf Flag Inconsistencies #6076

Open
Speiger opened this issue Jun 10, 2022 · 3 comments
Open

[Bug] GuavaTestLib RemoveIf Flag Inconsistencies #6076

Speiger opened this issue Jun 10, 2022 · 3 comments

Comments

@Speiger
Copy link

Speiger commented Jun 10, 2022

I have been working on a Test Library that basically adds support for Primitive Variants of Guavas Test Lib.
And some of my implementations require to change how iterators are checked and I noticed when disabling that the removeIf tests get disabled too.

While looking through that class I noticed that tests check if "SUPPORTS_ITERATOR_REMOVE" flag is set, but if you have the "SUPPORTS_REMOVE" flag missing it tries to test if removeIf throws exceptions.

There is clearly some inconsistencies/bug in there.
https://github.com/google/guava/blob/master/guava-testlib/src/com/google/common/collect/testing/testers/CollectionRemoveIfTester.java

My suggested fix is make them all "SUPPORTS_REMOVE", since the iterator has nothing to do with the removeIf function.
While that there could be a case made for it, though at that point a dedicated flag for lambdas/functions could be made.

Would be nice if this could be addressed, because I kinda either need to copy your class just to fix it or not test removeIf.

Edit: ListListIteratorTester has also these issues, where a dedicated "ITERATOR_MODIFIABLE" would be nice and would simplify the Selection of Features.

@amalloy
Copy link
Contributor

amalloy commented Jun 10, 2022

@lowasser might have an opinion here - it looks like he originally broke out SUPPORTS_ITERATOR_REMOVE from SUPPORTS_REMOVE. What you say sounds plausible to me, but I'm not an expert on this library.

@chaoren
Copy link
Member

chaoren commented Jun 16, 2022

since the iterator has nothing to do with the removeIf function.

https://docs.oracle.com/javase/8/docs/api/java/util/Collection.html#removeIf-java.util.function.Predicate-

The default implementation traverses all elements of the collection using its iterator(). Each matching element is removed using Iterator.remove(). If the collection's iterator does not support removal then an UnsupportedOperationException will be thrown on the first matching element.

I guess technically some collection could override that default implementation, but I wouldn't say the iterator completely unrelated to removeIf.

@Speiger
Copy link
Author

Speiger commented Jun 17, 2022

I guess technically some collection could override that default implementation, but I wouldn't say the iterator completely unrelated to removeIf.

Oh yeah its not fully unrelated, but the relation level that was applied is way to high right now.

Small edit since I just thought of this:

if we go by that logic every single remove/indexOf/contains method should also contain some form of "Iterator" check because the base implementation uses iterators EVERYWHERE...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants