SteNicholas removed a comment on pull request #14120: URL: https://github.com/apache/flink/pull/14120#issuecomment-731045138
> Thanks @SteNicholas for opening this pr. In general the pr is good to me. I just have some minor comments. > > 1. Provide more precise filtering. Currently it is fine that we use the “typeutils” to filter because there are already some cases. (For example PrimitiveArraySerializerUpgradeTest.java and BasicTypeSerializerUpgradeTest.java). But I think the filter scope might be a little big. If somebody adds a serializer in the “typeutils” package and does not follow our rule we could not find it. > 2. Make the filter consistent in the ‘TypeInfoTestCoverageTest’ and ‘TypeSerializerTestCoverageTest’. I think we might use the same condition to filter the “test/itcase/innercalss”. > 3. Maybe we could split the commit to two. One is to add a test for StreamElement. One is to add the `‘TypeSerializerTestCoverageTest’` Thanks for @guoweiM detailed review. I have followed the second comment for the filter consistent. About the first point, the typeutils package shoud be excluded. Because based on `SerializerTestBase `, the typeutils package has missing tests as follows: ``` Could not find test 'org.apache.flink.api.common.typeutils.base.GenericArraySerializerTest' that covers 'org.apache.flink.api.common.typeutils.base.GenericArraySerializer'. Could not find test 'org.apache.flink.api.common.typeutils.SingleThreadAccessCheckingTypeSerializerTest' that covers 'org.apache.flink.api.common.typeutils.SingleThreadAccessCheckingTypeSerializer'. Could not find test 'org.apache.flink.api.common.typeutils.UnloadableDummyTypeSerializerTest' that covers 'org.apache.flink.api.common.typeutils.UnloadableDummyTypeSerializer'. Could not find test 'org.apache.flink.api.common.typeutils.base.VoidSerializerTest' that covers 'org.apache.flink.api.common.typeutils.base.VoidSerializer'. Could not find test 'org.apache.flink.api.java.typeutils.runtime.CopyableValueSerializerTest' that covers 'org.apache.flink.api.java.typeutils.runtime.CopyableValueSerializer'. Could not find test 'org.apache.flink.api.common.typeutils.base.NullValueSerializerTest' that covers 'org.apache.flink.api.common.typeutils.base.NullValueSerializer'. ``` About the third point, IMO, this could be included in this commit because the `TypeSerializerTestCoverageTest ` checkes these tests in typesutils package that the name of the tests are correct but not extend `SerializerTestBase`. Therefore, to pass the tests cases for `TypeSerializerTestCoverageTest`, these tests that the pull request included are necessary. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected]
