Add Kotlin Sequence support for @TestFactory methods
#3377
Conversation
hanszt
force-pushed
the
feature/testfactory-kotlin-sequence-support
branch
10 times, most recently
from
8a0a767
to
52d6c31
Compare
hanszt
changed the title
junit-platform-commons/src/main/java/org/junit/platform/commons/util/CollectionUtils.java
Outdated
Show resolved
Hide resolved
junit-platform-commons/src/main/java/org/junit/platform/commons/util/CollectionUtils.java
Outdated
Show resolved
Hide resolved
hanszt
force-pushed
the
feature/testfactory-kotlin-sequence-support
branch
from
52d6c31
to
f32d798
Compare
|
Hello, I am curious if my changes in response to the review satisfy the requirements. I would love to hear what you think and if it can be merged to the main branch |
hanszt
force-pushed
the
feature/testfactory-kotlin-sequence-support
branch
from
f32d798
to
9e7a9a3
Compare
There was a problem hiding this comment.
@hanszt Sorry for the delay! I think this would be a very useful change.
| || (type.isArray() && type.getComponentType().isPrimitive())); | ||
| || (type.isArray() && type.getComponentType().isPrimitive())// | ||
| || Arrays.stream(type.getMethods())// | ||
| .filter(m -> m.getName().equals("iterator"))// |
There was a problem hiding this comment.
❓ If possible, I think we should check whether type implements kotlin.sequences.Sequence here because it's safer. We will need to defensively load the Kotlin type via reflection.
There was a problem hiding this comment.
I followed the same logic, the Kotlin language is following for for-loops, namely that anything that provides an iterator, can be iterated over using a for-loop. See also https://kotlinlang.org/docs/control-flow.html#for-loops.
I applied this logic to the conversion from anything that provides an iterator to convert to a stream. This also allows a Kotlin sequence to be converted to a stream. Don't you think it is safe enough to check whether the object to convert to a stream contains an iterator providing method with the name 'iterator' and actually provides a java.util.Iterator?
I'm not sure yet how to check via reflection if we are dealing with a Kotlin Sequence without having a dependency to the Kotlin standard library in the junit-platform-commons module.
| || Arrays.stream(type.getMethods())// | ||
| .filter(m -> m.getName().equals("iterator"))// | ||
| .map(Method::getReturnType)// | ||
| .anyMatch(returnType -> returnType == Iterator.class)); |
There was a problem hiding this comment.
❓ How does this work? Isn't the return type kotlin.collections.Iterator, not java.util.Iterator?
There was a problem hiding this comment.
This works because when Kotlin is used under the Jvm, some types are mapped to Java types. The kotlin.collections-Iterator is one of them and is mapped to java.util.Iterator. See also: https://kotlinlang.org/docs/java-interop.html#mapped-types.
|
No worries. I can imagine you are busy. I'm glad you think it is a good improvement. |
hanszt
force-pushed
the
feature/testfactory-kotlin-sequence-support
branch
from
9e7a9a3
to
4ce948f
Compare
sbrannen
changed the title
Sequence support for @TestFactory methods
Examples of benefits: - Kotlin Sequence support for @testfactory - Kotlin Sequence support for @MethodSource - Classes that expose an Iterator returning method, can be converted to a stream. Issue: junit-team#3376 I hereby agree to the terms of the JUnit Contributor License Agreement.
hanszt
force-pushed
the
feature/testfactory-kotlin-sequence-support
branch
from
4ce948f
to
8a9d199
Compare
|
@marcphilipp Thanks for the update. Congratz on the investment of Sovereign Tech Fund in Junit5 by the way! 🎉 |
Overview
Add support for converting
Iterator-providing classes to aStreaminorg.junit.platform.commons.util.CollectionUtils.Sequencesupport for@TestFactorymethods #3376I hereby agree to the terms of the JUnit Contributor License Agreement.
Definition of Done
@APIannotations