-
Notifications
You must be signed in to change notification settings - Fork 1
Clickhouse support for tests #21
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds support for embedded ClickHouse instances in tests, refactors Flink cluster setup, and extends CI to run tests across multiple ClickHouse versions including cloud.
- Introduce
ClickHouseTestHelpersandClickHouseServerForTestsfor managing test ClickHouse instances. - Refactor
EmbeddedFlinkClusterForTestsfor manual lifecycle control and add a combinedFlinkClusterTestsclass. - Update
DummyFlinkClusterTestwith new ClickHouse assertions and extend GitHub Actions to matrix-test against different ClickHouse versions.
Reviewed Changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| flink-connector-clickhouse-base/src/test/java/org/apache/flink/connector/test/embedded/flink/EmbeddedFlinkClusterForTests.java | Changed package, made lifecycle methods public for manual invocation |
| flink-connector-clickhouse-base/src/test/java/org/apache/flink/connector/test/embedded/clickhouse/ClickHouseTestHelpers.java | Added utility methods/constants for ClickHouse versions and client setup |
| flink-connector-clickhouse-base/src/test/java/org/apache/flink/connector/test/embedded/clickhouse/ClickHouseServerForTests.java | New class to start/stop an embedded or cloud ClickHouse server and run SQL |
| flink-connector-clickhouse-base/src/test/java/org/apache/flink/connector/test/FlinkClusterTests.java | Wrapper test class to orchestrate both Flink and ClickHouse lifecycles |
| flink-connector-clickhouse-base/src/test/java/org/apache/flink/connector/test/DummyFlinkClusterTest.java | Updated to extend FlinkClusterTests, removed redundant imports, added ClickHouse test |
| .github/workflows/tests.yaml | Extended CI with version matrix, added conditional skip for missing cloud credentials |
Files not reviewed (2)
- flink-connector-clickhouse-base/build.gradle.kts: Language not supported
- flink-connector-clickhouse-base/src/test/resources/log4j.properties: Language not supported
Comments suppressed due to low confidence (3)
flink-connector-clickhouse-base/src/test/java/org/apache/flink/connector/test/embedded/clickhouse/ClickHouseServerForTests.java:78
- The method name getDataBase() uses an inconsistent camel-case for 'database'. Rename it to getDatabase() for clarity and consistency.
public static String getDataBase() { return database; }
flink-connector-clickhouse-base/src/test/java/org/apache/flink/connector/test/DummyFlinkClusterTest.java:49
- The test only asserts on the collection size and no longer checks the actual mapped values. Reintroduce assertions for the expected output values to improve coverage and correctness.
Assertions.assertEquals(2, CollectSink.values.size());
.github/workflows/tests.yaml:23
- The conditional
if: env.SKIP_STEP != 'true'on the checkout step will skip checking out the repository when testing cloud, causing subsequent steps to fail. You should always run checkout before conditionally skipping test execution.
- uses: actions/checkout@v3
|
|
||
| import org.apache.flink.api.common.functions.MapFunction; | ||
| import org.apache.flink.connector.test.embedded.clickhouse.ClickHouseServerForTests; | ||
| import org.apache.flink.connector.test.embedded.flink.EmbeddedFlinkClusterForTests; |
Copilot
AI
May 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This import is no longer used after changing the base class to FlinkClusterTests. Consider removing it to clean up unused imports.
| import org.apache.flink.connector.test.embedded.flink.EmbeddedFlinkClusterForTests; |
Summary
Closes #6
Delete items not relevant to your PR: