-
Notifications
You must be signed in to change notification settings - Fork 6.9k
Refactor ClickHouse's nativeTest #35476
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
6227767 to
5465f08
Compare
linghengqian
left a comment
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.
- The Error Log is simply unbelievable.
./mvnw -PgenerateMetadata -e -T 1C clean testdoes not have this situation.
aused by: org.apache.maven.plugin.MojoExecutionException: Missing jar-file for org.apache.shardingsphere:shardingsphere-proxy-dialect-postgresql:jar:5.5.3-SNAPSHOT:test. Ensure that native-maven-plugin runs in package phase.
at org.graalvm.buildtools.maven.AbstractNativeImageMojo.processArtifact (AbstractNativeImageMojo.java:302)
at org.graalvm.buildtools.maven.AbstractNativeImageMojo.processSupportedArtifacts (AbstractNativeImageMojo.java:285)
at org.graalvm.buildtools.maven.AbstractNativeImageMojo.addArtifactToClasspath (AbstractNativeImageMojo.java:314)
at org.graalvm.buildtools.maven.AbstractNativeImageMojo.addDependenciesToClasspath (AbstractNativeImageMojo.java:366)
at org.graalvm.buildtools.maven.NativeTestMojo.addDependenciesToClasspath (NativeTestMojo.java:122)
at org.graalvm.buildtools.maven.AbstractNativeImageMojo.populateClasspath (AbstractNativeImageMojo.java:423)
at org.graalvm.buildtools.maven.AbstractNativeImageMojo.getClasspath (AbstractNativeImageMojo.java:430)
at org.graalvm.buildtools.maven.AbstractNativeImageMojo.getBuildArgs (AbstractNativeImageMojo.java:201)
at org.graalvm.buildtools.maven.AbstractNativeImageMojo.buildImage (AbstractNativeImageMojo.java:447)
at org.graalvm.buildtools.maven.NativeTestMojo.execute (NativeTestMojo.java:169)
at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo (DefaultBuildPluginManager.java:126)
at org.apache.maven.lifecycle.internal.MojoExecutor.doExecute2 (MojoExecutor.java:328)
at org.apache.maven.lifecycle.internal.MojoExecutor.doExecute (MojoExecutor.java:316)
at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:212)
at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:174)
at org.apache.maven.lifecycle.internal.MojoExecutor.access$000 (MojoExecutor.java:75)
at org.apache.maven.lifecycle.internal.MojoExecutor$1.run (MojoExecutor.java:162)
at org.apache.maven.plugin.DefaultMojosExecutionStrategy.execute (DefaultMojosExecutionStrategy.java:39)
at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:159)
at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject (LifecycleModuleBuilder.java:105)
at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject (LifecycleModuleBuilder.java:73)
at org.apache.maven.lifecycle.internal.builder.singlethreaded.SingleThreadedBuilder.build (SingleThreadedBuilder.java:53)
at org.apache.maven.lifecycle.internal.LifecycleStarter.execute (LifecycleStarter.java:118)
at org.apache.maven.DefaultMaven.doExecute (DefaultMaven.java:261)
at org.apache.maven.DefaultMaven.doExecute (DefaultMaven.java:173)
at org.apache.maven.DefaultMaven.execute (DefaultMaven.java:101)
at org.apache.maven.cli.MavenCli.execute (MavenCli.java:906)
at org.apache.maven.cli.MavenCli.doMain (MavenCli.java:283)
at org.apache.maven.cli.MavenCli.main (MavenCli.java:206)
at jdk.internal.reflect.DirectMethodHandleAccessor.invoke (DirectMethodHandleAccessor.java:103)
at java.lang.reflect.Method.invoke (Method.java:580)
at org.codehaus.plexus.classworlds.launcher.Launcher.launchEnhanced (Launcher.java:255)
at org.codehaus.plexus.classworlds.launcher.Launcher.launch (Launcher.java:201)
at org.codehaus.plexus.classworlds.launcher.Launcher.mainWithExitCode (Launcher.java:361)
at org.codehaus.plexus.classworlds.launcher.Launcher.main (Launcher.java:314)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
This pull request refactors ClickHouse's nativeTest setup by migrating to a Testcontainers-based JDBC driver and updating dependency versions. Key changes include:
- Replacing the old ClickHouse JDBC driver with org.testcontainers.jdbc.ContainerDatabaseDriver in the YAML configuration.
- Updating the test class (ClickHouseTest) to leverage the new Testcontainers driver and cleaning up container resources with ContainerDatabaseDriver.killContainers().
- Bumping dependency versions for Testcontainers and GraalVM Native Build Tools and updating related documentation.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/native/src/test/resources/test-native/yaml/jdbc/databases/clickhouse.yaml | Updated JDBC configuration to use Testcontainers driver and specified TC_INITSCRIPT. |
| test/native/src/test/resources/test-native/sql/clickhouse-init.sql | Added SQL initialization for ClickHouse with table definitions and truncation commands. |
| test/native/src/test/resources/META-INF/services/org.apache.shardingsphere.infra.database.core.type.DatabaseType | Registered the Testcontainers ClickHouse database type. |
| test/native/src/test/java/org/apache/shardingsphere/test/natived/jdbc/databases/ClickHouseTest.java | Refactored test logic including removal of hardcoded container management and use of ContainerDatabaseDriver.killContainers(). |
| test/native/src/test/java/org/apache/shardingsphere/test/natived/commons/algorithm/testcontainers/TcClickhouseDatabaseType.java | Introduced the dedicated Testcontainers ClickHouse database type implementation. |
| pom.xml and test/native/pom.xml | Updated dependency versions for Testcontainers and native-maven-plugin. |
| docs/* | Updated documentation to reflect the new ClickHouse image version and dependency upgrades. |
Comments suppressed due to low confidence (1)
test/native/src/test/java/org/apache/shardingsphere/test/natived/jdbc/databases/ClickHouseTest.java:52
- Consider verifying that calling ContainerDatabaseDriver.killContainers() in the afterEach hook effectively cleans up all Testcontainers resources and does not interfere with parallel test execution.
ContainerDatabaseDriver.killContainers();
test/native/src/test/resources/test-native/yaml/jdbc/databases/clickhouse.yaml
Show resolved
Hide resolved
4912b14 to
91e8805
Compare
For #29052.
Changes proposed in this pull request:
clickhouse/clickhouse-serveras Docker Image in ClickHouseProvider testcontainers/testcontainers-java#8738 , refactored to simplify nativeTest for clickhouse integration, which obviously helps to remove boilerplate code that is not convenient to understand.org.apache.shardingsphere:shardingsphere-infra-database-hiveno longer needs to be introduced separately.org.graalvm.buildtools:native-maven-plugin:0.10.6does not recognizepom.xmlwithout source code, which affects allorg.apache.shardingsphere:shardingsphere-proxy-dialect-*. See Native Build Tools cannot recognize Maven modules that only havepom.xmlbut no source code graalvm/native-build-tools#727 .Before committing this PR, I'm sure that I have checked the following options:
./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e.