KAFKA-16921: Migrate all junit 4 code to junit 5 for connect module (part 1) by m1a2st · Pull Request #16253 · apache/kafka

m1a2st · 2024-06-08T16:53:55Z

https://issues.apache.org/jira/browse/KAFKA-16921

This PR modify connect module

chia7712 · 2024-06-08T17:26:27Z

The junit 4 assertions are not compatible to junit 5, so you ought to fix the conflicts :)

chia7712 · 2024-06-08T17:26:54Z

Also, please remove dependency "JUnit Vintage Engine" from connect module

chia7712 · 2024-06-09T01:25:39Z

Also, please replace @Category(IntegrationTest.class) by @Tag("integrationTest") for connect module

chia7712 · 2024-06-09T01:36:55Z

    exclude testsToExclude

-    if (shouldUseJUnit5)
-      useJUnitPlatform()


Please keep this line ...

Sorry, I misunderstand the shouldUseJUnit5 variable logic, so I rollback it

chia7712 · 2024-06-09T01:39:12Z

-                    tasks.values().stream().distinct().count());
-            assertTrue("Connectors are imbalanced: " + formatAssignment(connectors), maxConnectors - minConnectors < 2);
-            assertTrue("Tasks are imbalanced: " + formatAssignment(tasks), maxTasks - minTasks < 2);
+            assertNotEquals(maxConnectors, 0, "Found no connectors running!");


it should be assertNotEquals(0, maxConnectors, "Found no connectors running!");

chia7712 · 2024-06-09T01:39:16Z

-            assertTrue("Connectors are imbalanced: " + formatAssignment(connectors), maxConnectors - minConnectors < 2);
-            assertTrue("Tasks are imbalanced: " + formatAssignment(tasks), maxTasks - minTasks < 2);
+            assertNotEquals(maxConnectors, 0, "Found no connectors running!");
+            assertNotEquals(maxTasks, 0, "Found no tasks running!");


chia7712 · 2024-06-09T17:58:37Z

@m1a2st Could you please split this PR into small PRs (similar to #16253)? It is hard to complete the migration (and review) at once.

chia7712

@m1a2st thanks for updated PR. It seems those changed files are still using @RunWith(MockitoJUnitRunner.StrictStubs.class). Could you please update them?

chia7712 · 2024-06-11T07:24:07Z

          includeTags "integration"
          includeTags "org.apache.kafka.test.IntegrationTest"
-	  // Both engines are needed to run JUnit 4 tests alongside JUnit 5 tests.
+          // Both engines are needed to run JUnit 4 tests alongside JUnit 5 tests.


Please revert those unrelated change

chia7712 · 2024-06-11T07:24:17Z

          excludeTags "integration"
          excludeTags "org.apache.kafka.test.IntegrationTest"
-	  // Both engines are needed to run JUnit 4 tests alongside JUnit 5 tests.
+          // Both engines are needed to run JUnit 4 tests alongside JUnit 5 tests.


Please revert those unrelated change

chia7712 · 2024-06-11T07:26:45Z

@@ -362,24 +362,16 @@ private void verifyNormalConnector() throws InterruptedException {
    private void assertRequestTimesOut(String requestDescription, ThrowingRunnable request, String expectedTimeoutMessage) {


ThrowingRunnable should be replaced by Executable

chia7712 · 2024-06-11T07:29:19Z

    }

-}
+}


Please revert this unrelated change

chia7712 · 2024-06-11T07:29:40Z

        String measured = metrics.currentMetricValueAsString(taskGroup, name);
        assertEquals(expected, measured);
    }
-}


Please revert this unrelated change

chia7712 · 2024-06-11T07:30:33Z

        verify(herder).setClusterLoggerLevel(logger, level);
    }

-}


Please revert this unrelated change

m1a2st · 2024-06-11T13:38:58Z

