Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kafka-connect-runtime: remove code duplications in integration tests #11883

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wombatu-kun
Copy link

@wombatu-kun wombatu-kun commented Dec 28, 2024

I've found a lot of code duplicates in integration tests of module iceberg-kafka-connect-runtime. I propose to avoid such code duplication by making parent class IntegrationTestBase abstract and move all common logic (beforeEach, afterEach, creating kafka common config, running test, asserting snapshot is added) from IntegrationTest, IntegrationMultiTableTest and IntegrationDynamicTableTest to IntegrationTestBase.

@ParameterizedTest
@NullSource
@ValueSource(strings = "test_branch")
public void testIcebergSink(String branch) {
// partitioned table
catalog().createTable(TABLE_IDENTIFIER1, TestEvent.TEST_SCHEMA, TestEvent.TEST_SPEC);
// unpartitioned table
// non-partitioned table
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use "unpartitioned" throughout the Iceberg code, so please revert this change.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, didn't know that. reverted

}

@AfterEach
public void baseAfter() {
context().stopConnector(connectorName());
deleteTopic(testTopic());
catalog().dropTable(TableIdentifier.of(TEST_DB, TEST_TABLE1));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel the drop table belongs in the subclass.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree with you, fixed

@bryanck
Copy link
Contributor

bryanck commented Jan 2, 2025

Thanks @wombatu-kun , it mostly looks good, I added a couple of comments.

@wombatu-kun wombatu-kun force-pushed the kafka-connect-runtime-integration-tests-refactoring branch from d82e94f to 1772bef Compare January 2, 2025 05:47
@wombatu-kun wombatu-kun requested a review from bryanck January 2, 2025 07:21
@@ -55,6 +61,19 @@ public class IntegrationTestBase {
private KafkaProducer<String, String> producer;

protected static final int TEST_TOPIC_PARTITIONS = 2;
protected static final String TEST_DB = "test";
protected static final String TEST_TABLE1 = "foobar1";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel we should keep the table name constants in the subclasses as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants