snuyanzin commented on PR #20991:
URL: https://github.com/apache/flink/pull/20991#issuecomment-1330839374
hi @ashmeet-kandhari
thanks for conflicts resolution
I left some comments (i didn't go through the whole changeset)
however there are several types of things which would be nice to address
1. Since junit5 classes and tests method could be package private (may be
except some cases with hierarchies). So would nice to have it fixed
2. Using the `META-INF/services/org.junit.jupiter.api.extension.Extension`
is the better approach here than using annotation
`@ExtendWith(TestLoggerExtension.class)` for every class.
3. in junit5 timeout unit by default is seconds while in junit4 it is
millis. It looks like this should be taken into account or may be better set it
explicitly like `@Timeout(value = 30_000, unit = TimeUnit.MILLISECONDS)`
4. I noticed there are tests failing would be nice to understand if it is
result of changes within this PR or something else
5. GitHub still shows changes for `docs/themes/book` in this PR. It should
be reverted since it has nothing to do with kafka-connector to junit5 migration
--
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]