@chia7712, Thanks for your comment, There are two classes still have @RunWith(MockitoJUnitRunner.StrictStubs.class) one is LoggingResourceTest and RestClientTest$Test, but these two classes modify to

@ExtendWith(MockitoExtension.class)
@MockitoSettings(strictness = Strictness.STRICT_STUBS)

gradle build test file will error, like

No matching tests found in any candidate test task.
    Requested tests:
        Test pattern org.apache.kafka.connect.runtime.rest.RestClientTest$RequestFailureParameterizedTest in task :connect:runtime:test

I think that it need to modify some gradle task to resolve it.

chia7712 · 2024-06-11T17:13:03Z

I think that it need to modify some gradle task to resolve it.

yep, connect:runtime can use junit 5 now. Sorry that my previous comment is not clear. Please complete following changes:

Please remove shouldUseJUnit5 from build.gradle.
please make sure connect:runtime has following dependencies:

    testImplementation libs.junitJupiter
    testImplementation libs.junitVintageEngine
    testImplementation libs.mockitoJunitJupiter
    testImplementation libs.mockitoCore

junitJupiterApi is included by libs.junitJupiter so we can remove it.

m1a2st · 2024-06-12T01:23:58Z

@chia7712, Thanks for your comment, I remove all @RunWith(MockitoJUnitRunner.StrictStubs.class), PTAL

chia7712

@m1a2st thanks for updated patch. I don't review all files, but it seems many files are still using junit 4 code. Could you please check them again?

chia7712 · 2024-06-12T06:51:56Z

