You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/06/12 06:31:47 UTC
[GitHub] [kafka] clolov opened a new pull request, #12285: KAFKA-7342 Part 1: Straightforward JUnit4 to JUnit5 migrations
clolov opened a new pull request, #12285:
URL: https://github.com/apache/kafka/pull/12285
This is the first part of KAFKA-7342 (https://issues.apache.org/jira/browse/KAFKA-7342). Here are the decisions I made in this change:
* Do not (yet) migrate files using Parameterized because they require more in-depth changes. A list of the files which I haven't changed can be found here (1).
* Prefix assertTrue, assertFalse, assertNull etc. with Assertions.
* Run IntelliJ's Optimize Imports and streams:spotlessApply on the streams module.
1. List of untouched tests. Once these are migrated then JUnit4 should no longer be used in Kafka Streams
streams/src/test/java/org/apache/kafka/streams/integration/RocksDBMetricsIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/integration/EOSUncleanShutdownIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/integration/IQv2StoreIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/integration/KTableKTableForeignKeyJoinMaterializationIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/integration/EosIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/integration/KStreamRepartitionIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/integration/PositionRestartIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/integration/SlidingWindowedKStreamIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/integration/StreamTableJoinTopologyOptimizationIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/kstream/internals/KStreamSlidingWindowAggregateTest.java
streams/src/test/java/org/apache/kafka/streams/processor/internals/ActiveTaskCreatorTest.java
streams/src/test/java/org/apache/kafka/streams/processor/internals/StoreChangelogReaderTest.java
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/ChangeLoggingWindowBytesStoreTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/MergedSortedCacheWrappedWindowStoreIteratorTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/RocksDBGenericOptionsToDbOptionsColumnFamilyOptionsAdapterTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/RocksDBTimestampedSegmentedBytesStoreTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/SessionStoreFetchTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/TimestampedKeyValueStoreBuilderTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/WindowStoreFetchTest.java
streams/src/test/java/org/apache/kafka/streams/integration/GlobalKTableEOSIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/integration/KTableKTableForeignKeyJoinIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/integration/ResetPartitionTimeIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/integration/StreamStreamJoinIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/integration/TableTableJoinIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/kstream/internals/TimeWindowedKStreamImplTest.java
streams/src/test/java/org/apache/kafka/streams/integration/EosV2UpgradeIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/integration/KTableEfficientRangeQueryTest.java
streams/src/test/java/org/apache/kafka/streams/integration/RangeQueryIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/integration/StandbyTaskEOSIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/integration/SuppressionDurabilityIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/kstream/internals/KStreamWindowAggregateTest.java
streams/src/test/java/org/apache/kafka/streams/processor/internals/ProcessorStateManagerTest.java
streams/src/test/java/org/apache/kafka/streams/processor/internals/StoreToProcessorContextAdapterTest.java
streams/src/test/java/org/apache/kafka/streams/processor/internals/TimestampedKeyValueStoreMaterializerTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/KeyValueIteratorFacadeTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/MergedSortedCacheWrappedWindowStoreKeyValueIteratorTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/RocksDBSegmentedBytesStoreTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/RocksDBWindowStoreTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/TimeOrderedCachingPersistentWindowStoreTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/TimestampedWindowStoreBuilderTest.java
streams/src/test/java/org/apache/kafka/streams/processor/internals/RepartitionTopicsTest.java
streams/src/test/java/org/apache/kafka/streams/processor/internals/StreamTaskTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/ChangeLoggingSessionBytesStoreTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/KeyValueStoreBuilderTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/ReadOnlyKeyValueStoreFacadeTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/RocksDBSessionStoreTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/SessionKeySchemaTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/TimeOrderedKeyValueBufferTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/WindowKeySchemaTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/metrics/RocksDBMetricsRecorderTest.java
streams/src/test/java/org/apache/kafka/streams/integration/StreamTableJoinIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/integration/TimeWindowedKStreamIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/kstream/internals/graph/TableSourceNodeTest.java
streams/src/test/java/org/apache/kafka/streams/processor/internals/StandbyTaskTest.java
streams/src/test/java/org/apache/kafka/streams/processor/internals/StreamsPartitionAssignorTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/ChangeLoggingTimestampedWindowBytesStoreTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/ListValueStoreTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/ReadOnlyWindowStoreFacadeTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/RocksDBTimeOrderedWindowSegmentedBytesStoreTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/SessionStoreBuilderTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/TimeOrderedWindowStoreTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/WindowStoreBuilderTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/AbstractRocksDBSegmentedBytesStoreTest.java
streams/src/test/java/org/apache/kafka/streams/integration/AbstractJoinIntegrationTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/AbstractSessionBytesStoreTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/AbstractDualSchemaRocksDBSegmentedBytesStoreTest.java
streams/src/test/java/org/apache/kafka/streams/state/internals/AbstractWindowBytesStoreTest.java
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] clolov commented on pull request #12285: KAFKA-14001: Migrate streams module to JUnit 5 - Part 1
Posted by GitBox <gi...@apache.org>.
clolov commented on PR #12285:
URL: https://github.com/apache/kafka/pull/12285#issuecomment-1195428834
Hello @cadonna! I spoke with Divij offline and we found a way to run both JUnit 4 and JUnit 5 tests while the migration is going on. The pull request which should enable this is https://github.com/apache/kafka/pull/12441.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] clolov commented on a diff in pull request #12285: KAFKA-14001: Migrate streams module to JUnit 5 - Part 1
Posted by GitBox <gi...@apache.org>.
clolov commented on code in PR #12285:
URL: https://github.com/apache/kafka/pull/12285#discussion_r926625230
##########
streams/src/test/java/org/apache/kafka/streams/integration/ErrorHandlingIntegrationTest.java:
##########
@@ -52,36 +52,46 @@
import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.MatcherAssert.assertThat;
+@Timeout(600)
@Category(IntegrationTest.class)
public class ErrorHandlingIntegrationTest {
+ private final String testId;
+ private final String appId;
+ private final Properties properties;
+
+ // Task 0
+ private final String inputTopic;
+ private final String outputTopic;
+ // Task 1
+ private final String errorInputTopic;
+ private final String errorOutputTopic;
+
private static final EmbeddedKafkaCluster CLUSTER = new EmbeddedKafkaCluster(1);
- @BeforeClass
+ ErrorHandlingIntegrationTest(final TestInfo testInfo) {
+ testId = safeUniqueTestName(getClass(), testInfo);
+ appId = "appId_" + testId;
+ properties = props();
+
+ inputTopic = "input" + testId;
+ outputTopic = "output" + testId;
+
+ errorInputTopic = "error-input" + testId;
+ errorOutputTopic = "error-output" + testId;
+ }
Review Comment:
I cannot remember, so maybe there was no reason. When I was rebasing I noticed that this test has since been deleted (or moved to `org.apache.kafka.connect.integration`) so I believe it is safe to say I do not need to address this?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #12285: KAFKA-14001: Migrate streams module to JUnit 5 - Part 1
Posted by GitBox <gi...@apache.org>.
ijuma commented on PR #12285:
URL: https://github.com/apache/kafka/pull/12285#issuecomment-1193073368
For other modules, we did the JUnit 5 migration in one PR to avoid this problem. As @divijvaidya mentioned, the build expects each module to be JUnit 4 or JUnit 5.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #12285: KAFKA-14001: Migrate streams module to JUnit 5 - Part 1
Posted by GitBox <gi...@apache.org>.
ijuma commented on PR #12285:
URL: https://github.com/apache/kafka/pull/12285#issuecomment-1195515462
Doing things incrementally over a long period of time is a bit confusing since you cannot enforce that JUnit 4 or JUnit 5 is used at any point in time. My take is that it's simplest if the conversion is done quickly. However, it's best to convert the tests to Mockito first - that part is more complicated and can be done incrementally.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna merged pull request #12285: KAFKA-14001: Migrate streams module to JUnit 5 - Part 1
Posted by GitBox <gi...@apache.org>.
cadonna merged PR #12285:
URL: https://github.com/apache/kafka/pull/12285
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] clolov commented on pull request #12285: KAFKA-7342 Part 1: Straightforward JUnit4 to JUnit5 migrations
Posted by GitBox <gi...@apache.org>.
clolov commented on PR #12285:
URL: https://github.com/apache/kafka/pull/12285#issuecomment-1153573803
Okay, I will work on 1 and 3, thank you for the review.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on pull request #12285: KAFKA-14001: Migrate streams module to JUnit 5 - Part 1
Posted by GitBox <gi...@apache.org>.
cadonna commented on PR #12285:
URL: https://github.com/apache/kafka/pull/12285#issuecomment-1191451288
Once I get acceptable builds, I am going to merge this PR!
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on pull request #12285: KAFKA-14001: Migrate streams module to JUnit 5 - Part 1
Posted by GitBox <gi...@apache.org>.
cadonna commented on PR #12285:
URL: https://github.com/apache/kafka/pull/12285#issuecomment-1190036847
@clolov could you please rebase your PR since there are conflicts?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on pull request #12285: KAFKA-14001: Migrate streams module to JUnit 5 - Part 1
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on PR #12285:
URL: https://github.com/apache/kafka/pull/12285#issuecomment-1193821477
@clolov we don't have to do it in one go. We can create a new project/task in `build.gradle` which runs streams `integration tests` tagged with `@tag('integration')` using Junit5. When we are done with Junit4 to Junit5 migration, we can remove that project/task use the usual `integrationTest` task.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] clolov commented on pull request #12285: KAFKA-14001: Migrate streams module to JUnit 5 - Part 1
Posted by GitBox <gi...@apache.org>.
clolov commented on PR #12285:
URL: https://github.com/apache/kafka/pull/12285#issuecomment-1191436663
Hey @cadonna, I rebased on top of trunk. Do let me know if there is something else you would like me to address :)
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] clolov commented on pull request #12285: KAFKA-14001: Migrate streams module to JUnit 5 - Part 1
Posted by GitBox <gi...@apache.org>.
clolov commented on PR #12285:
URL: https://github.com/apache/kafka/pull/12285#issuecomment-1195674893
Divij and I also prefer option 3 because of the same reasons posted by Bruno. So the next steps on our side are:
1) Address comments on https://github.com/apache/kafka/pull/12441 which should bring us back to a state where tests are ran on every pull request
2) Continue with the smaller pull requests for unit test migration from JUnit 4 to JUnit 5 which do not depend on PowerMock/EasyMock
3) Raise (multiple) pull request(s) to move PowerMock/EasyMock tests in Streams to Mockito
4) Once 3 is done prepare pull request(s) for them to be upgraded to JUnit 5
5) Cleanup the setup which allows JUnit 4 tests to run in Streams + any new tests which might have appeared in the interim
I will be away until 2nd of August so Divij will provide updates on https://github.com/apache/kafka/pull/12441. Once I am back I will continue with the plan as detailed above. Of course, if there are any suggestions for it to be done otherwise I am more than happy to discuss those.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #12285: KAFKA-14001: Migrate streams module to JUnit 5 - Part 1
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #12285:
URL: https://github.com/apache/kafka/pull/12285#discussion_r898982426
##########
streams/src/test/java/org/apache/kafka/streams/integration/utils/IntegrationTestUtils.java:
##########
@@ -236,6 +238,16 @@ public static String safeUniqueTestName(final Class<?> testClass, final TestName
.replace('=', '_');
}
+ public static String safeUniqueTestName(final Class<?> testClass, final TestInfo testInfo) {
Review Comment:
please add documentation
##########
streams/src/test/java/org/apache/kafka/streams/integration/RestoreIntegrationTest.java:
##########
@@ -87,37 +85,34 @@
import static org.apache.kafka.streams.integration.utils.IntegrationTestUtils.waitForStandbyCompletion;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.core.IsEqual.equalTo;
-import static org.junit.Assert.assertTrue;
+import static org.junit.jupiter.api.Assertions.assertTrue;
-@Category({IntegrationTest.class})
+@Timeout(600)
+@Tag("integration")
Review Comment:
I understand that this change is necessary for JUnit5 migration but in it's current state if we merge this PR, these tests will not execute. Here's why:
In `build.gradle`, these tests are marked for execution with JUnit5 and hence will enter the `else` block in
```
if (shouldUseJUnit5) {
useJUnitPlatform {
includeTags "integration"
}
} else {
useJUnit {
includeCategories 'org.apache.kafka.test.IntegrationTest'
}
}
```
where they will try to find the previous `@Category({IntegrationTest.class})` but will not find it and hence, these tests will not execute.
My suggestion would be to keep `@Category({IntegrationTest.class})` in these tests and make all other changes.
Once we have all tests migrated to Junit5, create a PR where we remove the Junit5 eligibility from `build.gradle` and at the same time, we replace this annotation with `@Tag("integration")`
##########
streams/src/test/java/org/apache/kafka/streams/integration/utils/IntegrationTestUtils.java:
##########
@@ -236,6 +238,16 @@ public static String safeUniqueTestName(final Class<?> testClass, final TestName
.replace('=', '_');
}
+ public static String safeUniqueTestName(final Class<?> testClass, final TestInfo testInfo) {
Review Comment:
We can refactor the existing `safeUniqueTestName` as follows to extract out common parts such as:
```
public static String safeUniqueTestName(final Class<?> testClass, final TestName testName) {
return IntegrationTestUtils.safeUniqueTestName(testClass, testName.getMethodName())
}
public static String safeUniqueTestName(final Class<?> testClass, final TestInfo testInfo) {
return IntegrationTestUtils.safeUniqueTestName(testClass, testInfo.getTestMethod().map(Method::getName))
}
private static String safeUniqueTestName(final Class<?> testClass, final String methodName) {
...
}
```
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #12285: KAFKA-7342 Part 1: Straightforward JUnit4 to JUnit5 migrations
Posted by GitBox <gi...@apache.org>.
ijuma commented on PR #12285:
URL: https://github.com/apache/kafka/pull/12285#issuecomment-1156705224
> Worth putting in a separate PR, but have you tried enabling the Jupiter parallel test runner? When I ran it on a work project, it improved build times by an order of magnitude
We use multiple forks at the gradle level, so we should not enable this.
A couple more comments:
1. Let's revert the formatting changes.
2. Do not prefix with `Assertions`.
3. Mention in the PR title that these changes are for the streams module(s).
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] Kvicii commented on pull request #12285: KAFKA-7342 Part 1: Straightforward JUnit4 to JUnit5 migrations
Posted by GitBox <gi...@apache.org>.
Kvicii commented on PR #12285:
URL: https://github.com/apache/kafka/pull/12285#issuecomment-1153094346
@clolov thanks your PR, I think we should multiple modify.
@divijvaidya @dengziming @mimaison Do you have any better suggestion?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on pull request #12285: KAFKA-14001: Migrate streams module to JUnit 5 - Part 1
Posted by GitBox <gi...@apache.org>.
cadonna commented on PR #12285:
URL: https://github.com/apache/kafka/pull/12285#issuecomment-1191644117
@clolov @divijvaidya I just checked and these tests are not run in the builds. I have also another PR in streams with tests that use junit5 and they are also not run in the builds. Does anybody of you know what is going on here?
I do not want to to go on with the migration from junit4 to junit5 if tests are not run until we have migrated all tests.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on pull request #12285: KAFKA-14001: Migrate streams module to JUnit 5 - Part 1
Posted by GitBox <gi...@apache.org>.
cadonna commented on PR #12285:
URL: https://github.com/apache/kafka/pull/12285#issuecomment-1193822849
@clolov Let's first try to make it work with the junit vintage engine. I am not clear if there is still an issue with powermock and junit5 and it seems like Streams still uses powermock.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #12285: KAFKA-14001: Migrate streams module to JUnit 5 - Part 1
Posted by GitBox <gi...@apache.org>.
ijuma commented on PR #12285:
URL: https://github.com/apache/kafka/pull/12285#issuecomment-1195681900
Option 3 says not to continue JUnit 5 migrations though - it says to do the mockito migration first.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on pull request #12285: KAFKA-14001: Migrate streams module to JUnit 5 - Part 1
Posted by GitBox <gi...@apache.org>.
cadonna commented on PR #12285:
URL: https://github.com/apache/kafka/pull/12285#issuecomment-1191623444
Failures are not related
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on pull request #12285: KAFKA-7342 Part 1: Straightforward JUnit4 to JUnit5 migrations
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on PR #12285:
URL: https://github.com/apache/kafka/pull/12285#issuecomment-1153118745
Thanks you submitting this change @clolov. This would greatly help us in moving towards getting rid of JUnit4 completely.
1\ To make the code review easier, could we please tackle the `Run IntelliJ's Optimize Imports and streams:spotlessApply on the streams module.` in a separate review? I would drastically decrease the number of files changed and help us reviewing the code in a better manner.
2\ I agree with (and appreciate) your incremental approach towards this migration. We can handle the tests using parameterised separately.
3\ It would be helpful if you can document the high level changes required for the migration from Junit4 to Junit5 in the description of this PR.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] OneCricketeer commented on pull request #12285: KAFKA-7342 Part 1: Straightforward JUnit4 to JUnit5 migrations
Posted by GitBox <gi...@apache.org>.
OneCricketeer commented on PR #12285:
URL: https://github.com/apache/kafka/pull/12285#issuecomment-1155968908
Worth putting in a separate PR, but have you tried enabling the Jupiter parallel test runner? When I ran it on a work project, it improved build times by an order of magnitude
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on pull request #12285: KAFKA-7342 Part 1: Straightforward JUnit4 to JUnit5 migrations
Posted by GitBox <gi...@apache.org>.
cadonna commented on PR #12285:
URL: https://github.com/apache/kafka/pull/12285#issuecomment-1156391414
@clolov Thank you for the PR!
I agree with @divijvaidya about doing the reformatting in a separate PR. Could you also try to subdivide the PR into smaller PRs? Reviewing a 6500 line PR is never fun. Sizes around 500 are acceptable.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison commented on pull request #12285: KAFKA-7342 Part 1: Straightforward JUnit4 to JUnit5 migrations
Posted by GitBox <gi...@apache.org>.
mimaison commented on PR #12285:
URL: https://github.com/apache/kafka/pull/12285#issuecomment-1154027464
Thanks for the PR!
Since this PR only touches Streams, I'll let committers focused on Streams review it.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on a diff in pull request #12285: KAFKA-14001: Migrate streams module to JUnit 5 - Part 1
Posted by GitBox <gi...@apache.org>.
cadonna commented on code in PR #12285:
URL: https://github.com/apache/kafka/pull/12285#discussion_r925333066
##########
streams/src/test/java/org/apache/kafka/streams/integration/ErrorHandlingIntegrationTest.java:
##########
@@ -52,36 +52,46 @@
import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.MatcherAssert.assertThat;
+@Timeout(600)
@Category(IntegrationTest.class)
public class ErrorHandlingIntegrationTest {
+ private final String testId;
+ private final String appId;
+ private final Properties properties;
+
+ // Task 0
+ private final String inputTopic;
+ private final String outputTopic;
+ // Task 1
+ private final String errorInputTopic;
+ private final String errorOutputTopic;
+
private static final EmbeddedKafkaCluster CLUSTER = new EmbeddedKafkaCluster(1);
- @BeforeClass
+ ErrorHandlingIntegrationTest(final TestInfo testInfo) {
+ testId = safeUniqueTestName(getClass(), testInfo);
+ appId = "appId_" + testId;
+ properties = props();
+
+ inputTopic = "input" + testId;
+ outputTopic = "output" + testId;
+
+ errorInputTopic = "error-input" + testId;
+ errorOutputTopic = "error-output" + testId;
+ }
Review Comment:
Why did you not put this code into the `setup()` method?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on pull request #12285: KAFKA-14001: Migrate streams module to JUnit 5 - Part 1
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on PR #12285:
URL: https://github.com/apache/kafka/pull/12285#issuecomment-1192428154
Hey @clolov @cadonna
Yes, I have figured out why the newly converted tests to JUnit5 are not running. There are two reasons to it:
1. There is an overall block to use JUnit5 for streams project at https://github.com/apache/kafka/blob/trunk/build.gradle#L324. [Action] We need to remove streams from here.
2. The dependency chain does not pull in Jupiter engine dependency instead it pulls in Jupiter API dependency at https://github.com/apache/kafka/blob/trunk/build.gradle#L1839 [Action] We need to replace this with `testImplementation libs.junitJupiter`
I made the above two changes and ensured that the modified test is run using `./gradlew streams:test --tests AdjustStreamThreadCountTest`
With the above changes, we will still have tests which won't execute during build because at https://github.com/apache/kafka/blob/trunk/build.gradle#L466, we will only run tests with "integration" tag. A simple way to fix it is to run all Junit4 and Junit5 tests using the Jupiter platform. It is allowed if we use the [JUnitVintageEngine](https://junit.org/junit5/docs/current/user-guide/#migrating-from-junit4). I will file a pull request soon to fix what I mentioned above.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] clolov commented on pull request #12285: KAFKA-14001: Migrate streams module to JUnit 5 - Part 1
Posted by GitBox <gi...@apache.org>.
clolov commented on PR #12285:
URL: https://github.com/apache/kafka/pull/12285#issuecomment-1192206399
I don't know, but I can have a look in the upcoming days.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] clolov commented on pull request #12285: KAFKA-7342 Part 1: Straightforward JUnit4 to JUnit5 migrations
Posted by GitBox <gi...@apache.org>.
clolov commented on PR #12285:
URL: https://github.com/apache/kafka/pull/12285#issuecomment-1156744613
Hello! Thank you to everyone who has left a comment and suggestions for improvement. In the next few days I will aim to rework this pull request. In summary:
* I will revert the import reordering
* I will not prefix assertions with Assertions
* I will mention that these changes are for the streams module
* I will split the PR into multiple ones so to stick to the <= 500 lines rule
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #12285: KAFKA-14001: Migrate streams module to JUnit 5 - Part 1
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #12285:
URL: https://github.com/apache/kafka/pull/12285#discussion_r898989140
##########
streams/src/test/java/org/apache/kafka/streams/integration/RestoreIntegrationTest.java:
##########
@@ -87,37 +85,34 @@
import static org.apache.kafka.streams.integration.utils.IntegrationTestUtils.waitForStandbyCompletion;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.core.IsEqual.equalTo;
-import static org.junit.Assert.assertTrue;
+import static org.junit.jupiter.api.Assertions.assertTrue;
-@Category({IntegrationTest.class})
+@Timeout(600)
+@Tag("integration")
Review Comment:
I understand that this change is necessary for JUnit5 migration but in it's current state if we merge this PR, these tests will not execute. Here's why:
In `build.gradle`, these tests are not marked for execution with JUnit5 and hence will enter the `else` block in
```
if (shouldUseJUnit5) {
useJUnitPlatform {
includeTags "integration"
}
} else {
useJUnit {
includeCategories 'org.apache.kafka.test.IntegrationTest'
}
}
```
where they will try to find the previous `@Category({IntegrationTest.class})` but will not find it and hence, these tests will not execute.
My suggestion would be to keep `@Category({IntegrationTest.class})` in these tests and make all other changes.
Once we have all tests migrated to Junit5, create a PR where we remove the Junit5 eligibility from `build.gradle` and at the same time, we replace this annotation with `@Tag("integration")`
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] clolov commented on pull request #12285: KAFKA-14001: Migrate streams module to JUnit 5 - Part 1
Posted by GitBox <gi...@apache.org>.
clolov commented on PR #12285:
URL: https://github.com/apache/kafka/pull/12285#issuecomment-1193806664
So then would you like me to coalesce https://github.com/apache/kafka/pull/12301 and https://github.com/apache/kafka/pull/12302 into one, add all remaining tests, make the changes to build.gradle and all other files into a single pull request?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on pull request #12285: KAFKA-14001: Migrate streams module to JUnit 5 - Part 1
Posted by GitBox <gi...@apache.org>.
cadonna commented on PR #12285:
URL: https://github.com/apache/kafka/pull/12285#issuecomment-1193824200
Personally, I would prefer to make the migration incrementally.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] clolov commented on pull request #12285: KAFKA-14001: Migrate streams module to JUnit 5 - Part 1
Posted by GitBox <gi...@apache.org>.
clolov commented on PR #12285:
URL: https://github.com/apache/kafka/pull/12285#issuecomment-1197688699
Well somewhere between reading the comments, having a discussion and typing my response I confused the options.
I don't see why allowing JUnit 4 tests to run alongside JUnit 5 tests albeit with a separate engine would be problematic while moving between them. After all, that's exactly what the vintage engine is supposed to do and what I as a developer want as an experience. I furthermore don't think enabling it on the overarching project level would be problematic. After all, there are two engines and JUnit 5 is smart enough to know which tests to run with which engine. To confirm/deny this statement I will add tests reports as Divij has suggested to the pull request for enabling the above change once I am back. What I am expecting the report to show is that more tests have been ran for streams and there has not been a change in the tests for other modules (barring flaky tests).
As to "PowerMock/EasyMock -> Mockito done first", "pure JUnit 4 -> JUnit 5 done first" I don't see why order matters. The sets of pure JUnit 4 tests and JUnit 4 + PowerMock/EasyMock tests are disjoint and can be addressed separately. In other words, I do not see reasons to reject pull requests (such as this one) given they advance the state of the world to JUnit 5.
As a summary, let me focus my efforts on https://github.com/apache/kafka/pull/12441 if it has not been agreed upon and merged by the time I am back. I believe we all agree that is a prerequisite to any incremental change. After that I will be opening pull requests for moving from PowerMock/EasyMock to Mockito and whatever tests can already be moved to JUnit 5 and you can review and merge them in whatever order you prefer :)
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] clolov commented on pull request #12285: KAFKA-14001: Migrate streams module to JUnit 5 - Part 1
Posted by GitBox <gi...@apache.org>.
clolov commented on PR #12285:
URL: https://github.com/apache/kafka/pull/12285#issuecomment-1180174715
Politely bumping the review @cadonna
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on pull request #12285: KAFKA-14001: Migrate streams module to JUnit 5 - Part 1
Posted by GitBox <gi...@apache.org>.
cadonna commented on PR #12285:
URL: https://github.com/apache/kafka/pull/12285#issuecomment-1195567473
I agree that the migration should be done quickly.
As far as I understand @clolov plans to do the whole migration quickly. Is this correct? If this is the case, I do not see the issue with making the migration incrementally for the sake of smaller PRs that are easier reviewed.
If you create one big PR for the migration you might run into the issue that during the review other PRs add tests in JUnit4. You would need to rebase and extend the PR to get green builds.
I agree that we should get the migration from EasyMock/PowerMock to Mockito done before we do the migration from JUnit4 to JUnit5 to make the reviews simpler.
So our option now are:
1. Revert this PR, do the migration to Mockito incrementally, and do the migration to JUnit 5 in one shot
2. Do the change in the builds to allow JUnit 5 along with JUnit 4 and do both migrations incrementally.
3. Do the change in the builds to allow JUnit 5 along with JUnit 4, migrate to Mockito, and then migrate all tests to JUnit 5.
For options 2 and 3 committers should be made aware to only accept JUnit 5 tests in new PRs. That should be easy for Streams.
I am in favor of 3 because it happened to me that I already added some tests in JUnit5 in some of my PRs because I was not aware that JUnit 4 and JUnit 5 do not run alongside each other in our builds. Migrating the JUnit 5 tests to JUnit 4 in my PRs do not seem to be a big deal, though.
I am also fine with option 1. Option 2 seems the oddest to me.
I would like to know the preferences of @clolov and @divijvaidya since they are driving this 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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #12285: KAFKA-14001: Migrate streams module to JUnit 5 - Part 1
Posted by GitBox <gi...@apache.org>.
ijuma commented on PR #12285:
URL: https://github.com/apache/kafka/pull/12285#issuecomment-1195649633
I am fine with 3 as long as we don't make changes that make it worse for modules that have already converted to JUnit 5.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] clolov commented on pull request #12285: KAFKA-7342 Part 1: Straightforward JUnit4 to JUnit5 migrations
Posted by GitBox <gi...@apache.org>.
clolov commented on PR #12285:
URL: https://github.com/apache/kafka/pull/12285#issuecomment-1153084216
Polite ask for a review from @divijvaidya @dengziming @Kvicii @mimaison. The decisions I have outlined in the description are reversible should there be objections to them :)
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] clolov commented on a diff in pull request #12285: KAFKA-14001: Migrate streams module to JUnit 5 - Part 1
Posted by GitBox <gi...@apache.org>.
clolov commented on code in PR #12285:
URL: https://github.com/apache/kafka/pull/12285#discussion_r899092186
##########
streams/src/test/java/org/apache/kafka/streams/integration/utils/IntegrationTestUtils.java:
##########
@@ -236,6 +238,16 @@ public static String safeUniqueTestName(final Class<?> testClass, final TestName
.replace('=', '_');
}
+ public static String safeUniqueTestName(final Class<?> testClass, final TestInfo testInfo) {
Review Comment:
Fair point, I will make the changes in the next commit.
##########
streams/src/test/java/org/apache/kafka/streams/integration/RestoreIntegrationTest.java:
##########
@@ -87,37 +85,34 @@
import static org.apache.kafka.streams.integration.utils.IntegrationTestUtils.waitForStandbyCompletion;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.core.IsEqual.equalTo;
-import static org.junit.Assert.assertTrue;
+import static org.junit.jupiter.api.Assertions.assertTrue;
-@Category({IntegrationTest.class})
+@Timeout(600)
+@Tag("integration")
Review Comment:
This is also a fair point, I will make the changes in the new commit.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] clolov commented on pull request #12285: KAFKA-14001: Migrate streams module to JUnit 5 - Part 1
Posted by GitBox <gi...@apache.org>.
clolov commented on PR #12285:
URL: https://github.com/apache/kafka/pull/12285#issuecomment-1157514249
@cadonna @ijuma @divijvaidya @Kvicii I hope the latest commits address all of the suggestions above :)
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org