Add Java 10 Collectors APIs #9860
Conversation
user/test/com/google/gwt/emultest/java10/util/stream/CollectorsTest.java
Outdated
Show resolved
Hide resolved
user/super/com/google/gwt/emul/java/util/stream/Collectors.java
Outdated
Show resolved
Hide resolved
| * Tests for java.util.stream.Collectors Java 10 API emulation. | ||
| */ | ||
| public class CollectorsTest extends EmulTestBase { | ||
| private static <T> boolean unmodifiableCollection(Collection<T> c, T existingSample, T newSample) { |
There was a problem hiding this comment.
It misses a lot of cases. Maybe we should refactor a bit in a follow up commit that introduces a helper class to check immutability of collections/sets, lists, maps. We now have many tests that need it: CollectionsTest, ListTest, SetTest, MapTest, CollectorsTest. Each doing it a bit differently and not covering everything.
There was a problem hiding this comment.
Good call, I'll file a followup. At least for the List-verification method, the simplest answer might be to make CollectionsTest.doTestModificationsToList public and static, and lean on it from here.
Note that I didn't work especially hard on fully validating every method, since the emulation depends on existing code that I assumed was well tested (or at least verified in real use) - at some point we have to assume that the levels below us are roughly sane...
There was a problem hiding this comment.
at some point we have to assume that the levels below us are roughly sane
Yes, I had the thought that it might be good to just make the emulated immutable collection classes public and then simply do an instanceof check. If Collections.unmodifiableList/List.of and friends all use the same implementations, that would be fine I guess. Then we could make one test for the immutable implementations and all other tests check if that implementation has been used.
| } | ||
|
|
||
| public void testToUnmodifiableList() { | ||
| applyItems(List.of("a", "b"), toUnmodifiableList(), "a", "b", (a, b) -> { |
There was a problem hiding this comment.
All these tests are fine but a bit difficult to review/read. applyItems isn't a great name as it also executes tests using the provided equals function. I had to lookup what applyItems actually does. Also using (a, b) was a bit distracting as a and b are used as values as well. Using expected and actual would probably make more sense. Then we also don't need to test immutability of expected.
There was a problem hiding this comment.
Happy to have a proposed name change for applyItems(), but this has gone through 4ish patches now, and I think the Javadoc is fairly clear - this method applies the collector to the items specified in a way that validates that the Collector instance functions according to its contract. A more accurate name would probably be too unwieldy, and still require that the test author/reader check for themselves what it does (applyCollectorToItemsWithAndWithoutSplittingAndValidateResultsMatchExpected?), or we could make it only marginally more clear - but this would still require the reader to actually check the code to understand the implications (applyCollectorToItemsAndValidate, applyCollectorAndValidate, validateCollectorBehavior?)
I'll rename the lambda args.
There was a problem hiding this comment.
applyCollectorToItemsWithAndWithoutSplittingAndValidateResultsMatchExpected
Hehe..
I thought about unwrapping it, e.g.
var list1 = collectItemsWithSplitting(collector, items...)
var list2 = collectItemsWithoutSplitting(collector, items...)
assertAllImmutable(list1, list2);
assertAllEqual(expected, list1, list2)
or
var result = collectItems(collector, items...)
assertAllImmutable(result.withSplitting, result.withoutSplitting);
assertAllEqual(expected, result.withSplitting, result.withoutSplitting)
user/test/com/google/gwt/emultest/java10/util/stream/CollectorsTest.java
Show resolved
Hide resolved
|
Java10 suite passed locally, full build running at https://github.com/niloc132/gwt/actions/runs/7309660201. |
Partial #9547