@@ -1228,7 +1228,7 @@ private void testConfigProviderRegex(String rawConnConfig) {
    private void testConfigProviderRegex(String rawConnConfig, boolean expected) {


this test AbstractHerderTest is still using junit 4.

chia7712 · 2024-06-12T06:52:27Z

 import static org.apache.kafka.connect.runtime.TopicCreationConfig.PARTITIONS_CONFIG;
 import static org.apache.kafka.connect.runtime.TopicCreationConfig.REPLICATION_FACTOR_CONFIG;
 import static org.apache.kafka.connect.runtime.WorkerConfig.TOPIC_CREATION_ENABLE_CONFIG;
-import static org.junit.Assert.assertArrayEquals;


this test AbstractWorkerSourceTaskTest is still using junit 4.

chia7712 · 2024-06-12T06:52:47Z

@@ -43,7 +43,7 @@
 import org.junit.Before;
 import org.junit.Test;


chia7712

LGTM

@m1a2st thanks for reducing the size of PR.

@tag

commit f380cd1 Author: Edoardo Comar <ecomar@uk.ibm.com> Date: Thu Jun 13 15:01:08 2024 +0100 MINOR: Add integration tag to AdminFenceProducersIntegrationTest (apache#16326) Add @tag("integration") to AdminFenceProducersIntegrationTest Reviewers: Chris Egerton <chrise@aiven.io> commit 11c85a9 Author: Dongnuo Lyu <139248811+dongnuo123@users.noreply.github.com> Date: Thu Jun 13 05:11:01 2024 -0400 MINOR: Make online downgrade failure logs less noisy and update the timeouts scheduled in `convertToConsumerGroup` (apache#16290) This patch: - changes the order of the checks in `validateOnlineDowngrade`, so that only when the last member using the consumer protocol leave and the group still has classic member(s), `online downgrade is disabled` is logged if the policy doesn't allow downgrade. - changes the session timeout in `convertToConsumerGroup` from `consumerGroupSessionTimeoutMs` to `member.classicProtocolSessionTimeout().get()`. Reviewers: David Jacot <djacot@confluent.io> commit ea60666 Author: Ken Huang <100591800+m1a2st@users.noreply.github.com> Date: Thu Jun 13 17:11:37 2024 +0900 KAFKA-16921 [1/N] Migrate all junit 4 code to junit 5 for connect module (apache#16253) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> commit 596b945 Author: gongxuanzhang <gongxuanzhang@foxmail.com> Date: Thu Jun 13 15:39:32 2024 +0800 KAFKA-16643 Add ModifierOrder checkstyle rule (apache#15890) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> commit 103ff5c Author: Antoine Pourchet <antoine@responsive.dev> Date: Thu Jun 13 01:32:39 2024 -0600 KAFKA-15045: (KIP-924 pt. 24) internal TaskAssignor rename to LegacyTaskAssignor (apache#16318) Since the new public API for TaskAssignor shared a name, this rename will prevent users from confusing the internal definition with the public one. Reviewers: Anna Sophie Blee-Goldman <ableegoldman@apache.org> commit e59c887 Author: brenden20 <118419078+brenden20@users.noreply.github.com> Date: Thu Jun 13 02:30:05 2024 -0500 KAFKA-16557 Implemented OffsetFetchRequestState toStringBase and added a test for it (apache#16291) Reviewers: Lianet Magrans <lianetmr@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com> commit dd6fcc6 Author: TingIāu "Ting" Kì <kitingiao@gmail.com> Date: Thu Jun 13 14:35:33 2024 +0800 KAFKA-16901 Add unit tests for ConsumerRecords#records(String) (apache#16227) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> commit fe98888 Author: Lianet Magrans <98415067+lianetm@users.noreply.github.com> Date: Thu Jun 13 08:31:16 2024 +0200 MINOR: Improving log for outstanding requests on close and cleanup (apache#16304) Reviewers: Andrew Schofield <aschofield@confluent.io>, Chia-Ping Tsai <chia7712@gmail.com> commit 9ddd58b Author: Chris Egerton <chrise@aiven.io> Date: Thu Jun 13 05:43:33 2024 +0200 MINOR: Add readiness check for connector and separate Kafka cluster in ExactlyOnceSourceIntegrationTest::testSeparateOffsetsTopic (apache#16306) Reviewers: Greg Harris <gharris1727@gmail.com> commit 0a203a9 Author: TingIāu "Ting" Kì <kitingiao@gmail.com> Date: Thu Jun 13 09:47:51 2024 +0800 KAFKA-16938 non-dynamic props gets corrupted due to circular reference between DynamicBrokerConfig and DynamicConfig. (apache#16302) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> commit 6d1f8f8 Author: Gantigmaa Selenge <39860586+tinaselenge@users.noreply.github.com> Date: Thu Jun 13 02:42:39 2024 +0100 MINOR: Clean up for KafkaAdminClientTest (apache#16285) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> commit e76e1da Author: Chris Egerton <chrise@aiven.io> Date: Thu Jun 13 02:18:23 2024 +0200 KAFKA-16935: Automatically wait for cluster startup in embedded Connect integration tests (apache#16288) Reviewers: Greg Harris <gharris1727@gmail.com>

@tag

commit 4333af5 Author: A. Sophie Blee-Goldman <ableegoldman@gmail.com> Date: Thu Jun 13 11:27:50 2024 -0700 KAFKA-15045: (KIP-924 pt. 25) Rename old internal StickyTaskAssignor to LegacyStickyTaskAssignor (apache#16322) To avoid confusion in 3.8/until we fully remove all the old task assignors and internal config, we should rename the old internal assignor classes like the StickyTaskAssignor so that they won't be mixed up with the new version of the assignor (which is also named StickyTaskAssignor) Reviewers: Bruno Cadonna <cadonna@apache.org>, Josep Prat <josep.prat@aiven.io> commit f380cd1 Author: Edoardo Comar <ecomar@uk.ibm.com> Date: Thu Jun 13 15:01:08 2024 +0100 MINOR: Add integration tag to AdminFenceProducersIntegrationTest (apache#16326) Add @tag("integration") to AdminFenceProducersIntegrationTest Reviewers: Chris Egerton <chrise@aiven.io> commit 11c85a9 Author: Dongnuo Lyu <139248811+dongnuo123@users.noreply.github.com> Date: Thu Jun 13 05:11:01 2024 -0400 MINOR: Make online downgrade failure logs less noisy and update the timeouts scheduled in `convertToConsumerGroup` (apache#16290) This patch: - changes the order of the checks in `validateOnlineDowngrade`, so that only when the last member using the consumer protocol leave and the group still has classic member(s), `online downgrade is disabled` is logged if the policy doesn't allow downgrade. - changes the session timeout in `convertToConsumerGroup` from `consumerGroupSessionTimeoutMs` to `member.classicProtocolSessionTimeout().get()`. Reviewers: David Jacot <djacot@confluent.io> commit ea60666 Author: Ken Huang <100591800+m1a2st@users.noreply.github.com> Date: Thu Jun 13 17:11:37 2024 +0900 KAFKA-16921 [1/N] Migrate all junit 4 code to junit 5 for connect module (apache#16253) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> commit 596b945 Author: gongxuanzhang <gongxuanzhang@foxmail.com> Date: Thu Jun 13 15:39:32 2024 +0800 KAFKA-16643 Add ModifierOrder checkstyle rule (apache#15890) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> commit 103ff5c Author: Antoine Pourchet <antoine@responsive.dev> Date: Thu Jun 13 01:32:39 2024 -0600 KAFKA-15045: (KIP-924 pt. 24) internal TaskAssignor rename to LegacyTaskAssignor (apache#16318) Since the new public API for TaskAssignor shared a name, this rename will prevent users from confusing the internal definition with the public one. Reviewers: Anna Sophie Blee-Goldman <ableegoldman@apache.org> commit e59c887 Author: brenden20 <118419078+brenden20@users.noreply.github.com> Date: Thu Jun 13 02:30:05 2024 -0500 KAFKA-16557 Implemented OffsetFetchRequestState toStringBase and added a test for it (apache#16291) Reviewers: Lianet Magrans <lianetmr@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com> commit dd6fcc6 Author: TingIāu "Ting" Kì <kitingiao@gmail.com> Date: Thu Jun 13 14:35:33 2024 +0800 KAFKA-16901 Add unit tests for ConsumerRecords#records(String) (apache#16227) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> commit fe98888 Author: Lianet Magrans <98415067+lianetm@users.noreply.github.com> Date: Thu Jun 13 08:31:16 2024 +0200 MINOR: Improving log for outstanding requests on close and cleanup (apache#16304) Reviewers: Andrew Schofield <aschofield@confluent.io>, Chia-Ping Tsai <chia7712@gmail.com> commit 9ddd58b Author: Chris Egerton <chrise@aiven.io> Date: Thu Jun 13 05:43:33 2024 +0200 MINOR: Add readiness check for connector and separate Kafka cluster in ExactlyOnceSourceIntegrationTest::testSeparateOffsetsTopic (apache#16306) Reviewers: Greg Harris <gharris1727@gmail.com> commit 0a203a9 Author: TingIāu "Ting" Kì <kitingiao@gmail.com> Date: Thu Jun 13 09:47:51 2024 +0800 KAFKA-16938 non-dynamic props gets corrupted due to circular reference between DynamicBrokerConfig and DynamicConfig. (apache#16302) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> commit 6d1f8f8 Author: Gantigmaa Selenge <39860586+tinaselenge@users.noreply.github.com> Date: Thu Jun 13 02:42:39 2024 +0100 MINOR: Clean up for KafkaAdminClientTest (apache#16285) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> commit e76e1da Author: Chris Egerton <chrise@aiven.io> Date: Thu Jun 13 02:18:23 2024 +0200 KAFKA-16935: Automatically wait for cluster startup in embedded Connect integration tests (apache#16288) Reviewers: Greg Harris <gharris1727@gmail.com>

m1a2st marked this pull request as draft June 8, 2024 16:55

m1a2st force-pushed the KAFKA-16921 branch from fd93670 to dd637de Compare June 8, 2024 18:12

m1a2st marked this pull request as ready for review June 8, 2024 18:14

chia7712 changed the title ~~KAFKA-16921: Migrate all junit 4 assert to junit 5 in connect module~~ KAFKA-16921: Migrate all junit 4 assert to junit 5 for connect module Jun 8, 2024

m1a2st force-pushed the KAFKA-16921 branch from dd637de to 35bd361 Compare June 9, 2024 01:14

chia7712 reviewed Jun 9, 2024

View reviewed changes

Comment thread build.gradle

chia7712 changed the title ~~KAFKA-16921: Migrate all junit 4 assert to junit 5 for connect module~~ KAFKA-16921: Migrate all junit 4 code to junit 5 for connect module Jun 9, 2024

chia7712 reviewed Jun 9, 2024

View reviewed changes

m1a2st force-pushed the KAFKA-16921 branch 2 times, most recently from e4865d4 to 502efcd Compare June 9, 2024 16:19

m1a2st changed the title ~~KAFKA-16921: Migrate all junit 4 code to junit 5 for connect module~~ KAFKA-16921: Migrate all junit 4 code to junit 5 for connect module (part 1) Jun 9, 2024

m1a2st force-pushed the KAFKA-16921 branch from 19822ec to 73e81e5 Compare June 10, 2024 00:54

C0urante reviewed Jun 10, 2024

View reviewed changes

Comment thread ...st/java/org/apache/kafka/connect/runtime/distributed/IncrementalCooperativeAssignorTest.java Outdated

C0urante reviewed Jun 10, 2024

View reviewed changes

Comment thread ...st/java/org/apache/kafka/connect/runtime/distributed/IncrementalCooperativeAssignorTest.java Outdated

chia7712 reviewed Jun 11, 2024

View reviewed changes

gharris1727 added connect tests Test fixes (including flaky tests) labels Jun 11, 2024

m1a2st force-pushed the KAFKA-16921 branch from 42ef1ac to b2371ba Compare June 12, 2024 01:15

chia7712 reviewed Jun 12, 2024

View reviewed changes

m1a2st added 2 commits June 12, 2024 21:50

modify build.gradle file

dc3282c

add cli policy converter modules:

794d6c9

m1a2st force-pushed the KAFKA-16921 branch from 62d0b9f to 794d6c9 Compare June 12, 2024 13:56

fix format

6ab875a

chia7712 approved these changes Jun 13, 2024

View reviewed changes

chia7712 merged commit ea60666 into apache:trunk Jun 13, 2024

gongxuanzhang mentioned this pull request Jun 15, 2024

KAFKA-16921: Migrate test of connect module to Junit5 (Runtime direct) #16351

Merged

		@@ -362,24 +362,16 @@ private void verifyNormalConnector() throws InterruptedException {
		private void assertRequestTimesOut(String requestDescription, ThrowingRunnable request, String expectedTimeoutMessage) {

                   }
-              }

                
                    No newline at end of file
+              }

		@@ -1228,7 +1228,7 @@ private void testConfigProviderRegex(String rawConnConfig) {
		private void testConfigProviderRegex(String rawConnConfig, boolean expected) {

		@@ -43,7 +43,7 @@
		import org.junit.Before;
		import org.junit.Test;

Conversation

m1a2st commented Jun 8, 2024

Uh oh!

chia7712 commented Jun 8, 2024

Uh oh!

chia7712 commented Jun 8, 2024

Uh oh!

Uh oh!

chia7712 commented Jun 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

chia7712 commented Jun 9, 2024

Uh oh!

Uh oh!

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

m1a2st commented Jun 11, 2024

Uh oh!

chia7712 commented Jun 11, 2024

Uh oh!

m1a2st commented Jun 12, 2024

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chia7712 commented Jun 9, 2024 •

edited

Loading